Hi Alexey,
Since you insist so much on adding this space character, I am doing
this, however, I think that absence of this space character would not
influence readability, is purely a matter of personal preference and
habits of the programmer. The 3rd version of the fix, which is identical
to the 2nd version with the exception of added 4 spaces in 4 places
listed below, was created.
Webrev (the 3rd version):
http://cr.openjdk.java.net/~alitvinov/8199748/jdk11/webrev.02
Source code lines, where this space character was introduced:
- 1109 ((TextComponent) comp).isEditable()) ||
- 1111 ((JTextComponent) comp).isEditable()))) {
- 1125 MouseEvent me = (MouseEvent) e;
- 1146 FocusEvent fe = (FocusEvent) e;
Regards,
Anton
On 20/04/2018 19:16, Alexey Ivanov wrote:
Hi Anton,
I implied the space after the cast would be added in all the cases
where cast is used in this code.
I don't insist: both styles are used throughout AWT and Swing; even in
WToolkit.java both are used.
I'm not for strictly following guidelines but for consistency. The
closest functions with casts to your code are grab() and ungrab(). In
both cases, there's a space after cast. Sergey Bylokhov has added the
space as part of JDK-8074028 [1][2].
However, Code Conventions for the Java™ Programming Language [3]
recommends using space after cast.
Regards,
Alexey
[1] https://bugs.openjdk.java.net/browse/JDK-8074028
[2] http://hg.openjdk.java.net/jdk9/client/jdk/rev/39bd7fa12bc3#l83.35
[3]
http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-141388.html#682
On 20/04/2018 14:30, Anton Litvinov wrote:
Hello Alexey,
Thank you for approval of the 2nd version of the fix. I understand
your recommendation to add space character in class cast expressions,
for example to make the expression
1109 ((TextComponent)comp).isEditable()) ||
look as
1109 ((TextComponent) comp).isEditable()) ||
But in case of this fix it will be necessary to apply this change to
other code related to the fix, like:
1125 MouseEvent me = (MouseEvent)e;
1146 FocusEvent fe = (FocusEvent)e;
In fact, I have never used the style of formatting suggested by you
in my previous fixes before, and nobody before recommended me to
follow this style with extra space in class cast expressions. Also I
did not see this style was widely used in AWT code, except for the
class "WToolkit", where I see many examples of it for the first time.
Personally I do not like this extra space, but, if you insist and
just for the purpose of making formatting style of this fix comply
with other code only in "WTookit.java" file, I could add these extra
spaces before pushing the fix, if the fix is approved by Sergey
Bylokhov.
Thank you,
Anton
On 20/04/2018 10:46, Alexey Ivanov wrote:
Hi Anton,
The fix looks good.
Could you please add space after the cast operator before pushing?
No additional review is required for this small code style change, I
believe.
Regards,
Alexey
On 19/04/2018 12:12, Anton Litvinov wrote:
Hello Alexey,
Thank you for review of this fix and for identification of this
issue. Yes, I agree that the touch keyboard should not be hidden,
when a focus is moved from one text component to the second, after
the TAB key was pressed on the touch keyboard. Hiding it in this
scenario, while not hiding the touch keyboard after touching the
area of the second text component, is inconsistent.
The new version of the fix addressing your remark was created.
Webrev (the 2nd version):
http://cr.openjdk.java.net/~alitvinov/8199748/jdk11/webrev.01
This issue and the original bug is resolved in the 2nd version of
the fix by not calling "WToolkit.hideTouchKeyboard()" on
"FocusEvent.FOCUS_LOST" event, if a component which gets focus is a
text component. Also this version of the fix contains resolution of
a possible memory leak from the variables: "compOnTouchDownEvent",
"compOnMousePressedEvent" by changing their types to
"WeakReference<Component>" and contains small refactoring of "if"
expressions in "WToolkit.showOrHideTouchKeyboard(Component,
AWTEvent)" method. Could you please look at the 2nd version of the
fix.
Thank you,
Anton
On 17/04/2018 17:24, Alexey Ivanov wrote:
Hi Anton,
Shouldn't touch keyboard remain visible if Tab key on the touch
keyboard itself is pressed? Provided the next focusable component
is a text component, of course.
Consider the following scenario: start Notepad and open Replace
dialog. Touch "Find what" field: the touch keyboard appears. Press
Tab on the touch keyboard: the focus moves to "Replace with" field
and touch keyboard stays visible.
Regards,
Alexey
On 16/04/2018 13:03, Anton Litvinov wrote:
Hello,
Could you please review the following fix for the bug.
Bug: https://bugs.openjdk.java.net/browse/JDK-8199748
Webrev:
http://cr.openjdk.java.net/~alitvinov/8199748/jdk11/webrev.00
In the fix for JDK-8166772 it was deliberately implemented that
the touch keyboard is shown only on "MouseEvent.MOUSE_RELEASED"
event and is hidden on "FocusEvent.FOCUS_LOST" event.
The reason of the bug is the fact that, when the touch keyboard
is already shown for one text component and a user touches
another text component, then the following 2 events occur in the
presented order:
1. "MouseEvent.MOUSE_RELEASED" event arrives. The touch keyboard
is shown for the new text component.
2. "FocusEvent.FOCUS_LOST" event arrives for the previous text
component. The touch keyboard shown for the new text component
becomes hidden.
The fix allows not to hide the touch keyboard during processing
of "FocusEvent.FOCUS_LOST" event, if the touch keyboard has just
been shown, as a result of processing of
"MouseEvent.MOUSE_RELEASED" event, for the component which gets
focus "FocusEvent.getOppositeComponent()".
Thank you,
Anton
