[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] <AWT Dev> [9] Review request for 8076545 Text size is twice bigger under Window
From: Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date: 2016-03-15 11:48:04
Message-ID: 56E7F674.2010207 () oracle ! com
[Download RAW message or body]
On 11.03.16 21:54, Alexandr Scherbatiy wrote:
> I need one more reviewer for the fix.
Looks fine.
>
> On 2/15/2016 11:24 AM, Jim Graham wrote:
>> Thanks Alexandr,
>>
>> The AWT changes all look good.
>>
>> I'll leave it to others as to whether the test case represents the
>> best way to test this. I have no idea how those two font values in
>> those two L&F's in a Swing button should compare - maybe a difference
>> of 8 is ordinary under some circumstances? Also, the test will only
>> be conclusive when run on a HiDPI display larger than a certain scale
>> factor so it may not even be testing anything in most test environments.
>
> Should I add a manual test where it will be required to run it on
> HiDPI displays?
>
> Thanks,
> Alexandr.
>
>>
>> In either case, the AWT code changes look fine...
>>
>> ...jim
>>
>> On 2/12/16 12:54 PM, Alexandr Scherbatiy wrote:
>>> On 2/8/2016 3:04 PM, Jim Graham wrote:
>>>> I don't understand the issue with the fonts that you are saying have
>>>> different sizes for different DPIs. Those are pixel sizes, aren't
>>>> they? They still need to be turned into user-space units for our
>>>> applications to know what to do with them. Or, are you saying that
>>>> they are not representing pixel sizes as the rest of the font units do
>>>> and so they are already in DPI-relative "user space" units?
>>>>
>>>> AT 192 DPI, oemFixed.font is 18, but are they saying 18 pixels? Which
>>>> would be a 9-point font in our automatically scaled coordinate
>>>> systems, wouldn't it?
>>>
>>> The font size in the AwtDesktopProperties is calculated as (tmHeight
>>> - tmInternalLeading) values from TEXTMETRIC:
>>> jint pointSize = metrics.tmHeight - metrics.tmInternalLeading;
>>> According to MSDN "All sizes are specified in logical units; that is,
>>> they depend on the current mapping mode of the display context."
>>>
>>> The calculated size value is proportional to the scale factor for the
>>> fonts like win.frame.captionFont :
>>> DPI 96 (scale 100%), size: 15
>>> DPI 144 (scale 150%), size: 23
>>> DPI 192 (scale 200%), size: 30
>>>
>>> That means that for each DPI it is possible to scale down the font
>>> size to its base value just using the formula:
>>> baseFontSize = fontSize / scaleFactor
>>>
>>> However, for win.ansiFixed.font the TEXTMETRIC returns the same values
>>> for each display DPI:
>>> DPI 96 (scale 100%), size: 13
>>> DPI 144 (scale 150%), size: 13
>>> DPI 192 (scale 200%), size: 13
>>>
>>> Now scaling the size down for DPI 192 ( 13/ 2) it does not give the
>>> font size for the DPI 96.
>>> That is why the scale factor 1.0 is used in this case in the fix.
>>>
>>> Thanks,
>>> Alexandr.
>>>
>>>>
>>>> ...jim
>>>>
>>>> On 2/6/16 7:19 AM, Alexander Scherbatiy wrote:
>>>>> On 2/5/2016 11:37 AM, Jim Graham wrote:
>>>>>> Hi Alexandr,
>>>>>>
>>>>>> awt_DesktopProperties.cpp, line 300 - is the "1.0f /" a typo?
>>>>> Sorry. Here is the updated fix without the typo:
>>>>> http://cr.openjdk.java.net/~alexsch/8076545/webrev.06/
>>>>>>
>>>>>> Also, is there a still a need for the setFontProperty() variants that
>>>>>> don't have a scale as the last parameter?
>>>>> There are fonts like win.ansiFixed.font, win.ansiVar.font, and
>>>>> win.deviceDefault.font which size is the same for any display DPI.
>>>>>
>>>>> And there are fonts like win.oemFixed.font, win.system.font, and
>>>>> win.systemFixed.font which have one size for DPI 96 and another
>>>>> size for
>>>>> all other DPI.
>>>>> For example win.oemFixed.font:
>>>>> DPI 96, size: 12
>>>>> DPI 144, size: 18
>>>>> DPI 192, size: 18
>>>>> DPI 240, size: 18
>>>>>
>>>>> I left them unscaled but may be it is better to have one
>>>>> precalculated scale for this fonts which is used when DPI is not 96.
>>>>>
>>>>> I updated the setFontProperty() method without scale parameter
>>>>> usage
>>>>> to call with scale 1.0f.
>>>>>
>>>>> Thanks,
>>>>> Alexandr.
>>>>>
>>>>>>
>>>>>> ...jim
>>>>>>
>>>>>> On 2/5/2016 2:12 AM, Alexandr Scherbatiy wrote:
>>>>>>>
>>>>>>> Could you review the updated fix:
>>>>>>> http://cr.openjdk.java.net/~alexsch/8076545/webrev.05
>>>>>>>
>>>>>>> - The awt_DesktopProperties.cpp file is updated to use scaleX for
>>>>>>> width rescaling and scaleY for height rescaling
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>>> On 2/1/2016 5:51 PM, Jim Graham wrote:
>>>>>>>> Hi Alexandr,
>>>>>>>>
>>>>>>>> In awt_DesktopProperties.cpp you are using the Y scaling factor to
>>>>>>>> scale widths still - see lines 287 (and others in that same
>>>>>>>> function)
>>>>>>>> and then 322 in another function. It looks like you'll need
>>>>>>>> getInvScaleX() and getInvScaleY().
>>>>>>>>
>>>>>>>> I'll leave it to Phil to comment on the unit test...
>>>>>>>>
>>>>>>>> ...jim
>>>>>>>>
>>>>>>>> On 2/1/16 4:27 AM, Alexandr Scherbatiy wrote:
>>>>>>>>>
>>>>>>>>> Could you review the updated fix:
>>>>>>>>> http://cr.openjdk.java.net/~alexsch/8076545/webrev.04/
>>>>>>>>>
>>>>>>>>> - both LOGPIXELSX and Y are used for the theme size scaling.
>>>>>>>>> - LOGPIXELSY is used for text metrics height rescaling
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Alexandr.
>>>>>>>>>
>>>>>>>>> On 1/29/2016 1:16 PM, Jim Graham wrote:
>>>>>>>>>> Hi Alexandr,
>>>>>>>>>>
>>>>>>>>>> Thanks for investigating the behaviors.
>>>>>>>>>>
>>>>>>>>>> With respect to using LOGPIXELSX or Y, these methods are used to
>>>>>>>>>> scale
>>>>>>>>>> both horizontal and vertical measurements so we really should
>>>>>>>>>> have 2
>>>>>>>>>> scale values and rescale methods, one for horizontal use and one
>>>>>>>>>> for
>>>>>>>>>> vertical...
>>>>>>>>>>
>>>>>>>>>> ...jim
>>>>>>>>>>
>>>>>>>>>> On 1/29/2016 10:41 AM, Alexandr Scherbatiy wrote:
>>>>>>>>>>> Could you review the updated fix:
>>>>>>>>>>> http://cr.openjdk.java.net/~alexsch/8076545/webrev.03
>>>>>>>>>>>
>>>>>>>>>>> - LOGPIXELSX is changed to LOGPIXELSY
>>>>>>>>>>>
>>>>>>>>>>> On 11/16/2015 1:43 PM, Jim Graham wrote:
>>>>>>>>>>>> Note that LOGPIXELSY is global and static between reboots so it
>>>>>>>>>>>> doesn't really matter which monitor is used to get the value.
>>>>>>>>>>>>
>>>>>>>>>>>> Also, the issue is that the measurements are in pixels, so
>>>>>>>>>>>> if we
>>>>>>>>>>>> convert them into a resolution independent measurement then the
>>>>>>>>>>>> rest
>>>>>>>>>>>> of the scaling in the AWT/2D will be correct regardless of any
>>>>>>>>>>>> given
>>>>>>>>>>>> monitor's resolution. We just have to make sure the
>>>>>>>>>>>> "de-pixelization"
>>>>>>>>>>>> of the measurement is an apples-to-apples calculation.
>>>>>>>>>>>>
>>>>>>>>>>>> The question in my mind is whether the values they get from
>>>>>>>>>>>> GetTheme*() and SPI_GETNONCLIENTMETRICS are relative to
>>>>>>>>>>>> LOGPIXELSY, or
>>>>>>>>>>>> the more recent Per-Monitor aware DPI APIs, or ...? It would be
>>>>>>>>>>>> interesting to see what happens to those values when you
>>>>>>>>>>>> change the
>>>>>>>>>>>> DPI settings on Windows 8.1 and not reboot. If they stay
>>>>>>>>>>>> constant
>>>>>>>>>>>> until you reboot then LOGPIXELSY on the main screen would be
>>>>>>>>>>>> the
>>>>>>>>>>>> value
>>>>>>>>>>>> to use to de-scale them...
>>>>>>>>>>>
>>>>>>>>>>> I tried to use the "Change the size of all items" slider on
>>>>>>>>>>> Windows
>>>>>>>>>>> 8.1 without rebooting.
>>>>>>>>>>> GetDpiForMonitor() returns the updated DPI values: 192,
>>>>>>>>>>> 144, 96
>>>>>>>>>>> LOGPIXELSY returns unchanged values: 192, 192, 192
>>>>>>>>>>> SystemParametersInfo(SPI_GETNONCLIENTMETRICS,...) returns
>>>>>>>>>>> unchanged
>>>>>>>>>>> NONCLIENTMETRICS
>>>>>>>>>>> GetThemePartSize() returns unchanged theme part sizes
>>>>>>>>>>>
>>>>>>>>>>> It seems that theme part sizes behave in the same way as the
>>>>>>>>>>> LOGPIXELSY in this case.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Alexandr.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ...jim
>>>>>>>>>>>>
>>>>>>>>>>>> On 11/16/2015 12:51 PM, Phil Race wrote:
>>>>>>>>>>>>> That seems better. But one more question to get a point
>>>>>>>>>>>>> clarified.
>>>>>>>>>>>>> You are using getDesktopWindow() which is for the primary
>>>>>>>>>>>>> monitor.
>>>>>>>>>>>>> I assume that the 'inversion' results in the value being used
>>>>>>>>>>>>> to be
>>>>>>>>>>>>> independent
>>>>>>>>>>>>> of the monitor in a multi-mon situation and so when we move to
>>>>>>>>>>>>> a 2nd
>>>>>>>>>>>>> monitor
>>>>>>>>>>>>> that inverted size remains valid ?
>>>>>>>>>>>>>
>>>>>>>>>>>>> If so looks good to me.
>>>>>>>>>>>>>
>>>>>>>>>>>>> -phil.
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 11/16/2015 06:07 AM, Alexander Scherbatiy wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Could you review the updated fix:
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~alexsch/8076545/webrev.02
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> - round is used instead of ceil
>>>>>>>>>>>>>> - inverted scales are used
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 10/30/2015 10:40 PM, Jim Graham wrote:
>>>>>>>>>>>>>>> In this case round may be better. ceil() is more for cases
>>>>>>>>>>>>>>> where you
>>>>>>>>>>>>>>> needed "at least X amount of room", but I don't think a font
>>>>>>>>>>>>>>> size is
>>>>>>>>>>>>>>> an "at least this much" kind of case.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Also, I've been toying with the idea that use of ceil() and
>>>>>>>>>>>>>>> floor()
>>>>>>>>>>>>>>> in any DPI-adjustment equations should really be "ceil(val -
>>>>>>>>>>>>>>> epsilon)" or "floor(ceil + epsilon)" for some small value of
>>>>>>>>>>>>>>> epsilon
>>>>>>>>>>>>>>> chosen just large enough to prevent various round-off errors
>>>>>>>>>>>>>>> from
>>>>>>>>>>>>>>> affecting the outcome. One idea is for 1/256 as the
>>>>>>>>>>>>>>> value of
>>>>>>>>>>>>>>> epsilon
>>>>>>>>>>>>>>> since that could equate to the smallest measurable
>>>>>>>>>>>>>>> difference in
>>>>>>>>>>>>>>> terms of alpha or interpolation results (or 1/512 for "half
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> smallest quantum")...
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ...jim
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 10/29/15 1:36 PM, Phil Race wrote:
>>>>>>>>>>>>>>>> size->cx = (int)ceil(size->cx / scale);
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So if size->cx / scale works out to be 12.0001 you will
>>>>>>>>>>>>>>>> round
>>>>>>>>>>>>>>>> it up
>>>>>>>>>>>>>>>> to 13?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Can you check what pixel size windows gives you in such a
>>>>>>>>>>>>>>>> case ?
>>>>>>>>>>>>>>>> I'd be a little surprised if they did that rather than
>>>>>>>>>>>>>>>> round.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Is the SetFontProperty that does not accept a scale
>>>>>>>>>>>>>>>> parameter
>>>>>>>>>>>>>>>> still
>>>>>>>>>>>>>>>> used
>>>>>>>>>>>>>>>> somewhere ?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> -phil.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 10/29/2015 04:53 AM, Sergey Bylokhov wrote:
>>>>>>>>>>>>>>>>> On 17.07.15 16:27, Alexander Scherbatiy wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> - Sergey's point about multi-mon should be checked out.
>>>>>>>>>>>>>>>>>> Windows 8.1 has option "Let me choose one scaling
>>>>>>>>>>>>>>>>>> level
>>>>>>>>>>>>>>>>>> for
>>>>>>>>>>>>>>>>>> all my
>>>>>>>>>>>>>>>>>> displays".
>>>>>>>>>>>>>>>>>> If I unset it I am able to change the size of all
>>>>>>>>>>>>>>>>>> items.
>>>>>>>>>>>>>>>>>> However,
>>>>>>>>>>>>>>>>>> the DPI which returns GetDPIForMonitor is still 2 on
>>>>>>>>>>>>>>>>>> HiDPI
>>>>>>>>>>>>>>>>>> displays.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> This version looks fine, but I am sure it can be double
>>>>>>>>>>>>>>>>> checked on
>>>>>>>>>>>>>>>>> windows 10 at some moment as well.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>
--
Best regards, Sergey.
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic