<Swing Dev> [11] Review Request: 8201552 Ellipsis in "Classical" label in SwingSet2 demo with Windows L&F at Hidpi

Phil Race philip.race at oracle.com
Thu Jun 28 22:34:55 UTC 2018

On 6/28/2018 3:14 PM, Sergey Bylokhov wrote:
> On 28/06/2018 14:20, Phil Race wrote:
>> 2336 var newGC = (GraphicsConfiguration) oldValue; 2337 var oldGC = 
>> (GraphicsConfiguration) newValue; 2338 var newTx = newGC != null ? 
>> newGC.getDefaultTransform() : null;
>> 2339 var oldTx = oldGC != null ? oldGC.getDefaultTransform() : null;
>> 2340 return !Objects.equals(newTx, oldTx); var ?? I suppose it I can 
>> infer what the type of newTx and oldTx is because I know the API, but 
>> for someone else this makes it less readable.
> The type of "newGC/oldGC" is on the right where we apply the cast:
> var newGC = (GraphicsConfiguration) oldValue;

and I wasn't questioning that one for that reason ..
> The type of newTx/oldTx is not so important since we only check they 
> are equivalent оr not. It will works even if the type will be Object 
> instead of var.
That's the one I am questioning. I can't tell by reading the code what 
type is, so
I think this is a borderline use of var.

>> I suppose your comments below are discussing the effect of line 2340 
>> We generally consider a null TX to mean identity and so consider them 
>> equal but this would do the opposite .. and you say this is what 
>> Swing wants ? -phil.
> In theory Swing wants to consider null as a identity TX, but in 
> practice if we skip such validation we will expose other bugs like 
> JDK-8206024. The change of "GC=null" to "GC=non-null" is occurred when 
> the component id added to the parent and is used as a last chance to 
> update the state of the component before show it to the user.
> For example:
> 1 Jtree tree;
> 2 Dimension before = component.getPreferredSize();
> 3 tree.setFont(new Font());
> .... some other initialization....
> 4 Dimension after = component.getPreferredSize();
> 5 frame.add(tree);
> In line 3 the components should be revalidated because the font which 
> affects the size was changed. But this validation can be skipped 
> because of the some bugs. And it is not visible to the user just 
> because we do validation at step 5(when the GC=null will be replaced 
> by the GC=non-null). Before my fix it it was done using "ancestor" 
> property(in some cases)

You are confirming what I thought you were saying.
Perhaps all such bugs could be fixed, and then we could fix the logic 
but that is
something that is beyond the scope of this fix.



