<AWT Dev> Proposal for consolidation of KeyboardFocusManagerPeer

[Oleg Sukhodolsky]

On Tue, Nov 15, 2011 at 4:00 PM, Anton V. Tarasov
<anton.tarasov at oracle.com> wrote:
> On 15.11.2011 15:32, Oleg Sukhodolsky wrote:
>>
>> On Tue, Nov 15, 2011 at 3:23 PM, Roman Kennke<roman at kennke.org>  wrote:
>>>
>>> Hi Anton,
>>>
>>>>> On Tue, Nov 15, 2011 at 1:44 PM, Roman Kennke<roman at kennke.org>
>>>>>  wrote:
>>>>>>
>>>>>> Find attached a patch that illustrates the idea (couldn't find a
>>>>>> recent
>>>>>> version of webrev tool online, at least not in
>>>>>> http://openjdk.java.net/guide/codeReview.html/webrevHelp.html).
>>>>>>
>>>>>> It basically moves identical/equivalent code from the peers to
>>>>>> java.awt
>>>>>> package. Notice that this is not yet complete, with this change, the
>>>>>> methods shouldNativelyFocusHeavyweight() and
>>>>>> processSynchronousLightweightTransfer() can be removed from the
>>>>>> KeyboardFocusManagerAccessor and KeyboardFocusManagerPeerImpl classes.
>>>>>>
>>>>>> Maybe I am missing something important, i.e. why this code has to be
>>>>>> in
>>>>>> the peers and needs to jump through hoops to satisfy the KFM. ?
>>>>>
>>>>> I think this is due to the fact that [WX]ComponentPeer are not the
>>>>> only implementation of ComponentPeer interface,
>>>>> we also have NullComponentPeer (or something like that) for
>>>>> lightweight components.
>>>>> and the code you are moving should be called for heavyweight components
>>>>> only.
>>>>>
>>>>> Oleg.
>>>>
>>>> Oleg,
>>>>
>>>> The requestFocus method is called on the peer of a hw container of the
>>>> component (which is either
>>>> the component itself, or the nearest hw parent, as you know).
>>>> So, I actually have no reason why Roman can't change it like the way he
>>>> did... Roman split the peer
>>>> method quite correctly, leaving the peer part to the peer.
>>>> Don't you agree?
>>>
>>> It is probably a little more subtle: The current code doesn't call back
>>> to the KFM at all for LW components, because the NullComponentPeer
>>> implements requestFocus() as no-op. With my change, we would still call
>>> the HW peer, which causes callback into the KFM. However, I am not sure
>>> this matters as the KFM seems to reject such requests. If this is an
>>> issue this could be solved by checking isLightweight() before calling
>>> into the peer.
>
> The getNativeContainer method (called in the code) does return you a
> heavyweight component. So, you don't need to double check it
> (it can't have a NullComponentPeer which implements LightweightPeer).
>
>> yep, most likely you can workaround this by this check, but I think it
>> will be more safe to perform the refactoring
>> in peer code (at least as the first step).  Unfortunately focus code
>> always was (and now is) rather fragile, so
>> less changes, less regressions.  If you will move the code to java.awt
>> you will have to spend much more time
>> with testing.
>
> I won't argue with the above concern (because the focus code is fragile
> indeed...). Though your fix looks correct.

Unfortunately I can neither support Anton nor show problems with the
fix right now.  I've no seen this code for several years,
and I need refresh my memory. Hope I will have time this weekend to
re-read the code :)

Regards, Oleg.

