On 15.11.2011 15:32, Oleg Sukhodolsky wrote:
On Tue, Nov 15, 2011 at 3:23 PM, Roman Kennke<[email protected]> wrote:
Hi Anton,
On Tue, Nov 15, 2011 at 1:44 PM, Roman Kennke<[email protected]> 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.
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
