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