<AWT Dev> [11] Review request for 8199748: Touch keyboard is not shown, if text component gets focus from other text component
Alexey Ivanov
alexey.ivanov at oracle.com
Tue Apr 24 12:53:53 UTC 2018
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