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