Hello, Alexey. The final version still looks good.
With best regards. Petr. On 06 июня 2014 г., at 15:02, Alexey Ivanov <alexey.iva...@oracle.com> wrote: > Hi Anthony, Petr, AWT and Swing teams, > > I've addressed Anthony's and Petr's comments in the updated webrev: > http://cr.openjdk.java.net/~aivanov/8039383/jdk9/webrev.04/ > > Thank you, > Alexey. > > On 06.06.2014 12:16, 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. >>>>> >>>> >> >
