<AWT Dev> [11] Review request for 8199748: Touch keyboard is not shown, if text component gets focus from other text component
Anton Litvinov
anton.litvinov at oracle.com
Tue Apr 24 13:52:17 UTC 2018
Hi Alexey,
Good, thank you, I understood. I will push the 3rd version of the fix
with spaces.
Thank you,
Anton
On 24/04/2018 13:53, Alexey Ivanov wrote:
> Hi Anton,
>
> On 23/04/2018 16:20, Anton Litvinov wrote:
>> Hi Alexey,
>>
>> Since you insist so much on adding this space character,
>
> No, I don't insist. I'm fine with either style.
> It's up to you to choose the style: either with space or without it.
>
>
> Regards,
> Alexey
>
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the awt-dev
mailing list