>> On 6/28/2018 1:54 PM, Sergey Bylokhov wrote:
>>> Hi, Phil.
>>> The new webrev:
>>> http://cr.openjdk.java.net/~serb/8201552/webrev.03/
>>> I have checked why validation of the component is necessary, when 
>>> the "ancestor" is changed (I removed it, but the new validation on 
>>> GC change null->non-null is equivalent).
>>> Such validation is used as a last chance to update the state of the 
>>> component before show it to the user. In theory it is not necessary 
>>> but some components skips validation on font change(like jtree) and 
>>> maybe other properties. I have files a separate bug for that: 
>>> https://bugs.openjdk.java.net/browse/JDK-8206024
>>>  - In the fix I have moved these checks to SwingU2.
>>>  - TODO block for JSpinner: is removed because this bug was fixed
>>> https://bugs.openjdk.java.net/browse/JDK-8205144
>>> On 22/06/2018 16:54, Philip Race wrote:
>>>> It is not really the GC .. there is no GC .. but because of that the
>>>> code uses a default FontRenderContext.
>>>> I think Swing is conflating GC + FRC but given that, then the logic
>>>> to recalculate based on graphicsconfiguration should be fine, although
>>>> maybe you can check if we really need to trigger it ..
>>>> if the screen we are shown on has no scale (ie uiScale == 1) then
>>>> we may be triggering a pointless invalidation with consequent 
>>>> overhead.
>>>> Can  you check if we can skip it in some cases ?
>>>> -phil.
>>>> On 6/22/18, 4:16 PM, Sergey Bylokhov wrote:
>>>>> The place where the current graphicsConfiguration of the component 
>>>>> is matter:
>>>>>  - JComponent.getFontMetrics(Font)
>>>>>  -- SwingUtilities2.getFontMetrics(this, font)
>>>>>  --- SwingUtilities2.getFRCProperty(JComponent c)
>>>>>     if (c != null) {
>>>>>         GraphicsConfiguration gc = c.getGraphicsConfiguration();
>>>>>         AffineTransform tx = (gc == null) ? null : 
>>>>> gc.getDefaultTransform();
>>>>>         Object aaHint = c.getClientProperty(KEY_TEXT_ANTIALIASING);
>>>>>         return getFRCFromCache(tx, aaHint);
>>>>>     }
>>>>> When the bug is reproduced we calculated the size of the tree 
>>>>> based on one GC and draw it using another GC. The first GC which 
>>>>> is used for size calculation may be null or it maybe a non-null 
>>>>> value pointed to another screen, for example if the jtree was 
>>>>> shown on one screen and then dragged to another.
>>>>> On 22/06/2018 14:40, Sergey Bylokhov wrote:
>>>>>> In the SwingSet2 the bug is visible only if the user switches the 
>>>>>> L&F immediately before switching to the tree demo. And is not 
>>>>>> reproduced if the user switches to the tree demo and then change 
>>>>>> the L&F.
>>>>>>   - If the tree is not visible then it will calculate the size of 
>>>>>> the text based on some default GC, and will not update this size 
>>>>>> when it will be added to the frame(which has another actual GC). 
>>>>>> So the tree will use the size calculated using one GC, and will 
>>>>>> draw the text using another GC. If the user will switch L&F the 
>>>>>> size will be recalculated using correct GraphicsConfiguration.
>>>>>>> -phil.
>>>>>>> On 06/22/2018 01:48 PM, Sergey Bylokhov wrote:
>>>>>>>> Any volunteers for review?
>>>>>>>> =)
>>>>>>>> On 17/06/2018 15:37, Sergey Bylokhov wrote:
>>>>>>>>> Unfortunately after additional testing I found a bug in our 
>>>>>>>>> text related components. In the JTextPane the text looks 
>>>>>>>>> broken if we request some change in the component after it is 
>>>>>>>>> became visible.
>>>>>>>>> For example if we change the font then the text will be 
>>>>>>>>> overlapping. So if I will be applied this fix, which will 
>>>>>>>>> force text component to relayout(because of the change in 
>>>>>>>>> graphics config), then the text will be broken from the 
>>>>>>>>> beginning.
>>>>>>>>> But before the fix it will be broken only if the application 
>>>>>>>>> will change the pane after it became visible(BTW text 
>>>>>>>>> rendering during editing is broken in both cases).
>>>>>>>>> So I temporary reverted the changes in the text related 
>>>>>>>>> components:
>>>>>>>>> http://cr.openjdk.java.net/~serb/8201552/webrev.02
>>>>>>>>> Two follow bugs were created:
>>>>>>>>>   - Text components: 
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8205143
>>>>>>>>>   - JSpinner: https://bugs.openjdk.java.net/browse/JDK-8205144
>>>>>>>>> On 15/06/2018 23:31, Sergey Bylokhov wrote:
>>>>>>>>>> Hello.
>>>>>>>>>> Please review the fix for jdk11.
>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8201552
>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~serb/8201552/webrev.01/
>>>>>>>>>> Short description:
>>>>>>>>>> This fix enhance implementation of JDK-8178025[1] for most of 
>>>>>>>>>> our Swing components. The main rule which I tried to follow:
>>>>>>>>>> "If layout of the component depends from the font then it 
>>>>>>>>>> should depend on the current graphics configuration as well, 
>>>>>>>>>> because FontMetrics depends on graphics configuration".
>>>>>>>>>> Long description:
>>>>>>>>>> The fix for JDK-8178025 added a special property 
>>>>>>>>>> "graphicsConfiguration" which: fired when the 
>>>>>>>>>> graphicsConfiguration is changed from one non-null value to 
>>>>>>>>>> another non-null value.
>>>>>>>>>> Those fix also updated some of the components(to 
>>>>>>>>>> refresh/re-validate its states when the 
>>>>>>>>>> "graphicsConfiguration" or "ancestor" were changed).
>>>>>>>>>> The usage of "ancestor" was not obvious, so I modify the code 
>>>>>>>>>> to fire "graphicsConfiguration" every time, this cover a 
>>>>>>>>>> situation when the "GC=null" is changed to 
>>>>>>>>>> "GC=non-null"(previously it was covered by "ancestor" 
>>>>>>>>>> property). So after this fix our components will listen only 
>>>>>>>>>> "font" and "graphicsConfiguration".
>>>>>>>>>> In implementation of JDK-8178025 the "graphicsConfiguration" 
>>>>>>>>>> is fired immediately after GC is changed, it caused the 
>>>>>>>>>> issues in some containers like JTree. When the container get 
>>>>>>>>>> such notification it usually tries to get some information 
>>>>>>>>>> from children, but in this moment children had previous 
>>>>>>>>>> graphic config, so the result calculated(and usually cached) 
>>>>>>>>>> in the container was wrong. In this fix I changed 
>>>>>>>>>> implementation of this property. Now it will fired only when 
>>>>>>>>>> the container and all its children are updated.
>>>>>>>>>> ===
>>>>>>>>>> Note that the new test StalePreferredSize.java has a TODO 
>>>>>>>>>> block. Because JSpinner does not work properly even after the 
>>>>>>>>>> current fix. The reason is that during validation it is 
>>>>>>>>>> unexpectedly change the font from normal to bold(I'll fix 
>>>>>>>>>> this in a separate bug)
>>>>>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8178025

More information about the swing-dev mailing list