Hello, Peter. Looks good to me too.
With best regards. Petr. On May 23, 2014, at 7:14 PM, Pete Brunet <peter.bru...@oracle.com> wrote: > Thanks Alexandr. > > So far Alexandr is the only reviewer of this. I'd like one more. > > Thanks, Pete > > On 5/23/14 5:34 AM, Alexander Scherbatiy wrote: >> On 5/22/2014 9:59 PM, Pete Brunet wrote: >>> I'd like one more reviewer of this fix. >>> >>> Also I removed the @Deprecated and will deal with this in a following >>> JBS issue. >>> >>> http://cr.openjdk.java.net/~ptbrunet/JDK-8009883/webrev.04/ >> >> The fix looks good for me. >> >> Thanks, >> Alexandr. >> >>> >>> Pete >>> >>> On 5/21/14 10:08 AM, Pete Brunet wrote: >>>> On 5/21/14 7:04 AM, Alexander Scherbatiy wrote: >>>>> On 5/21/2014 2:56 PM, Alexander Scherbatiy wrote: >>>>>> On 5/20/2014 1:10 AM, Pete Brunet wrote: >>>>>>> On 5/16/14 11:04 AM, Pete Brunet wrote: >>>>>>>> On 5/16/14 10:45 AM, Alexander Scherbatiy wrote: >>>>>>>>> On 5/16/2014 7:15 PM, Pete Brunet wrote: >>>>>>>>>> On 5/16/14 6:45 AM, Alexander Scherbatiy wrote: >>>>>>>>>>> Hi Peter, >>>>>>>>>>> >>>>>>>>>>> Is there any difference between >>>>>>>>>>> AccessibleAWTFocusHandler and >>>>>>>>>>> AccessibleFocusHandler classes? >>>>>>>>>> Hi Alexandr, The former is the focus handler used by the inner >>>>>>>>>> class >>>>>>>>>> accessibility support for an AWT Component and the latter is the >>>>>>>>>> focus >>>>>>>>>> handler used by the inner class accessibility support for a >>>>>>>>>> JComponent. >>>>>>>>>> >>>>>>>>>> The code is the same. >>>>>>>>> Is it possible to remove the AccessibleFocusHandler class >>>>>>>>> at all >>>>>>>>> and initialize the accessibleAWTFocusHandler >>>>>>>>> only in the >>>>>>>>> AccessibleAWTFocusHandler.addPropertyChangeListener() >>>>>>>>> method? >>>>>>>> This looks like it will work and I would like that solution a lot >>>>>>>> better. I'll give it a try and if that works out I'll provide a >>>>>>>> new >>>>>>>> webrev. Thanks Alexandr. >>>>>>> Hi Alexandr and AWT/Swing teams, >>>>>>> >>>>>>> Please review the new webrev at >>>>>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8009883/webrev.01/ >>>>>> The fix looks good for me. >>>>> Sorry. I just realize that the AccessibleFocusHandler class has >>>>> protected access and can't be removed because of the compatibility >>>>> reasons. >>>>> http://docs.oracle.com/javase/7/docs/api/javax/swing/JComponent.AccessibleJComponent.AccessibleFocusHandler.html >>>>> >>>>> >>>>> >>>>> All others looks good for me. Just leave the >>>>> AccessibleFocusHandler class from AccessibleJComponent as is. >>>> Thanks very much for catching that Alexandr! The new webrev is here: >>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8009883/webrev.03/ >>>> >>>> Note that I added a @Deprecated tag (and javadoc tag). Hopefully >>>> that's >>>> the correct thing to do. >>>> >>>> I'd like to list one more reviewer on the patch so I'd appreciate one >>>> more set of eyes on this. >>>> >>>> Pete >>>>> Thanks, >>>>> Alexandr. >>>>> >>>>>> Thanks, >>>>>> Alexandr. >>>>>> >>>>>>> Pete >>>>>>>>> Thanks, >>>>>>>>> Alexandr. >>>>>>>>> >>>>>>>>>> The former code is this: >>>>>>>>>> >>>>>>>>>> protected class AccessibleAWTFocusHandler implements >>>>>>>>>> FocusListener { >>>>>>>>>> public void focusGained(FocusEvent event) { >>>>>>>>>> if (accessibleContext != null) { >>>>>>>>>> accessibleContext.firePropertyChange( >>>>>>>>>> AccessibleContext.ACCESSIBLE_STATE_PROPERTY, >>>>>>>>>> null, >>>>>>>>>> AccessibleState.FOCUSED); >>>>>>>>>> } >>>>>>>>>> } >>>>>>>>>> public void focusLost(FocusEvent event) { >>>>>>>>>> if (accessibleContext != null) { >>>>>>>>>> accessibleContext.firePropertyChange( >>>>>>>>>> AccessibleContext.ACCESSIBLE_STATE_PROPERTY, >>>>>>>>>> AccessibleState.FOCUSED, null); >>>>>>>>>> } >>>>>>>>>> } >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> and the latter code is this >>>>>>>>>> >>>>>>>>>> protected class AccessibleFocusHandler implements >>>>>>>>>> FocusListener { >>>>>>>>>> public void focusGained(FocusEvent event) { >>>>>>>>>> if (accessibleContext != null) { >>>>>>>>>> accessibleContext.firePropertyChange( >>>>>>>>>> AccessibleContext.ACCESSIBLE_STATE_PROPERTY, >>>>>>>>>> null, AccessibleState.FOCUSED); >>>>>>>>>> } >>>>>>>>>> } >>>>>>>>>> public void focusLost(FocusEvent event) { >>>>>>>>>> if (accessibleContext != null) { >>>>>>>>>> accessibleContext.firePropertyChange( >>>>>>>>>> AccessibleContext.ACCESSIBLE_STATE_PROPERTY, >>>>>>>>>> AccessibleState.FOCUSED, null); >>>>>>>>>> } >>>>>>>>>> } >>>>>>>>>> } >>>>>>>>>>> Thanks, >>>>>>>>>>> Alexandr. >>>>>>>>>>> >>>>>>>>>>> On 5/7/2014 5:45 AM, Pete Brunet wrote: >>>>>>>>>>>> Hi Swing and AWT teams, (and Artem since you were involved >>>>>>>>>>>> in the >>>>>>>>>>>> 7179482 fix), >>>>>>>>>>>> >>>>>>>>>>>> I'd appreciate your review for a fix to a regression in >>>>>>>>>>>> java.awt.Component and javax.swing.JComponent. >>>>>>>>>>>> >>>>>>>>>>>> Original bug: https://bugs.openjdk.java.net/browse/JDK-7179482 >>>>>>>>>>>> Orignal CCC: 7179482 >>>>>>>>>>>> Regression Bug: >>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8009883 >>>>>>>>>>>> Regression Fix Webrev: >>>>>>>>>>>> http://cr.openjdk.java.net/~ptbrunet/JDK-8009883/webrev.00/ >>>>>>>>>>>> >>>>>>>>>>>> Problem Description: >>>>>>>>>>>> jdk/test/closed/javax/swing/AbstractButton/4246045/bug4246045.java >>>>>>>>>>>> >>>>>>>>>>>> tests >>>>>>>>>>>> to make sure the focus state changes whenever a JButton or >>>>>>>>>>>> JToggleButton >>>>>>>>>>>> property change listener is called. The problem in this case >>>>>>>>>>>> is that >>>>>>>>>>>> for each change of focus the listener is called twice. The >>>>>>>>>>>> first time >>>>>>>>>>>> there is no problem because for example the component is not >>>>>>>>>>>> focused and >>>>>>>>>>>> then it is, but the second time the listener is called the >>>>>>>>>>>> component is >>>>>>>>>>>> already focused and the new state is also focused so the >>>>>>>>>>>> test case >>>>>>>>>>>> detects that the focus state didn't change. >>>>>>>>>>>> >>>>>>>>>>>> Root cause: >>>>>>>>>>>> There are two focus gained and two focus lost events fired when >>>>>>>>>>>> JComponents gain/loose focus. Part of the fix for JDK-7179482 >>>>>>>>>>>> was to >>>>>>>>>>>> deprecate the >>>>>>>>>>>> JComponent.AccessibleJComponent.accessibleFocusHandler >>>>>>>>>>>> field noting that >>>>>>>>>>>> Component.AccessibleAWTComponent.accessibleAWTFocusHandler >>>>>>>>>>>> should be >>>>>>>>>>>> used instead. The problem is that this was not done when >>>>>>>>>>>> 7179482 was >>>>>>>>>>>> fixed. The end result is that two focus listeners are added >>>>>>>>>>>> when >>>>>>>>>>>> JComponent.AccessibleJComponent.addPropertyChangeListener is >>>>>>>>>>>> called. >>>>>>>>>>>> The first listener is added in that property change method >>>>>>>>>>>> and the >>>>>>>>>>>> second one is added when the superclass method is called, i.e. >>>>>>>>>>>> Component.AccessibleAWTComponent.addPropertyChangeListener. The >>>>>>>>>>>> two >>>>>>>>>>>> listeners cause two focus events with the second one not >>>>>>>>>>>> causing a >>>>>>>>>>>> state >>>>>>>>>>>> change. >>>>>>>>>>>> >>>>>>>>>>>> Proposed fix: >>>>>>>>>>>> The proposed fix is for >>>>>>>>>>>> JComponent.AccessibleJComponent.addPropertyChangeListener to >>>>>>>>>>>> set its >>>>>>>>>>>> listener into >>>>>>>>>>>> Component.AccessibleAWTComponent.accessibleAWTFocusHandler >>>>>>>>>>>> so when the superclass method, >>>>>>>>>>>> Component.AccessibleAWTComponent.addPropertyChangeListener is >>>>>>>>>>>> called the >>>>>>>>>>>> listener at that level will already be set. >>>>>>>>>>>> >>>>>>>>>>>> A side effect of the fix is that logic was needed to be >>>>>>>>>>>> added in >>>>>>>>>>>> Component.AccessibleAWTComponent.addPropertyChangeListener >>>>>>>>>>>> to deal >>>>>>>>>>>> with >>>>>>>>>>>> the fact that the >>>>>>>>>>>> Component.AccessibleAWTComponent.accessibleAWTFocusHandler >>>>>>>>>>>> field >>>>>>>>>>>> can be >>>>>>>>>>>> set either from within the object's subclass method >>>>>>>>>>>> JComponent.AccessibleJComponent.addPropertyChangeListener or >>>>>>>>>>>> via an >>>>>>>>>>>> external call to >>>>>>>>>>>> Component.AccessAWTComponent.addPropertyChangeListener >>>>>>>>>>>> for cases other than when there is a JComponent subclass of >>>>>>>>>>>> Component. >>>>>>>>>>>> In the former case the listener is already set by the subclass >>>>>>>>>>>> so a >>>>>>>>>>>> focus listener isn't added. In the latter case the listener is >>>>>>>>>>>> not >>>>>>>>>>>> yet >>>>>>>>>>>> added so a focus listener is new'd and added. >>>>>>>>>>>> >>>>>>>>>>>> There are similar considerations when removing a listener; this >>>>>>>>>>>> can be >>>>>>>>>>>> seen in the webrev. >>>>>>>>>>>> >>>>>>>>>>>> There may be a better way to fix this but the proposed fix does >>>>>>>>>>>> resolve >>>>>>>>>>>> the issue that the test case exposed. >>>>>>>>>>>> >>>>>>>>>>>> Best regards, >>>>>>>>>>>> Pete >> >
