Thanks. The rest of the fix looks good to me.
--
best regards,
Anthony
On 6/6/2014 12:16 PM, Alexey Ivanov wrote:
Hi Anthony,
Thank you for your review.
I've removed synchronized modifier from updateProperties() method which
protected access to wprops field. Now this field is accessed from
synchronized methods lazilyInitWProps() and getWProps();
setDesktopProperty also has proper synchronization - that was my
reasoning for removing synchronized.
But you're right, it was not the right decision as the update loop now
could execute simultaneously which is undesirable. I'll put synchronized
modifier to updateProperties() method.
Regards,
Alexey.
On 06.06.2014 1:07, Anthony Petrov wrote:
Hi Alexey,
In WToolkit.java you're removing the synchronized modifier from the
private updateProperties() method. And it looks like the method itself
does allow for calling from multiple threads. So I'm concerned about
the lack of synchronization in this method. Was the removal
intentional? How is the method supposed to work now when called from
multiple threads?
--
best regards,
Anthony
On 6/5/2014 12:16 PM, Petr Pchelko wrote:
Thank you for clarifications, I've been most interested in #2 obviously.
The fix looks good to me.
With best regards. Petr.
On 05 июня 2014 г., at 11:57, Alexey Ivanov
<alexey.iva...@oracle.com> wrote:
Hello Petr,
Thank you for your comments.
1. Sure, I'll remove the comment before the change set is pushed.
2. No, I didn't try stub XPStyle object. First of all, UI delegates
decide what painting method to use depending on whether
XPStyle.getXP() returns null or not. If XPStyle.getXP() always
returns non-null value, we'll have re-implement all UI delegates for
Windows plaf, and I believe it would result in larger changeset.
Additionally, some classes fallback to inherited behavior where
XPStyle.getXP() is not available.
Another reason is that UI delegates cache Skins: those objects
shouldn't paint where theming is unavailable.
3. No, I haven't filed any bugs yet. I'll file all the issues I've
found in the nearest future.
Regards,
Alexey.
On 05.06.2014 11:08, Petr Pchelko wrote:
Hello, Alexey.
A couple of comments:
1. ThemeReader:64 - I suggest to remove that comment as it does not
add any value. The variable name is self explanatory.
2. XPStyle - did you try providing a stub XPStyle object instead of
changing many of it's methods? Do I understand correctly that this
is not possible because XPstyle is cached in many place is our code?
3. In offline discussion you've mentioned that you've found another
related issue. Did you have a chance to file a bug about it?
Thank you.
With best regards. Petr.
On 05 июня 2014 г., at 10:35, Alexey Ivanov
<alexey.iva...@oracle.com> wrote:
Hi AWT and Swing teams,
Could you please review the updated fix:
http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.03/
What has changed since version .02?
During additional testing, I found another scenario where NPE was
thrown. So the new version adds more checks to prevent access to
XPStyle and ThemeReader where Windows visual styles become
unavailable.
As in previous version, getters in XPStyle class check for null
values and return dummy defaults if ThemeReader returned null.
Skin painters also check whether theming is still available before
asking ThemeReader to paint.
Access to XPStyle.xp instance is blocked as soon as user switched
themes. The object will be cleaned when the corresponding
PropertyChangeEvent is handled by Swing.
Thank you,
Alexey.
On 06.05.2014 12:14, Alexey Ivanov wrote:
Hi AWT and Swing teams,
Could you please review the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.02/
What has changed since version .01?
The issue was still reproducible with the .01 version of the fix,
however it was harder to reproduce.
So in version .02 I added checks for null where possible. If
theme becomes unavailable, a dummy value will be used; this way
applications will be more stable. As soon as the theme change
events are handled, the entire UI will update properly.
Thank you,
Alexey.
On 24.04.2014 16:23, Alexey Ivanov wrote:
Hi Anthony, AWT and Swing teams,
Here's the updated fix:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.01/
Description:
In the new version of the fix, I use a new xpstyleEnabled field
of AtomicBoolean in WToolkit to track the current value of
"win.xpstyle.themeActive" desktop property. Its value is updated
in windowsSettingChange() and in updateProperties().
XPStyle.getXP() checks its cached value in themeActive and the
current value in WToolkit; if the value changes, it schedules
updateAllUIs and invalidates the current style right away, so
that components would not access data from the theme that became
unavailable.
Two functions in ThemeReader also check this special field from
WToolkit which prevents throwing InternalError when paint is
called before theme change is fully handled.
Before updateAllUIs is invoked, it's possible that application
will paint with some values cached from the previous theme.
Usually it happens before "Theme change" dialog disappears from
the screen, at least I noticed no UI glitches during normal
theme change.
Regression test:
No regression test is provided due to its complexity.
Additionally, whether NullPointerException or InternalError are
thrown depends on the order of event handling, sometimes
exceptions do not occur when changing theme of visual styles
enabled theme to a classic theme.
Thank you,
Alexey.
On 18.04.2014 18:52, Anthony Petrov wrote:
Hi Alexey,
No, unfortunately I don't have any suggestions right now.
As for allowing executing user code on the toolkit thread, we
can't accept such a fix. Sorry about that.
--
best regards,
Anthony
On 4/18/2014 6:48 PM, Alexey Ivanov wrote:
Hi Anthony,
Thank you for your review.
Yes, user code can install a property change listener... It
was my
concern too, that's why I explicitly noted about this.
Do you have any suggestion how this situation can be handled?
Is it a general rule that all desktop property change
listeners must be
called on EDT?
Thanks,
Alexey.
On 18.04.2014 16:02, Anthony Petrov wrote:
Hi Alexey,
With this change, property "win.xpstyle.themeActive" change
is fired
on the toolkit thread
Is it possible to install a change listener for this property
from
user code, and hence eventually allow executing some user
code on the
toolkit thread with your fix?
--
best regards,
Anthony
On 4/18/2014 12:09 PM, Alexey Ivanov wrote:
Hello Swing and AWT teams,
Could you please review the fix for jdk9:
bug: https://bugs.openjdk.java.net/browse/JDK-8039383
webrev:
http://cr.openjdk.java.net/~dmarkov/8039383/jdk9/webrev.00/
Problem description:
When changing Windows themes from a theme with visual styles
(Windows
Aero or Windows Basic) to a classic one,
NullPointerException could be
thrown from Swing code during component tree validation, or
InternalError could be thrown during component painting.
Root cause:
Windows theme data are "cached" in XPStyle object. When the
theme is
switched to a classic one, HTHEME handle becomes unavailable
and data
cannot be accessed from the theme any more. The change in
theme in
posted to EDT via invokeLater. At the same time, the UI
needs to repaint
itself as soon as Windows changed the theme, and paint code
is often
called before the theme change is handled in Java. This
leads to NPE and
InternalError as the code tries to access the data that has
become
unavailable.
The fix:
Update "win.xpstyle.themeActive" desktop property and
invalidate the
cached XPStyle as soon as windowsSettingChange() is called
from native
code. Thus when Swing code needs to access theme data, it
will see no
theme is available and will fallback to classic painting.
Note: Before the fix, PropertyChangeEvents for desktop
properties in
Windows were fired on the Event Dispatch Thread. With this
change,
property "win.xpstyle.themeActive" change is fired on the
toolkit
thread; all other properties are changed on the EDT as before.
Regards,
Alexey.
