<AWT Dev> <Swing Dev> Request for review, JDK-8009883, REGRESSION: test/closed/javax/swing/AbstractButton/4246045/bug4246045.java fails
Petr Pchelko
petr.pchelko at oracle.com
Fri May 23 15:26:52 UTC 2014
Hello, Peter.
Looks good to me too.
With best regards. Petr.
On May 23, 2014, at 7:14 PM, Pete Brunet <peter.brunet at 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
>>
>
More information about the awt-dev
mailing list