Re: [10] Review request for 8155197: Focus transition issue

2017-10-06 Thread Alexey Ivanov
Hi Dmitry, Looks fine to me too. Regards, Alexey On 05/10/2017 15:27, Dmitry Markov wrote: Hi Sergey, On 4 Oct 2017, at 19:27, Sergey Bylokhov > wrote: On 10/4/17 03:25, Dmitry Markov wrote: Good catch! Actually we check restoreFocusTo before its usage, (

Re: [10] Review request for 8155197: Focus transition issue

2017-10-05 Thread Semyon Sadetsky
OK, looks good to me, thanks. --Semyon On 10/05/2017 02:15 PM, Dmitry Markov wrote: Hi Semyon, On 5 Oct 2017, at 17:13, Semyon Sadetsky > wrote: Hi Dmitriy, On 10/05/2017 07:25 AM, Dmitry Markov wrote: Hi Semyon, Actually the most recent focus owner is

Re: [10] Review request for 8155197: Focus transition issue

2017-10-05 Thread Dmitry Markov
Hi Semyon, > On 5 Oct 2017, at 17:13, Semyon Sadetsky wrote: > > Hi Dmitriy, > > On 10/05/2017 07:25 AM, Dmitry Markov wrote: >> Hi Semyon, >> >> Actually the most recent focus owner is updated during invocation of >> setEnabled(), setFocusable() and setVisible(). Please find the details belo

Re: [10] Review request for 8155197: Focus transition issue

2017-10-05 Thread Semyon Sadetsky
Hi Dmitriy, On 10/05/2017 07:25 AM, Dmitry Markov wrote: Hi Semyon, Actually the most recent focus owner is updated during invocation of setEnabled(), setFocusable() and setVisible(). Please find the details below: - Component.setEnabled(false), (i.e. disable a component) - setEnabled(false

Re: [10] Review request for 8155197: Focus transition issue

2017-10-05 Thread Dmitry Markov
Hi Sergey, > On 4 Oct 2017, at 19:27, Sergey Bylokhov wrote: > > On 10/4/17 03:25, Dmitry Markov wrote: >> Good catch! >> Actually we check restoreFocusTo before its usage, (i.e. we compare >> restoreFocusTo with most resent focus owner for the window; most recent >> focus owner is visible and

Re: [10] Review request for 8155197: Focus transition issue

2017-10-05 Thread Dmitry Markov
Hi Semyon, Actually the most recent focus owner is updated during invocation of setEnabled(), setFocusable() and setVisible(). Please find the details below: - Component.setEnabled(false), (i.e. disable a component) - setEnabled(false) -> Component.this.setEnabled(false) -> enable(false) -> dis

Re: [10] Review request for 8155197: Focus transition issue

2017-10-04 Thread Sergey Bylokhov
On 10/4/17 03:25, Dmitry Markov wrote: Good catch! Actually we check restoreFocusTo before its usage, (i.e. we compare restoreFocusTo with most resent focus owner for the window; most recent focus owner is visible and focusable, otherwise it is removed from recent focus owners map). So the che

Re: [10] Review request for 8155197: Focus transition issue

2017-10-04 Thread Semyon Sadetsky
Hi Dmitry, If you make a detailed look on the Component class methods like setEnables(), setFocusable() and setVisible() the most recent focus owner is cleared or transferred only after the component is made non-focusable. Since the fix is aimed for concurrent scenarios the recheck actually m

Re: [10] Review request for 8155197: Focus transition issue

2017-10-04 Thread Dmitry Markov
Hi Sergey, Good catch! Actually we check restoreFocusTo before its usage, (i.e. we compare restoreFocusTo with most resent focus owner for the window; most recent focus owner is visible and focusable, otherwise it is removed from recent focus owners map). So the checks before assignment are not

Re: [10] Review request for 8155197: Focus transition issue

2017-10-03 Thread Sergey Bylokhov
Hi, Dmitry. If this check is valid then probably the same check should be added to restoreFocus()? before: #173 restoreFocusTo = toFocus; But as it was mentioned before, the component can become non-visible/non-focusable at any point, for example after this check but before the assignment. An

Re: [10] Review request for 8155197: Focus transition issue

2017-10-03 Thread Semyon Sadetsky
+1 --Semyon On 10/03/2017 08:23 AM, Dmitry Markov wrote: Hi Semyon, I have updated the fix based on your suggestion. The new version is located at http://cr.openjdk.java.net/~dmarkov/8155197/webrev.02/ Also I slightly modified the

Re: [10] Review request for 8155197: Focus transition issue

2017-10-03 Thread Dmitry Markov
Hi Semyon, I have updated the fix based on your suggestion. The new version is located at http://cr.openjdk.java.net/~dmarkov/8155197/webrev.02/ Also I slightly modified the test to simplify it. Thanks, Dmitry > On 2 Oct 2017, at 18:32, S

Re: [10] Review request for 8155197: Focus transition issue

2017-10-02 Thread Alexey Ivanov
Hi Semyon and Dmitry, On 28/09/2017 18:34, Semyon Sadetsky wrote: Hi Dmitry and Alexey, On 9/28/2017 8:14 AM, Dmitry Markov wrote: I have updated the regression test for the fix. The new version is located at http://cr.openjdk.java.net/~dmarkov/8155197/webrev.01/

Re: [10] Review request for 8155197: Focus transition issue

2017-10-02 Thread Semyon Sadetsky
Hi Dmitry, Actually the parent frame doesn't own the input focus when the issue happens. The focus is on the dialog already and when FOCUS_GAINED event comes for the dialog the KFM discovers that the dialog should not have the focus and rejects it in line 588 of the DefaultKeyboardFocusManage

Re: [10] Review request for 8155197: Focus transition issue

2017-09-29 Thread Dmitry Markov
Hi Semyon, > On 28 Sep 2017, at 18:34, Semyon Sadetsky wrote: > > Hi Dmitry and Alexey, > > On 9/28/2017 8:14 AM, Dmitry Markov wrote: >> I have updated the regression test for the fix. The new version is located >> at http://cr.openjdk.java.net/~dmarkov/8155197/webrev.01/ >>

Re: [10] Review request for 8155197: Focus transition issue

2017-09-28 Thread Semyon Sadetsky
Hi Dmitry and Alexey, On 9/28/2017 8:14 AM, Dmitry Markov wrote: I have updated the regression test for the fix. The new version is located at http://cr.openjdk.java.net/~dmarkov/8155197/webrev.01/ Now the test is failed on the build wi

Re: [10] Review request for 8155197: Focus transition issue

2017-09-28 Thread Dmitry Markov
I have updated the regression test for the fix. The new version is located at http://cr.openjdk.java.net/~dmarkov/8155197/webrev.01/ Now the test is failed on the build without fix and passed on the build with the fix. Tested on Mac and Windows. Many thanks to Alexey for his help with testing an

Re: [10] Review request for 8155197: Focus transition issue

2017-09-27 Thread Semyon Sadetsky
Hi Alexey & Dmitry, The bug may be a Mac specific issue. Can you try to replace in the LWComponentPeer::requestFocus 960boolean res = parentPeer.requestWindowFocus(cause); with KeyboardFocusManagerPeer kfmPeer = LWKeyboardFocusManagerPeer.getInstance();

Re: [10] Review request for 8155197: Focus transition issue

2017-09-27 Thread Alexey Ivanov
Hi Semyon, You're right: the test does not fail for me too on Windows 10 using jdk10. At the same time, I can reproduce the problem using focus8Test.jar attached to the bug [1]. However, it's harder to reproduce the issue using jdk10 or jdk9. It takes more attempts than with jdk8. I could

Re: [10] Review request for 8155197: Focus transition issue

2017-09-26 Thread Dmitry Markov
Hi Semyon, > On 26 Sep 2017, at 23:46, Semyon Sadetsky wrote: > > Hi Dmitry, > > On 9/26/2017 1:56 AM, Dmitry Markov wrote: >> Hi Semyon, >> >> Please find my answer inline. >> >> Thanks, >> Dmitry >>> On 25 Sep 2017, at 22:26, Semyon Sadetsky >>> wrote:

Re: [10] Review request for 8155197: Focus transition issue

2017-09-26 Thread Semyon Sadetsky
Hi Dmitry, On 9/26/2017 1:56 AM, Dmitry Markov wrote: Hi Semyon, Please find my answer inline. Thanks, Dmitry On 25 Sep 2017, at 22:26, Semyon Sadetsky wrote: Hi Dmitry, On 09/25/2017 01:09 PM, Dmitry Markov wrote: Hi Semyon, This issue and the problem addressed by 8139218 and 8159432 a

Re: [10] Review request for 8155197: Focus transition issue

2017-09-26 Thread Dmitry Markov
Hi Semyon, Please find my answer inline. Thanks, Dmitry > On 25 Sep 2017, at 22:26, Semyon Sadetsky wrote: > > Hi Dmitry, > > > On 09/25/2017 01:09 PM, Dmitry Markov wrote: >> Hi Semyon, >> >> This issue and the problem addressed by 8139218 and 8159432 are slightly >> different. This one is

Re: [10] Review request for 8155197: Focus transition issue

2017-09-25 Thread Semyon Sadetsky
Hi Dmitry, On 09/25/2017 01:09 PM, Dmitry Markov wrote: Hi Semyon, This issue and the problem addressed by 8139218 and 8159432 are slightly different. This one is still reproducible on latest 9 and 10 builds using the test case attached to the bug or regression test provided with the fix. I

Re: [10] Review request for 8155197: Focus transition issue

2017-09-25 Thread Dmitry Markov
Hi Semyon, This issue and the problem addressed by 8139218 and 8159432 are slightly different. This one is still reproducible on latest 9 and 10 builds using the test case attached to the bug or regression test provided with the fix. The problem takes place when we restore focus to a component

Re: [10] Review request for 8155197: Focus transition issue

2017-09-25 Thread Sergey Bylokhov
Looks fine. On 9/23/17 08:25, Dmitry Markov wrote: Hello, Could you review a fix for jdk10, please? bug: https://bugs.openjdk.java.net/browse/JDK-8155197 webrev: http://cr.openjdk.java.net/~dmarkov/8155197/webrev.00/ Problem description: Currently we restore focus synchronously to a com

Re: [10] Review request for 8155197: Focus transition issue

2017-09-25 Thread Semyon Sadetsky
Hi Dmitry, This issue was already fixed in 9. See 8139218 & 8159432. --Semyon On 09/23/2017 08:25 AM, Dmitry Markov wrote: Hello, Could you review a fix for jdk10, please? bug: https://bugs.openjdk.java.net/browse/JDK-8155197 webrev: http://cr.openjdk.java.net/~dmarkov/8155197/webrev.

[10] Review request for 8155197: Focus transition issue

2017-09-23 Thread Dmitry Markov
Hello, Could you review a fix for jdk10, please? bug: https://bugs.openjdk.java.net/browse/JDK-8155197 webrev: http://cr.openjdk.java.net/~dmarkov/8155197/webrev.00/ Problem description: Currently we restore focus synchronously to a component if the window (which contains it) owns the focus