> So, if you decide to move the code to KFMPeerImpl you may also consider
> moving KFM.shouldNativelyFocusHeavyweight &
> KFM.processSynchronousLightweithTransfer
> methods to the same class (we no longer call them from the native code, so
> we can avoid using Accessor for them). It should look consistent.
>
> Thanks,
> Anton.
>
>>
>>> Another concern is that the requestFocus() is overridden in 1 or 2 other
>>> places like WFileDialogPeer to implement no-op. I am not sure what is
>>> the issue here or the impact of my change. Any ideas?
>>
>> file dialog on Windows is implemented using native dialog and we do
>> not support focus requests for it.
>>
>> Oleg.
>>
>>>
>>> Regards, Roman
>>>
>>>> Roman,
>>>>
>>>> Nevertheless, this was my comment (which I mentioned in the previous
>>>> post), I think the fix is fine
>>>> =) (if only Oleg doesn't provide another concern).
>>>>
>>>> Thanks,
>>>> Anton.
>>>>
>>>>
>>>>>> Regards, Roman
>>>>>>
>>>>>> Am Montag, den 14.11.2011, 22:35 +0100 schrieb Roman Kennke:
>>>>>>>
>>>>>>> Hi there,
>>>>>>>
>>>>>>> One thing that's bugging me for a while is how the ComponentPeer's
>>>>>>> requestFocus() method is supposed to work. As far as I could figure
>>>>>>> out,
>>>>>>> it's basically always like this (I use KFMHelper to call the
>>>>>>> corresponding KeyboardFocusManager's private methods by reflection):
>>>>>>>
>>>>>>>      public boolean requestFocus(Component lightweightChild, boolean
>>>>>>> temporary,
>>>>>>>              boolean focusedWindowChangeAllowed, long time, Cause
>>>>>>> cause)
>>>>>>> {
>>>>>>>          if (KFMHelper.processSynchronousLightweightTransfer(window,
>>>>>>>                  lightweightChild, temporary,
>>>>>>> focusedWindowChangeAllowed,
>>>>>>> time)) {
>>>>>>>              return true;
>>>>>>>          }
>>>>>>>
>>>>>>>          int result =
>>>>>>> KFMHelper.shouldNativelyFocusHeavyweight(window,
>>>>>>>                  lightweightChild, temporary,
>>>>>>> focusedWindowChangeAllowed,
>>>>>>> time,
>>>>>>>                  cause);
>>>>>>>
>>>>>>>          switch (result) {
>>>>>>>          case KFMHelper.SNFH_FAILURE:
>>>>>>>              return false;
>>>>>>>          case KFMHelper.SNFH_SUCCESS_PROCEED:
>>>>>>>                      requestFocusImpl(window, lightweightChild,
>>>>>>> temporary,
>>>>>>>                                   focusedWindowChangeAllowed, time,
>>>>>>> cause);
>>>>>>>          case KFMHelper.SNFH_SUCCESS_HANDLED:
>>>>>>>              // Either lightweight or excessive request - all events
>>>>>>> are
>>>>>>>              // generated.
>>>>>>>              return true;
>>>>>>>          default:
>>>>>>>              return false;
>>>>>>>          }
>>>>>>>      }
>>>>>>>
>>>>>>> The only thing that really differs between implementations would be
>>>>>>> the
>>>>>>> requestFocusImpl() method call in the SNFH_SUCCESS_PROCEED case. The
>>>>>>> rest seems to be the same in all implementations, except that in one
>>>>>>> case (Windows I believe) it is done in JNI while in others (X11) it's
>>>>>>> done by reflection.
>>>>>>>
>>>>>>> I think this can be consolidated by doing the above directly in the
>>>>>>> KeyboardFocusManager, before calling the peer requestFocus(), and
>>>>>>> have
>>>>>>> the peer's requestFocus() only do the requestFocusImpl() handling.
>>>>>>> This
>>>>>>> way we could avoid duplicate code and avoid reflection/JNI
>>>>>>> altogether.
>>>>>>>
>>>>>>> Maybe I am missing something?
>>>>>>>
>>>>>>> If not, I would work on a patch to move the above
>>>>>>> KeyboardFocusManager
>>>>>>> calls into the KFM and have the peer only bothers with the part that
>>>>>>> is
>>>>>>> requestFocusImpl() in the above example. Does that sound reasonable?
>>>>>>> It
>>>>>>> would certainly make some things simpler in OpenJDK as well as Cacio
>>>>>>> and
>>>>>>> the JavaFX SwingView that I am working on.
>>>>>>>
>>>>>>> Best regards, Roman
>>>>>>>
>>>>>>>
>>>>>>>
>>>>
>>>
>>>
>
>