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

[Phil Race]

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. 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.



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
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>
>