<AWT Dev> Proposal for consolidation of KeyboardFocusManagerPeer
Oleg Sukhodolsky
son.two at gmail.com
Fri Nov 18 20:47:43 PST 2011
I've refreshed my memory (re-read the code :) and it looks like
NullComponentPeer is not a problem
since Component.requestFocus() always calls
ComponentPeer.requestFocus() on heavyweight.
But you still need to "reject focus requests" for peers which do not
support it (such as WFileDialogPeer).
Perhaps it is better to move even more code in Component so
peerRequestFocus() would look like:
<some calls to KFM>
requestAccepted = hw.peer.requestFocus(...);
if (!requestAccepted) {
KFM.removeLastFocusRequest();
}
return requestAccepted;
<rest of the code>
(and make sure that WFileDialog.requestFocus() and other such methods
return false ;)
Oleg.
P.S. and, perhaps, peerRequestFocus() should be part of KFM (but it is
up to you)
On Wed, Nov 16, 2011 at 3:51 PM, Oleg Sukhodolsky <son.two at gmail.com> wrote:
> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>
>>>>
>>>>
>>
>>
>
More information about the awt-dev
mailing list