<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
Mon Apr 23 15:20:17 UTC 2018


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
>>>>>
>>>>
>>>
>>
>



More information about the awt-dev mailing list