<AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Manajit Halder
Hi All,

Kindly review the fix for JDK11.

Bug: 

Webrev:

Fix:
Frame is focusable:
Retaining the existing frame resizable behaviour (Fixes the current issue).
Frame is non-focusable:
Making the Frame non-resizable (Fix for issue 7158623).

Regards,
Manajit
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Phil Race
I would like to refer back to a comment you made in the previous fix
http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html

> It is not mentioned in the focus spec whether the unfocusable maximized frames should be resizable or not.

Yet there seems to be a JCK test that says it should not be resizable ?

Can you review the spec. again ?
JCK must have based the test on something .. else the test is not valid.

If we want that behaviour specified .. we should be specifying it ..
But I am not sure if it is actually enforceable on all window managers / desktops.

But I have the same issue with this fix as the previous one. Perhaps not the fix,
but the explanation. The code being changed can't be understood without seeing
the method it calls, and the native method it in turn calls.

Can you provide a detailed explanation as to how this change propagates down
and does the right thing ?

BTW stylistically - if this is the right fix - you could do :

setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable) ? RESIZABLE : 0), isFocusable);

-phil.

On 06/14/2018 11:37 PM, Manajit Halder wrote:
Hi All,

Kindly review the fix for JDK11.

Bug: 

Webrev:

Fix:
Frame is focusable:
Retaining the existing frame resizable behaviour (Fixes the current issue).
Frame is non-focusable:
Making the Frame non-resizable (Fix for issue 7158623).

Regards,
Manajit

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Sergey Bylokhov
I still do not understand why the resizable property depends from the
focusable property? I guess it should depend only on Frame.isResizable().

On 15/06/2018 10:54, Phil Race wrote:

> I would like to refer back to a comment you made in the previous fix
> http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html
>
>  > It is not mentioned in the focus spec whether the unfocusable
> maximized frames should be resizable or not.
>
> Yet there seems to be a JCK test that says it should not be resizable ?
>
> Can you review the spec. again ?
> JCK must have based the test on something .. else the test is not valid.
>
> If we want that behaviour specified .. we should be specifying it ..
> But I am not sure if it is actually enforceable on all window managers /
> desktops.
>
> But I have the same issue with this fix as the previous one. Perhaps not
> the fix,
> but the explanation. The code being changed can't be understood without
> seeing
> the method it calls, and the native method it in turn calls.
>
> Can you provide a detailed explanation as to how this change propagates down
> and does the right thing ?
>
> BTW stylistically - if this is the right fix - you could do :
>
> setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable) ?
> RESIZABLE : 0), isFocusable);
>
>
> -phil.
>
> On 06/14/2018 11:37 PM, Manajit Halder wrote:
>> Hi All,
>>
>> Kindly review the fix for JDK11.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8204860
>>
>> Webrev:
>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ 
>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>
>>
>> Fix:
>> Frame is focusable:
>> Retaining the existing frame resizable behaviour (Fixes the current
>> issue).
>> Frame is non-focusable:
>> Making the Frame non-resizable (Fix for issue 7158623).
>>
>> Regards,
>> Manajit
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Manajit Halder
In reply to this post by Phil Race
Hi Phil,

Please find my answer inline to your comment.

On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email]> wrote:

I would like to refer back to a comment you made in the previous fix
http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html

> It is not mentioned in the focus spec whether the unfocusable maximized frames should be resizable or not.

Yet there seems to be a JCK test that says it should not be resizable ?

Can you review the spec. again ?
JCK must have based the test on something .. else the test is not valid.

Yes, I checked FocusSpec.html and it doesn’t specify anything about resizable behaviour of non-focusable Frame.

The UnfocusableMaximizedFrameResizablity.java  test passes on Window and Linux and fails on Mac OS. 
Fix for issue 7158623 was done accordingly to make sure the behaviour is same on all platforms. 

If this behaviour is not correct then Window and Linux code should be changed accordingly so that all three platforms behave same.


If we want that behaviour specified .. we should be specifying it ..
But I am not sure if it is actually enforceable on all window managers / desktops.

But I have the same issue with this fix as the previous one. Perhaps not the fix,
but the explanation. The code being changed can't be understood without seeing
the method it calls, and the native method it in turn calls.

Can you provide a detailed explanation as to how this change propagates down
and does the right thing ?

The call flow:

updateFocusableWindowState() calls setStyleBits with style bits to be set on the window.
setStyleBits() calls native method nativeSetNSWindowStyleBits. nativeSetNSWindowStyleBits passes mask and 0 as the second parameter in our case (for non-focusable windows).
Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits in AWTWindow.m generates newBits and applies it on the NSWindow.

My previous fix for issue 7158623 explains bits set on the window.


BTW stylistically - if this is the right fix - you could do :

setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable) ? RESIZABLE : 0), isFocusable);
Changed the code as per the suggestion. Please review the modified code.

Regards,
Manajit

-phil.

On 06/14/2018 11:37 PM, Manajit Halder wrote:
Hi All,

Kindly review the fix for JDK11.

Bug: 

Webrev:

Fix:
Frame is focusable:
Retaining the existing frame resizable behaviour (Fixes the current issue).
Frame is non-focusable:
Making the Frame non-resizable (Fix for issue 7158623).

Regards,
Manajit


Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Manajit Halder
Hi Phil and Sergey,

I have changed the code as per your suggestions. Now window is resized based on the following condition:

If window is non-focusable OR window is focusable but non-resizable THEN window is made non-resizable.
Otherwise window is made resizable (when window is resizable and focusable).

Please review the changes:

Note: Fix was verified by running all the java/awt/ jtreg test cases and by executing the reported JCK interactive test case.

Regards,
Manajit

On 20-Jun-2018, at 6:27 PM, Manajit Halder <[hidden email]> wrote:

Hi Phil,

Please find my answer inline to your comment.

On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email]> wrote:

I would like to refer back to a comment you made in the previous fix
http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html

> It is not mentioned in the focus spec whether the unfocusable maximized frames should be resizable or not.

Yet there seems to be a JCK test that says it should not be resizable ?

Can you review the spec. again ?
JCK must have based the test on something .. else the test is not valid.

Yes, I checked FocusSpec.html and it doesn’t specify anything about resizable behaviour of non-focusable Frame.

The UnfocusableMaximizedFrameResizablity.java  test passes on Window and Linux and fails on Mac OS. 
Fix for issue 7158623 was done accordingly to make sure the behaviour is same on all platforms. 

If this behaviour is not correct then Window and Linux code should be changed accordingly so that all three platforms behave same.


If we want that behaviour specified .. we should be specifying it ..
But I am not sure if it is actually enforceable on all window managers / desktops.

But I have the same issue with this fix as the previous one. Perhaps not the fix,
but the explanation. The code being changed can't be understood without seeing
the method it calls, and the native method it in turn calls.

Can you provide a detailed explanation as to how this change propagates down
and does the right thing ?

The call flow:

updateFocusableWindowState() calls setStyleBits with style bits to be set on the window.
setStyleBits() calls native method nativeSetNSWindowStyleBits. nativeSetNSWindowStyleBits passes mask and 0 as the second parameter in our case (for non-focusable windows).
Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits in AWTWindow.m generates newBits and applies it on the NSWindow.

My previous fix for issue 7158623 explains bits set on the window.


BTW stylistically - if this is the right fix - you could do :

setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable) ? RESIZABLE : 0), isFocusable);
Changed the code as per the suggestion. Please review the modified code.

Regards,
Manajit

-phil.

On 06/14/2018 11:37 PM, Manajit Halder wrote:
Hi All,

Kindly review the fix for JDK11.

Bug: 

Webrev:

Fix:
Frame is focusable:
Retaining the existing frame resizable behaviour (Fixes the current issue).
Frame is non-focusable:
Making the Frame non-resizable (Fix for issue 7158623).

Regards,
Manajit



Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Sergey Bylokhov
Hi, Manajit.
There is one more inconsistency, I have tested it using the test for teh
previous fix: UnfocusableMaximizedFrameResizablity.java

In this case the window is zoomable(the green button is active, but does
not work as expected):
         frame.setFocusableWindowState(false);
         frame.setResizable(true);

In this case the window is not zoomable(the green button is inactive):
         frame.setResizable(true);
         frame.setFocusableWindowState(false);

I suggest to update the testcase to cover all this cases which we found
during review.

On 21/06/2018 12:26, Manajit Halder wrote:

> Hi Phil and Sergey,
>
> I have changed the code as per your suggestions. Now window is resized
> based on the following condition:
>
> If window is non-focusable OR window is focusable but non-resizable THEN
> window is made non-resizable.
> Otherwise window is made resizable (when window is resizable and focusable).
>
> Please review the changes:
> http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/
>
> Note: Fix was verified by running all the java/awt/ jtreg test cases and
> by executing the reported JCK interactive test case.
>
> Regards,
> Manajit
>
>> On 20-Jun-2018, at 6:27 PM, Manajit Halder <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> Hi Phil,
>>
>> Please find my answer inline to your comment.
>>
>>> On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>
>>> I would like to refer back to a comment you made in the previous fix
>>> http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html
>>>
>>> > It is not mentioned in the focus spec whether the unfocusable
>>> maximized frames should be resizable or not.
>>>
>>> Yet there seems to be a JCK test that says it should not be resizable ?
>>>
>>> Can you review the spec. again ?
>>> JCK must have based the test on something .. else the test is not valid.
>>
>> Yes, I checked FocusSpec.html and it doesn’t specify anything about
>> resizable behaviour of non-focusable Frame.
>>
>> The UnfocusableMaximizedFrameResizablity.java  test passes on Window
>> and Linux and fails on Mac OS.
>> Fix for issue 7158623 was done accordingly to make sure the behaviour
>> is same on all platforms.
>>
>> If this behaviour is not correct then Window and Linux code should be
>> changed accordingly so that all three platforms behave same.
>>
>>>
>>> If we want that behaviour specified .. we should be specifying it ..
>>> But I am not sure if it is actually enforceable on all window
>>> managers / desktops.
>>>
>>> But I have the same issue with this fix as the previous one. Perhaps
>>> not the fix,
>>> but the explanation. The code being changed can't be understood
>>> without seeing
>>> the method it calls, and the native method it in turn calls.
>>>
>>> Can you provide a detailed explanation as to how this change
>>> propagates down
>>> and does the right thing ?
>>>
>> The call flow:
>>
>> updateFocusableWindowState() calls setStyleBits with style bits to be
>> set on the window.
>> setStyleBits() calls native method nativeSetNSWindowStyleBits.
>> nativeSetNSWindowStyleBits passes mask and 0 as the second parameter
>> in our case (for non-focusable windows).
>> Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits in
>> AWTWindow.m generates newBits and applies it on the NSWindow.
>>
>> My previous fix for issue 7158623 explains bits set on the window.
>> http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071
>>
>>
>>> BTW stylistically - if this is the right fix - you could do :
>>>
>>> setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable)
>>> ? RESIZABLE : 0), isFocusable);
>> Changed the code as per the suggestion. Please review the modified code.
>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/
>>
>> Regards,
>> Manajit
>>
>>> -phil.
>>>
>>> On 06/14/2018 11:37 PM, Manajit Halder wrote:
>>>> Hi All,
>>>>
>>>> Kindly review the fix for JDK11.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8204860
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ 
>>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>
>>>>
>>>> Fix:
>>>> Frame is focusable:
>>>> Retaining the existing frame resizable behaviour (Fixes the current
>>>> issue).
>>>> Frame is non-focusable:
>>>> Making the Frame non-resizable (Fix for issue 7158623).
>>>>
>>>> Regards,
>>>> Manajit
>>>
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Manajit Halder
Hi Sergey,

Thanks for your review comment. I have modified the fix to incorporate your suggestions. 
Please review the modified webrev:

Regards,
Manajit

On 26-Jun-2018, at 4:59 AM, Sergey Bylokhov <[hidden email]> wrote:

Hi, Manajit.
There is one more inconsistency, I have tested it using the test for teh previous fix: UnfocusableMaximizedFrameResizablity.java

In this case the window is zoomable(the green button is active, but does not work as expected):
       frame.setFocusableWindowState(false);
       frame.setResizable(true);

In this case the window is not zoomable(the green button is inactive):
       frame.setResizable(true);
       frame.setFocusableWindowState(false);

I suggest to update the testcase to cover all this cases which we found during review.

On 21/06/2018 12:26, Manajit Halder wrote:
Hi Phil and Sergey,
I have changed the code as per your suggestions. Now window is resized based on the following condition:
If window is non-focusable OR window is focusable but non-resizable THEN window is made non-resizable.
Otherwise window is made resizable (when window is resizable and focusable).
Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/
Note: Fix was verified by running all the java/awt/ jtreg test cases and by executing the reported JCK interactive test case.
Regards,
Manajit
On 20-Jun-2018, at 6:27 PM, Manajit Halder <[hidden email] <mailto:[hidden email]>> wrote:

Hi Phil,

Please find my answer inline to your comment.

On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email] <mailto:[hidden email]>> wrote:

I would like to refer back to a comment you made in the previous fix
http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html

> It is not mentioned in the focus spec whether the unfocusable maximized frames should be resizable or not.

Yet there seems to be a JCK test that says it should not be resizable ?

Can you review the spec. again ?
JCK must have based the test on something .. else the test is not valid.

Yes, I checked FocusSpec.html and it doesn’t specify anything about resizable behaviour of non-focusable Frame.

The UnfocusableMaximizedFrameResizablity.java  test passes on Window and Linux and fails on Mac OS.
Fix for issue 7158623 was done accordingly to make sure the behaviour is same on all platforms.

If this behaviour is not correct then Window and Linux code should be changed accordingly so that all three platforms behave same.


If we want that behaviour specified .. we should be specifying it ..
But I am not sure if it is actually enforceable on all window managers / desktops.

But I have the same issue with this fix as the previous one. Perhaps not the fix,
but the explanation. The code being changed can't be understood without seeing
the method it calls, and the native method it in turn calls.

Can you provide a detailed explanation as to how this change propagates down
and does the right thing ?

The call flow:

updateFocusableWindowState() calls setStyleBits with style bits to be set on the window.
setStyleBits() calls native method nativeSetNSWindowStyleBits. nativeSetNSWindowStyleBits passes mask and 0 as the second parameter in our case (for non-focusable windows).
Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits in AWTWindow.m generates newBits and applies it on the NSWindow.

My previous fix for issue 7158623 explains bits set on the window.
http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071


BTW stylistically - if this is the right fix - you could do :

setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable) ? RESIZABLE : 0), isFocusable);
Changed the code as per the suggestion. Please review the modified code.
http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/

Regards,
Manajit

-phil.

On 06/14/2018 11:37 PM, Manajit Halder wrote:
Hi All,

Kindly review the fix for JDK11.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8204860

Webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>

Fix:
Frame is focusable:
Retaining the existing frame resizable behaviour (Fixes the current issue).
Frame is non-focusable:
Making the Frame non-resizable (Fix for issue 7158623).

Regards,
Manajit




--
Best regards, Sergey.

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Sergey Bylokhov
Hi, Manajit
Can you please simplify such new code in the fix:
(!isFocusable || !isResizable) ? false : true

"false: true" may be omitted.

Also please split this new long line:
486 return (target instanceof Frame) ? ((Frame)target).isResizable() :
((target instanceof Dialog) ? ((Dialog)target).isResizable() : false

On 28/06/2018 05:54, Manajit Halder wrote:

> Hi Sergey,
>
> Thanks for your review comment. I have modified the fix to incorporate
> your suggestions.
> Please review the modified webrev:
> http://cr.openjdk.java.net/~mhalder/8204860/webrev.03/
>
> Regards,
> Manajit
>
>> On 26-Jun-2018, at 4:59 AM, Sergey Bylokhov
>> <[hidden email] <mailto:[hidden email]>> wrote:
>>
>> Hi, Manajit.
>> There is one more inconsistency, I have tested it using the test for
>> teh previous fix: UnfocusableMaximizedFrameResizablity.java
>>
>> In this case the window is zoomable(the green button is active, but
>> does not work as expected):
>>        frame.setFocusableWindowState(false);
>>        frame.setResizable(true);
>>
>> In this case the window is not zoomable(the green button is inactive):
>>        frame.setResizable(true);
>>        frame.setFocusableWindowState(false);
>>
>> I suggest to update the testcase to cover all this cases which we
>> found during review.
>>
>> On 21/06/2018 12:26, Manajit Halder wrote:
>>> Hi Phil and Sergey,
>>> I have changed the code as per your suggestions. Now window is
>>> resized based on the following condition:
>>> If window is non-focusable OR window is focusable but non-resizable
>>> THEN window is made non-resizable.
>>> Otherwise window is made resizable (when window is resizable and
>>> focusable).
>>> Please review the changes:
>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/
>>> Note: Fix was verified by running all the java/awt/ jtreg test cases
>>> and by executing the reported JCK interactive test case.
>>> Regards,
>>> Manajit
>>>> On 20-Jun-2018, at 6:27 PM, Manajit Halder
>>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>>>
>>>> Hi Phil,
>>>>
>>>> Please find my answer inline to your comment.
>>>>
>>>>> On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email]
>>>>> <mailto:[hidden email]>> wrote:
>>>>>
>>>>> I would like to refer back to a comment you made in the previous fix
>>>>> http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html
>>>>>
>>>>> > It is not mentioned in the focus spec whether the unfocusable
>>>>> maximized frames should be resizable or not.
>>>>>
>>>>> Yet there seems to be a JCK test that says it should not be resizable ?
>>>>>
>>>>> Can you review the spec. again ?
>>>>> JCK must have based the test on something .. else the test is not
>>>>> valid.
>>>>
>>>> Yes, I checked FocusSpec.html and it doesn’t specify anything about
>>>> resizable behaviour of non-focusable Frame.
>>>>
>>>> The UnfocusableMaximizedFrameResizablity.java  test passes on Window
>>>> and Linux and fails on Mac OS.
>>>> Fix for issue 7158623 was done accordingly to make sure the
>>>> behaviour is same on all platforms.
>>>>
>>>> If this behaviour is not correct then Window and Linux code should
>>>> be changed accordingly so that all three platforms behave same.
>>>>
>>>>>
>>>>> If we want that behaviour specified .. we should be specifying it ..
>>>>> But I am not sure if it is actually enforceable on all window
>>>>> managers / desktops.
>>>>>
>>>>> But I have the same issue with this fix as the previous one.
>>>>> Perhaps not the fix,
>>>>> but the explanation. The code being changed can't be understood
>>>>> without seeing
>>>>> the method it calls, and the native method it in turn calls.
>>>>>
>>>>> Can you provide a detailed explanation as to how this change
>>>>> propagates down
>>>>> and does the right thing ?
>>>>>
>>>> The call flow:
>>>>
>>>> updateFocusableWindowState() calls setStyleBits with style bits to
>>>> be set on the window.
>>>> setStyleBits() calls native method nativeSetNSWindowStyleBits.
>>>> nativeSetNSWindowStyleBits passes mask and 0 as the second parameter
>>>> in our case (for non-focusable windows).
>>>> Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits in
>>>> AWTWindow.m generates newBits and applies it on the NSWindow.
>>>>
>>>> My previous fix for issue 7158623 explains bits set on the window.
>>>> http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071
>>>>
>>>>
>>>>> BTW stylistically - if this is the right fix - you could do :
>>>>>
>>>>> setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN |
>>>>> ((isFocusable) ? RESIZABLE : 0), isFocusable);
>>>> Changed the code as per the suggestion. Please review the modified code.
>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/
>>>>
>>>> Regards,
>>>> Manajit
>>>>
>>>>> -phil.
>>>>>
>>>>> On 06/14/2018 11:37 PM, Manajit Halder wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> Kindly review the fix for JDK11.
>>>>>>
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8204860
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ 
>>>>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>
>>>>>>
>>>>>> Fix:
>>>>>> Frame is focusable:
>>>>>> Retaining the existing frame resizable behaviour (Fixes the
>>>>>> current issue).
>>>>>> Frame is non-focusable:
>>>>>> Making the Frame non-resizable (Fix for issue 7158623).
>>>>>>
>>>>>> Regards,
>>>>>> Manajit
>>>>>
>>>>
>>
>>
>> --
>> Best regards, Sergey.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Manajit Halder
Hi Sergey,

Thanks for your review comment. Code changed as per your suggestion. 
Please review the changes: 

Regards,
Manajit
 
On 01-Jul-2018, at 4:45 AM, Sergey Bylokhov <[hidden email]> wrote:

Hi, Manajit
Can you please simplify such new code in the fix:
(!isFocusable || !isResizable) ? false : true

"false: true" may be omitted.

Also please split this new long line:
486 return (target instanceof Frame) ? ((Frame)target).isResizable() : ((target instanceof Dialog) ? ((Dialog)target).isResizable() : false

On 28/06/2018 05:54, Manajit Halder wrote:
Hi Sergey,
Thanks for your review comment. I have modified the fix to incorporate your suggestions.
Please review the modified webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.03/
Regards,
Manajit
On 26-Jun-2018, at 4:59 AM, Sergey Bylokhov <[hidden email] <[hidden email]>> wrote:

Hi, Manajit.
There is one more inconsistency, I have tested it using the test for teh previous fix: UnfocusableMaximizedFrameResizablity.java

In this case the window is zoomable(the green button is active, but does not work as expected):
       frame.setFocusableWindowState(false);
       frame.setResizable(true);

In this case the window is not zoomable(the green button is inactive):
       frame.setResizable(true);
       frame.setFocusableWindowState(false);

I suggest to update the testcase to cover all this cases which we found during review.

On 21/06/2018 12:26, Manajit Halder wrote:
Hi Phil and Sergey,
I have changed the code as per your suggestions. Now window is resized based on the following condition:
If window is non-focusable OR window is focusable but non-resizable THEN window is made non-resizable.
Otherwise window is made resizable (when window is resizable and focusable).
Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/
Note: Fix was verified by running all the java/awt/ jtreg test cases and by executing the reported JCK interactive test case.
Regards,
Manajit
On 20-Jun-2018, at 6:27 PM, Manajit Halder <[hidden email] <mailto:[hidden email]>> wrote:

Hi Phil,

Please find my answer inline to your comment.

On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email] <mailto:[hidden email]>> wrote:

I would like to refer back to a comment you made in the previous fix
http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html

> It is not mentioned in the focus spec whether the unfocusable maximized frames should be resizable or not.

Yet there seems to be a JCK test that says it should not be resizable ?

Can you review the spec. again ?
JCK must have based the test on something .. else the test is not valid.

Yes, I checked FocusSpec.html and it doesn’t specify anything about resizable behaviour of non-focusable Frame.

The UnfocusableMaximizedFrameResizablity.java  test passes on Window and Linux and fails on Mac OS.
Fix for issue 7158623 was done accordingly to make sure the behaviour is same on all platforms.

If this behaviour is not correct then Window and Linux code should be changed accordingly so that all three platforms behave same.


If we want that behaviour specified .. we should be specifying it ..
But I am not sure if it is actually enforceable on all window managers / desktops.

But I have the same issue with this fix as the previous one. Perhaps not the fix,
but the explanation. The code being changed can't be understood without seeing
the method it calls, and the native method it in turn calls.

Can you provide a detailed explanation as to how this change propagates down
and does the right thing ?

The call flow:

updateFocusableWindowState() calls setStyleBits with style bits to be set on the window.
setStyleBits() calls native method nativeSetNSWindowStyleBits. nativeSetNSWindowStyleBits passes mask and 0 as the second parameter in our case (for non-focusable windows).
Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits in AWTWindow.m generates newBits and applies it on the NSWindow.

My previous fix for issue 7158623 explains bits set on the window.
http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071


BTW stylistically - if this is the right fix - you could do :

setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable) ? RESIZABLE : 0), isFocusable);
Changed the code as per the suggestion. Please review the modified code.
http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/

Regards,
Manajit

-phil.

On 06/14/2018 11:37 PM, Manajit Halder wrote:
Hi All,

Kindly review the fix for JDK11.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8204860

Webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>

Fix:
Frame is focusable:
Retaining the existing frame resizable behaviour (Fixes the current issue).
Frame is non-focusable:
Making the Frame non-resizable (Fix for issue 7158623).

Regards,
Manajit




-- 
Best regards, Sergey.


-- 
Best regards, Sergey.

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Sergey Bylokhov
Hi, Manajit.
Can you please recheck the usage of setCanFullscreen() method at line
691. Is it possible to make the window "FULLSCREENABLE" when it is not
focusable? I guess it can be checked by something like this:


frame.setFocusableWindowState(false);
frame.setResizable(true);
v1 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
frame.setVisible(false);
frame.setVisible(true);
v2 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
if(!v1.equals(v2)) Error();


On 02/07/2018 20:12, Manajit Halder wrote:

> Hi Sergey,
>
> Thanks for your review comment. Code changed as per your suggestion.
> Please review the changes:
> http://cr.openjdk.java.net/~mhalder/8204860/webrev.04/
>
> Regards,
> Manajit
>
>> On 01-Jul-2018, at 4:45 AM, Sergey Bylokhov
>> <[hidden email] <mailto:[hidden email]>> wrote:
>>
>> Hi, Manajit
>> Can you please simplify such new code in the fix:
>> (!isFocusable || !isResizable) ? false : true
>>
>> "false: true" may be omitted.
>>
>> Also please split this new long line:
>> 486 return (target instanceof Frame) ? ((Frame)target).isResizable() :
>> ((target instanceof Dialog) ? ((Dialog)target).isResizable() : false
>>
>> On 28/06/2018 05:54, Manajit Halder wrote:
>>> Hi Sergey,
>>> Thanks for your review comment. I have modified the fix to
>>> incorporate your suggestions.
>>> Please review the modified webrev:
>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.03/
>>> Regards,
>>> Manajit
>>>> On 26-Jun-2018, at 4:59 AM, Sergey Bylokhov
>>>> <[hidden email]
>>>> <mailto:[hidden email]><mailto:[hidden email]>>
>>>> wrote:
>>>>
>>>> Hi, Manajit.
>>>> There is one more inconsistency, I have tested it using the test for
>>>> teh previous fix: UnfocusableMaximizedFrameResizablity.java
>>>>
>>>> In this case the window is zoomable(the green button is active, but
>>>> does not work as expected):
>>>>        frame.setFocusableWindowState(false);
>>>>        frame.setResizable(true);
>>>>
>>>> In this case the window is not zoomable(the green button is inactive):
>>>>        frame.setResizable(true);
>>>>        frame.setFocusableWindowState(false);
>>>>
>>>> I suggest to update the testcase to cover all this cases which we
>>>> found during review.
>>>>
>>>> On 21/06/2018 12:26, Manajit Halder wrote:
>>>>> Hi Phil and Sergey,
>>>>> I have changed the code as per your suggestions. Now window is
>>>>> resized based on the following condition:
>>>>> If window is non-focusable OR window is focusable but non-resizable
>>>>> THEN window is made non-resizable.
>>>>> Otherwise window is made resizable (when window is resizable and
>>>>> focusable).
>>>>> Please review the changes:
>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/
>>>>> Note: Fix was verified by running all the java/awt/ jtreg test
>>>>> cases and by executing the reported JCK interactive test case.
>>>>> Regards,
>>>>> Manajit
>>>>>> On 20-Jun-2018, at 6:27 PM, Manajit Halder
>>>>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>>
>>>>>> Hi Phil,
>>>>>>
>>>>>> Please find my answer inline to your comment.
>>>>>>
>>>>>>> On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email]
>>>>>>> <mailto:[hidden email]>> wrote:
>>>>>>>
>>>>>>> I would like to refer back to a comment you made in the previous fix
>>>>>>> http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html
>>>>>>>
>>>>>>> > It is not mentioned in the focus spec whether the unfocusable
>>>>>>> maximized frames should be resizable or not.
>>>>>>>
>>>>>>> Yet there seems to be a JCK test that says it should not be
>>>>>>> resizable ?
>>>>>>>
>>>>>>> Can you review the spec. again ?
>>>>>>> JCK must have based the test on something .. else the test is not
>>>>>>> valid.
>>>>>>
>>>>>> Yes, I checked FocusSpec.html and it doesn’t specify anything
>>>>>> about resizable behaviour of non-focusable Frame.
>>>>>>
>>>>>> The UnfocusableMaximizedFrameResizablity.java  test passes on
>>>>>> Window and Linux and fails on Mac OS.
>>>>>> Fix for issue 7158623 was done accordingly to make sure the
>>>>>> behaviour is same on all platforms.
>>>>>>
>>>>>> If this behaviour is not correct then Window and Linux code should
>>>>>> be changed accordingly so that all three platforms behave same.
>>>>>>
>>>>>>>
>>>>>>> If we want that behaviour specified .. we should be specifying it ..
>>>>>>> But I am not sure if it is actually enforceable on all window
>>>>>>> managers / desktops.
>>>>>>>
>>>>>>> But I have the same issue with this fix as the previous one.
>>>>>>> Perhaps not the fix,
>>>>>>> but the explanation. The code being changed can't be understood
>>>>>>> without seeing
>>>>>>> the method it calls, and the native method it in turn calls.
>>>>>>>
>>>>>>> Can you provide a detailed explanation as to how this change
>>>>>>> propagates down
>>>>>>> and does the right thing ?
>>>>>>>
>>>>>> The call flow:
>>>>>>
>>>>>> updateFocusableWindowState() calls setStyleBits with style bits to
>>>>>> be set on the window.
>>>>>> setStyleBits() calls native method nativeSetNSWindowStyleBits.
>>>>>> nativeSetNSWindowStyleBits passes mask and 0 as the second
>>>>>> parameter in our case (for non-focusable windows).
>>>>>> Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits
>>>>>> in AWTWindow.m generates newBits and applies it on the NSWindow.
>>>>>>
>>>>>> My previous fix for issue 7158623 explains bits set on the window.
>>>>>> http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071
>>>>>>
>>>>>>
>>>>>>> BTW stylistically - if this is the right fix - you could do :
>>>>>>>
>>>>>>> setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN |
>>>>>>> ((isFocusable) ? RESIZABLE : 0), isFocusable);
>>>>>> Changed the code as per the suggestion. Please review the modified
>>>>>> code.
>>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/
>>>>>>
>>>>>> Regards,
>>>>>> Manajit
>>>>>>
>>>>>>> -phil.
>>>>>>>
>>>>>>> On 06/14/2018 11:37 PM, Manajit Halder wrote:
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>> Kindly review the fix for JDK11.
>>>>>>>>
>>>>>>>> Bug:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8204860
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ 
>>>>>>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>
>>>>>>>>
>>>>>>>> Fix:
>>>>>>>> Frame is focusable:
>>>>>>>> Retaining the existing frame resizable behaviour (Fixes the
>>>>>>>> current issue).
>>>>>>>> Frame is non-focusable:
>>>>>>>> Making the Frame non-resizable (Fix for issue 7158623).
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Manajit
>>>>>>>
>>>>>>
>>>>
>>>>
>>>> --
>>>> Best regards, Sergey.
>>
>>
>> --
>> Best regards, Sergey.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Manajit Halder
Hi Sergey,

Checked the behaviour of setCanFullscreen() and found the test was failing. The window was "FULLSCREENABLE” when it was not focusable. 
Changed the code to fix this issue. After fix the window (JFrame) is not “FULLSCREENABLE” when it is not resizable and non-focusable.

Please review the changes:

Regards,
Manajit

On 04-Jul-2018, at 5:52 PM, Sergey Bylokhov <[hidden email]> wrote:

Hi, Manajit.
Can you please recheck the usage of setCanFullscreen() method at line 691. Is it possible to make the window "FULLSCREENABLE" when it is not focusable? I guess it can be checked by something like this:


frame.setFocusableWindowState(false);
frame.setResizable(true);
v1 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
frame.setVisible(false);
frame.setVisible(true);
v2 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
if(!v1.equals(v2)) Error();


On 02/07/2018 20:12, Manajit Halder wrote:
Hi Sergey,
Thanks for your review comment. Code changed as per your suggestion.
Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.04/
Regards,
Manajit
On 01-Jul-2018, at 4:45 AM, Sergey Bylokhov <[hidden email] <[hidden email]>> wrote:

Hi, Manajit
Can you please simplify such new code in the fix:
(!isFocusable || !isResizable) ? false : true

"false: true" may be omitted.

Also please split this new long line:
486 return (target instanceof Frame) ? ((Frame)target).isResizable() : ((target instanceof Dialog) ? ((Dialog)target).isResizable() : false

On 28/06/2018 05:54, Manajit Halder wrote:
Hi Sergey,
Thanks for your review comment. I have modified the fix to incorporate your suggestions.
Please review the modified webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.03/
Regards,
Manajit
On 26-Jun-2018, at 4:59 AM, Sergey Bylokhov <[hidden email] <[hidden email]><[hidden email]>> wrote:

Hi, Manajit.
There is one more inconsistency, I have tested it using the test for teh previous fix: UnfocusableMaximizedFrameResizablity.java

In this case the window is zoomable(the green button is active, but does not work as expected):
       frame.setFocusableWindowState(false);
       frame.setResizable(true);

In this case the window is not zoomable(the green button is inactive):
       frame.setResizable(true);
       frame.setFocusableWindowState(false);

I suggest to update the testcase to cover all this cases which we found during review.

On 21/06/2018 12:26, Manajit Halder wrote:
Hi Phil and Sergey,
I have changed the code as per your suggestions. Now window is resized based on the following condition:
If window is non-focusable OR window is focusable but non-resizable THEN window is made non-resizable.
Otherwise window is made resizable (when window is resizable and focusable).
Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/
Note: Fix was verified by running all the java/awt/ jtreg test cases and by executing the reported JCK interactive test case.
Regards,
Manajit
On 20-Jun-2018, at 6:27 PM, Manajit Halder <[hidden email] <mailto:[hidden email]>> wrote:

Hi Phil,

Please find my answer inline to your comment.

On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email] <mailto:[hidden email]>> wrote:

I would like to refer back to a comment you made in the previous fix
http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html

> It is not mentioned in the focus spec whether the unfocusable maximized frames should be resizable or not.

Yet there seems to be a JCK test that says it should not be resizable ?

Can you review the spec. again ?
JCK must have based the test on something .. else the test is not valid.

Yes, I checked FocusSpec.html and it doesn’t specify anything about resizable behaviour of non-focusable Frame.

The UnfocusableMaximizedFrameResizablity.java  test passes on Window and Linux and fails on Mac OS.
Fix for issue 7158623 was done accordingly to make sure the behaviour is same on all platforms.

If this behaviour is not correct then Window and Linux code should be changed accordingly so that all three platforms behave same.


If we want that behaviour specified .. we should be specifying it ..
But I am not sure if it is actually enforceable on all window managers / desktops.

But I have the same issue with this fix as the previous one. Perhaps not the fix,
but the explanation. The code being changed can't be understood without seeing
the method it calls, and the native method it in turn calls.

Can you provide a detailed explanation as to how this change propagates down
and does the right thing ?

The call flow:

updateFocusableWindowState() calls setStyleBits with style bits to be set on the window.
setStyleBits() calls native method nativeSetNSWindowStyleBits. nativeSetNSWindowStyleBits passes mask and 0 as the second parameter in our case (for non-focusable windows).
Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits in AWTWindow.m generates newBits and applies it on the NSWindow.

My previous fix for issue 7158623 explains bits set on the window.
http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071


BTW stylistically - if this is the right fix - you could do :

setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable) ? RESIZABLE : 0), isFocusable);
Changed the code as per the suggestion. Please review the modified code.
http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/

Regards,
Manajit

-phil.

On 06/14/2018 11:37 PM, Manajit Halder wrote:
Hi All,

Kindly review the fix for JDK11.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8204860

Webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>

Fix:
Frame is focusable:
Retaining the existing frame resizable behaviour (Fixes the current issue).
Frame is non-focusable:
Making the Frame non-resizable (Fix for issue 7158623).

Regards,
Manajit




--
Best regards, Sergey.


--
Best regards, Sergey.


-- 
Best regards, Sergey.

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Phil Race
+1 from me.

-phil.

On 07/06/2018 06:37 AM, Manajit Halder wrote:
Hi Sergey,

Checked the behaviour of setCanFullscreen() and found the test was failing. The window was "FULLSCREENABLE” when it was not focusable. 
Changed the code to fix this issue. After fix the window (JFrame) is not “FULLSCREENABLE” when it is not resizable and non-focusable.

Please review the changes:

Regards,
Manajit

On 04-Jul-2018, at 5:52 PM, Sergey Bylokhov <[hidden email]> wrote:

Hi, Manajit.
Can you please recheck the usage of setCanFullscreen() method at line 691. Is it possible to make the window "FULLSCREENABLE" when it is not focusable? I guess it can be checked by something like this:


frame.setFocusableWindowState(false);
frame.setResizable(true);
v1 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
frame.setVisible(false);
frame.setVisible(true);
v2 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
if(!v1.equals(v2)) Error();


On 02/07/2018 20:12, Manajit Halder wrote:
Hi Sergey,
Thanks for your review comment. Code changed as per your suggestion.
Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.04/
Regards,
Manajit
On 01-Jul-2018, at 4:45 AM, Sergey Bylokhov <[hidden email] <[hidden email]>> wrote:

Hi, Manajit
Can you please simplify such new code in the fix:
(!isFocusable || !isResizable) ? false : true

"false: true" may be omitted.

Also please split this new long line:
486 return (target instanceof Frame) ? ((Frame)target).isResizable() : ((target instanceof Dialog) ? ((Dialog)target).isResizable() : false

On 28/06/2018 05:54, Manajit Halder wrote:
Hi Sergey,
Thanks for your review comment. I have modified the fix to incorporate your suggestions.
Please review the modified webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.03/
Regards,
Manajit
On 26-Jun-2018, at 4:59 AM, Sergey Bylokhov <[hidden email] <[hidden email]><[hidden email]>> wrote:

Hi, Manajit.
There is one more inconsistency, I have tested it using the test for teh previous fix: UnfocusableMaximizedFrameResizablity.java

In this case the window is zoomable(the green button is active, but does not work as expected):
       frame.setFocusableWindowState(false);
       frame.setResizable(true);

In this case the window is not zoomable(the green button is inactive):
       frame.setResizable(true);
       frame.setFocusableWindowState(false);

I suggest to update the testcase to cover all this cases which we found during review.

On 21/06/2018 12:26, Manajit Halder wrote:
Hi Phil and Sergey,
I have changed the code as per your suggestions. Now window is resized based on the following condition:
If window is non-focusable OR window is focusable but non-resizable THEN window is made non-resizable.
Otherwise window is made resizable (when window is resizable and focusable).
Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/
Note: Fix was verified by running all the java/awt/ jtreg test cases and by executing the reported JCK interactive test case.
Regards,
Manajit
On 20-Jun-2018, at 6:27 PM, Manajit Halder <[hidden email] [hidden email]> wrote:

Hi Phil,

Please find my answer inline to your comment.

On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email] [hidden email]> wrote:

I would like to refer back to a comment you made in the previous fix
http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html

> It is not mentioned in the focus spec whether the unfocusable maximized frames should be resizable or not.

Yet there seems to be a JCK test that says it should not be resizable ?

Can you review the spec. again ?
JCK must have based the test on something .. else the test is not valid.

Yes, I checked FocusSpec.html and it doesn’t specify anything about resizable behaviour of non-focusable Frame.

The UnfocusableMaximizedFrameResizablity.java  test passes on Window and Linux and fails on Mac OS.
Fix for issue 7158623 was done accordingly to make sure the behaviour is same on all platforms.

If this behaviour is not correct then Window and Linux code should be changed accordingly so that all three platforms behave same.


If we want that behaviour specified .. we should be specifying it ..
But I am not sure if it is actually enforceable on all window managers / desktops.

But I have the same issue with this fix as the previous one. Perhaps not the fix,
but the explanation. The code being changed can't be understood without seeing
the method it calls, and the native method it in turn calls.

Can you provide a detailed explanation as to how this change propagates down
and does the right thing ?

The call flow:

updateFocusableWindowState() calls setStyleBits with style bits to be set on the window.
setStyleBits() calls native method nativeSetNSWindowStyleBits. nativeSetNSWindowStyleBits passes mask and 0 as the second parameter in our case (for non-focusable windows).
Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits in AWTWindow.m generates newBits and applies it on the NSWindow.

My previous fix for issue 7158623 explains bits set on the window.
http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071


BTW stylistically - if this is the right fix - you could do :

setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable) ? RESIZABLE : 0), isFocusable);
Changed the code as per the suggestion. Please review the modified code.
http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/

Regards,
Manajit

-phil.

On 06/14/2018 11:37 PM, Manajit Halder wrote:
Hi All,

Kindly review the fix for JDK11.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8204860

Webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>

Fix:
Frame is focusable:
Retaining the existing frame resizable behaviour (Fixes the current issue).
Frame is non-focusable:
Making the Frame non-resizable (Fix for issue 7158623).

Regards,
Manajit




--
Best regards, Sergey.


--
Best regards, Sergey.


-- 
Best regards, Sergey.


Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Sergey Bylokhov
In reply to this post by Manajit Halder
Hi, Manajit.
Did you run the test on other platforms?
I am not sure, but look like the line below can throw npe:
if(!prop1.equals(prop2)) {

On 06/07/2018 16:37, Manajit Halder wrote:

> Hi Sergey,
>
> Checked the behaviour of setCanFullscreen() and found the test was
> failing. The window was "FULLSCREENABLE” when it was not focusable.
> Changed the code to fix this issue. After fix the window (JFrame) is not
> “FULLSCREENABLE” when it is not resizable and non-focusable.
>
> Please review the changes:
> http://cr.openjdk.java.net/~mhalder/8204860/webrev.05/ 
> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.05/>
>
> Regards,
> Manajit
>
>> On 04-Jul-2018, at 5:52 PM, Sergey Bylokhov
>> <[hidden email] <mailto:[hidden email]>> wrote:
>>
>> Hi, Manajit.
>> Can you please recheck the usage of setCanFullscreen() method at line
>> 691. Is it possible to make the window "FULLSCREENABLE" when it is not
>> focusable? I guess it can be checked by something like this:
>>
>>
>> frame.setFocusableWindowState(false);
>> frame.setResizable(true);
>> v1 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
>> frame.setVisible(false);
>> frame.setVisible(true);
>> v2 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
>> if(!v1.equals(v2)) Error();
>>
>>
>> On 02/07/2018 20:12, Manajit Halder wrote:
>>> Hi Sergey,
>>> Thanks for your review comment. Code changed as per your suggestion.
>>> Please review the changes:
>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.04/ 
>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.04/>
>>> Regards,
>>> Manajit
>>>> On 01-Jul-2018, at 4:45 AM, Sergey Bylokhov
>>>> <[hidden email]
>>>> <mailto:[hidden email]><mailto:[hidden email]>>
>>>> wrote:
>>>>
>>>> Hi, Manajit
>>>> Can you please simplify such new code in the fix:
>>>> (!isFocusable || !isResizable) ? false : true
>>>>
>>>> "false: true" may be omitted.
>>>>
>>>> Also please split this new long line:
>>>> 486 return (target instanceof Frame) ? ((Frame)target).isResizable()
>>>> : ((target instanceof Dialog) ? ((Dialog)target).isResizable() : false
>>>>
>>>> On 28/06/2018 05:54, Manajit Halder wrote:
>>>>> Hi Sergey,
>>>>> Thanks for your review comment. I have modified the fix to
>>>>> incorporate your suggestions.
>>>>> Please review the modified webrev:
>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.03/ 
>>>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.03/>
>>>>> Regards,
>>>>> Manajit
>>>>>> On 26-Jun-2018, at 4:59 AM, Sergey Bylokhov
>>>>>> <[hidden email]
>>>>>> <mailto:[hidden email]><mailto:[hidden email]><mailto:[hidden email]>>
>>>>>> wrote:
>>>>>>
>>>>>> Hi, Manajit.
>>>>>> There is one more inconsistency, I have tested it using the test
>>>>>> for teh previous fix: UnfocusableMaximizedFrameResizablity.java
>>>>>>
>>>>>> In this case the window is zoomable(the green button is active,
>>>>>> but does not work as expected):
>>>>>>        frame.setFocusableWindowState(false);
>>>>>>        frame.setResizable(true);
>>>>>>
>>>>>> In this case the window is not zoomable(the green button is inactive):
>>>>>>        frame.setResizable(true);
>>>>>>        frame.setFocusableWindowState(false);
>>>>>>
>>>>>> I suggest to update the testcase to cover all this cases which we
>>>>>> found during review.
>>>>>>
>>>>>> On 21/06/2018 12:26, Manajit Halder wrote:
>>>>>>> Hi Phil and Sergey,
>>>>>>> I have changed the code as per your suggestions. Now window is
>>>>>>> resized based on the following condition:
>>>>>>> If window is non-focusable OR window is focusable but
>>>>>>> non-resizable THEN window is made non-resizable.
>>>>>>> Otherwise window is made resizable (when window is resizable and
>>>>>>> focusable).
>>>>>>> Please review the changes:
>>>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/ 
>>>>>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.02/>
>>>>>>> Note: Fix was verified by running all the java/awt/ jtreg test
>>>>>>> cases and by executing the reported JCK interactive test case.
>>>>>>> Regards,
>>>>>>> Manajit
>>>>>>>> On 20-Jun-2018, at 6:27 PM, Manajit Halder
>>>>>>>> <[hidden email] <mailto:[hidden email]>>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Hi Phil,
>>>>>>>>
>>>>>>>> Please find my answer inline to your comment.
>>>>>>>>
>>>>>>>>> On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email]
>>>>>>>>> <mailto:[hidden email]>> wrote:
>>>>>>>>>
>>>>>>>>> I would like to refer back to a comment you made in the
>>>>>>>>> previous fix
>>>>>>>>> http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html
>>>>>>>>>
>>>>>>>>> > It is not mentioned in the focus spec whether the unfocusable
>>>>>>>>> maximized frames should be resizable or not.
>>>>>>>>>
>>>>>>>>> Yet there seems to be a JCK test that says it should not be
>>>>>>>>> resizable ?
>>>>>>>>>
>>>>>>>>> Can you review the spec. again ?
>>>>>>>>> JCK must have based the test on something .. else the test is
>>>>>>>>> not valid.
>>>>>>>>
>>>>>>>> Yes, I checked FocusSpec.html and it doesn’t specify anything
>>>>>>>> about resizable behaviour of non-focusable Frame.
>>>>>>>>
>>>>>>>> The UnfocusableMaximizedFrameResizablity.java  test passes on
>>>>>>>> Window and Linux and fails on Mac OS.
>>>>>>>> Fix for issue 7158623 was done accordingly to make sure the
>>>>>>>> behaviour is same on all platforms.
>>>>>>>>
>>>>>>>> If this behaviour is not correct then Window and Linux code
>>>>>>>> should be changed accordingly so that all three platforms behave
>>>>>>>> same.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> If we want that behaviour specified .. we should be specifying
>>>>>>>>> it ..
>>>>>>>>> But I am not sure if it is actually enforceable on all window
>>>>>>>>> managers / desktops.
>>>>>>>>>
>>>>>>>>> But I have the same issue with this fix as the previous one.
>>>>>>>>> Perhaps not the fix,
>>>>>>>>> but the explanation. The code being changed can't be understood
>>>>>>>>> without seeing
>>>>>>>>> the method it calls, and the native method it in turn calls.
>>>>>>>>>
>>>>>>>>> Can you provide a detailed explanation as to how this change
>>>>>>>>> propagates down
>>>>>>>>> and does the right thing ?
>>>>>>>>>
>>>>>>>> The call flow:
>>>>>>>>
>>>>>>>> updateFocusableWindowState() calls setStyleBits with style bits
>>>>>>>> to be set on the window.
>>>>>>>> setStyleBits() calls native method nativeSetNSWindowStyleBits.
>>>>>>>> nativeSetNSWindowStyleBits passes mask and 0 as the second
>>>>>>>> parameter in our case (for non-focusable windows).
>>>>>>>> Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits
>>>>>>>> in AWTWindow.m generates newBits and applies it on the NSWindow.
>>>>>>>>
>>>>>>>> My previous fix for issue 7158623 explains bits set on the window.
>>>>>>>> http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071
>>>>>>>>
>>>>>>>>
>>>>>>>>> BTW stylistically - if this is the right fix - you could do :
>>>>>>>>>
>>>>>>>>> setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN |
>>>>>>>>> ((isFocusable) ? RESIZABLE : 0), isFocusable);
>>>>>>>> Changed the code as per the suggestion. Please review the
>>>>>>>> modified code.
>>>>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Manajit
>>>>>>>>
>>>>>>>>> -phil.
>>>>>>>>>
>>>>>>>>> On 06/14/2018 11:37 PM, Manajit Halder wrote:
>>>>>>>>>> Hi All,
>>>>>>>>>>
>>>>>>>>>> Kindly review the fix for JDK11.
>>>>>>>>>>
>>>>>>>>>> Bug:
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8204860
>>>>>>>>>>
>>>>>>>>>> Webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ 
>>>>>>>>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>
>>>>>>>>>>
>>>>>>>>>> Fix:
>>>>>>>>>> Frame is focusable:
>>>>>>>>>> Retaining the existing frame resizable behaviour (Fixes the
>>>>>>>>>> current issue).
>>>>>>>>>> Frame is non-focusable:
>>>>>>>>>> Making the Frame non-resizable (Fix for issue 7158623).
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Manajit
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Best regards, Sergey.
>>>>
>>>>
>>>> --
>>>> Best regards, Sergey.
>>
>>
>> --
>> Best regards, Sergey.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

prasanta sadhukhan
Also, I guess JFrame handling should be in EDT.

Regards
Prasanta
On 7/9/2018 6:14 PM, Sergey Bylokhov wrote:

> Hi, Manajit.
> Did you run the test on other platforms?
> I am not sure, but look like the line below can throw npe:
> if(!prop1.equals(prop2)) {
>
> On 06/07/2018 16:37, Manajit Halder wrote:
>> Hi Sergey,
>>
>> Checked the behaviour of setCanFullscreen() and found the test was
>> failing. The window was "FULLSCREENABLE” when it was not focusable.
>> Changed the code to fix this issue. After fix the window (JFrame) is
>> not “FULLSCREENABLE” when it is not resizable and non-focusable.
>>
>> Please review the changes:
>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.05/ 
>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.05/>
>>
>> Regards,
>> Manajit
>>
>>> On 04-Jul-2018, at 5:52 PM, Sergey Bylokhov
>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>>
>>> Hi, Manajit.
>>> Can you please recheck the usage of setCanFullscreen() method at
>>> line 691. Is it possible to make the window "FULLSCREENABLE" when it
>>> is not focusable? I guess it can be checked by something like this:
>>>
>>>
>>> frame.setFocusableWindowState(false);
>>> frame.setResizable(true);
>>> v1 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
>>> frame.setVisible(false);
>>> frame.setVisible(true);
>>> v2 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
>>> if(!v1.equals(v2)) Error();
>>>
>>>
>>> On 02/07/2018 20:12, Manajit Halder wrote:
>>>> Hi Sergey,
>>>> Thanks for your review comment. Code changed as per your suggestion.
>>>> Please review the changes:
>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.04/ 
>>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.04/>
>>>> Regards,
>>>> Manajit
>>>>> On 01-Jul-2018, at 4:45 AM, Sergey Bylokhov
>>>>> <[hidden email]
>>>>> <mailto:[hidden email]><mailto:[hidden email]>>
>>>>> wrote:
>>>>>
>>>>> Hi, Manajit
>>>>> Can you please simplify such new code in the fix:
>>>>> (!isFocusable || !isResizable) ? false : true
>>>>>
>>>>> "false: true" may be omitted.
>>>>>
>>>>> Also please split this new long line:
>>>>> 486 return (target instanceof Frame) ?
>>>>> ((Frame)target).isResizable() : ((target instanceof Dialog) ?
>>>>> ((Dialog)target).isResizable() : false
>>>>>
>>>>> On 28/06/2018 05:54, Manajit Halder wrote:
>>>>>> Hi Sergey,
>>>>>> Thanks for your review comment. I have modified the fix to
>>>>>> incorporate your suggestions.
>>>>>> Please review the modified webrev:
>>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.03/ 
>>>>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.03/>
>>>>>> Regards,
>>>>>> Manajit
>>>>>>> On 26-Jun-2018, at 4:59 AM, Sergey Bylokhov
>>>>>>> <[hidden email]
>>>>>>> <mailto:[hidden email]><mailto:[hidden email]><mailto:[hidden email]>>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hi, Manajit.
>>>>>>> There is one more inconsistency, I have tested it using the test
>>>>>>> for teh previous fix: UnfocusableMaximizedFrameResizablity.java
>>>>>>>
>>>>>>> In this case the window is zoomable(the green button is active,
>>>>>>> but does not work as expected):
>>>>>>>        frame.setFocusableWindowState(false);
>>>>>>>        frame.setResizable(true);
>>>>>>>
>>>>>>> In this case the window is not zoomable(the green button is
>>>>>>> inactive):
>>>>>>>        frame.setResizable(true);
>>>>>>>        frame.setFocusableWindowState(false);
>>>>>>>
>>>>>>> I suggest to update the testcase to cover all this cases which
>>>>>>> we found during review.
>>>>>>>
>>>>>>> On 21/06/2018 12:26, Manajit Halder wrote:
>>>>>>>> Hi Phil and Sergey,
>>>>>>>> I have changed the code as per your suggestions. Now window is
>>>>>>>> resized based on the following condition:
>>>>>>>> If window is non-focusable OR window is focusable but
>>>>>>>> non-resizable THEN window is made non-resizable.
>>>>>>>> Otherwise window is made resizable (when window is resizable
>>>>>>>> and focusable).
>>>>>>>> Please review the changes:
>>>>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/ 
>>>>>>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.02/>
>>>>>>>> Note: Fix was verified by running all the java/awt/ jtreg test
>>>>>>>> cases and by executing the reported JCK interactive test case.
>>>>>>>> Regards,
>>>>>>>> Manajit
>>>>>>>>> On 20-Jun-2018, at 6:27 PM, Manajit Halder
>>>>>>>>> <[hidden email] <mailto:[hidden email]>>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Hi Phil,
>>>>>>>>>
>>>>>>>>> Please find my answer inline to your comment.
>>>>>>>>>
>>>>>>>>>> On 15-Jun-2018, at 11:24 PM, Phil Race
>>>>>>>>>> <[hidden email] <mailto:[hidden email]>> wrote:
>>>>>>>>>>
>>>>>>>>>> I would like to refer back to a comment you made in the
>>>>>>>>>> previous fix
>>>>>>>>>> http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> > It is not mentioned in the focus spec whether the
>>>>>>>>>> unfocusable maximized frames should be resizable or not.
>>>>>>>>>>
>>>>>>>>>> Yet there seems to be a JCK test that says it should not be
>>>>>>>>>> resizable ?
>>>>>>>>>>
>>>>>>>>>> Can you review the spec. again ?
>>>>>>>>>> JCK must have based the test on something .. else the test is
>>>>>>>>>> not valid.
>>>>>>>>>
>>>>>>>>> Yes, I checked FocusSpec.html and it doesn’t specify anything
>>>>>>>>> about resizable behaviour of non-focusable Frame.
>>>>>>>>>
>>>>>>>>> The UnfocusableMaximizedFrameResizablity.java test passes on
>>>>>>>>> Window and Linux and fails on Mac OS.
>>>>>>>>> Fix for issue 7158623 was done accordingly to make sure the
>>>>>>>>> behaviour is same on all platforms.
>>>>>>>>>
>>>>>>>>> If this behaviour is not correct then Window and Linux code
>>>>>>>>> should be changed accordingly so that all three platforms
>>>>>>>>> behave same.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If we want that behaviour specified .. we should be
>>>>>>>>>> specifying it ..
>>>>>>>>>> But I am not sure if it is actually enforceable on all window
>>>>>>>>>> managers / desktops.
>>>>>>>>>>
>>>>>>>>>> But I have the same issue with this fix as the previous one.
>>>>>>>>>> Perhaps not the fix,
>>>>>>>>>> but the explanation. The code being changed can't be
>>>>>>>>>> understood without seeing
>>>>>>>>>> the method it calls, and the native method it in turn calls.
>>>>>>>>>>
>>>>>>>>>> Can you provide a detailed explanation as to how this change
>>>>>>>>>> propagates down
>>>>>>>>>> and does the right thing ?
>>>>>>>>>>
>>>>>>>>> The call flow:
>>>>>>>>>
>>>>>>>>> updateFocusableWindowState() calls setStyleBits with style
>>>>>>>>> bits to be set on the window.
>>>>>>>>> setStyleBits() calls native method nativeSetNSWindowStyleBits.
>>>>>>>>> nativeSetNSWindowStyleBits passes mask and 0 as the second
>>>>>>>>> parameter in our case (for non-focusable windows).
>>>>>>>>> Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits
>>>>>>>>> in AWTWindow.m generates newBits and applies it on the NSWindow.
>>>>>>>>>
>>>>>>>>> My previous fix for issue 7158623 explains bits set on the
>>>>>>>>> window.
>>>>>>>>> http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> BTW stylistically - if this is the right fix - you could do :
>>>>>>>>>>
>>>>>>>>>> setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN |
>>>>>>>>>> ((isFocusable) ? RESIZABLE : 0), isFocusable);
>>>>>>>>> Changed the code as per the suggestion. Please review the
>>>>>>>>> modified code.
>>>>>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Manajit
>>>>>>>>>
>>>>>>>>>> -phil.
>>>>>>>>>>
>>>>>>>>>> On 06/14/2018 11:37 PM, Manajit Halder wrote:
>>>>>>>>>>> Hi All,
>>>>>>>>>>>
>>>>>>>>>>> Kindly review the fix for JDK11.
>>>>>>>>>>>
>>>>>>>>>>> Bug:
>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8204860
>>>>>>>>>>>
>>>>>>>>>>> Webrev:
>>>>>>>>>>> http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ 
>>>>>>>>>>> <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>
>>>>>>>>>>>
>>>>>>>>>>> Fix:
>>>>>>>>>>> Frame is focusable:
>>>>>>>>>>> Retaining the existing frame resizable behaviour (Fixes the
>>>>>>>>>>> current issue).
>>>>>>>>>>> Frame is non-focusable:
>>>>>>>>>>> Making the Frame non-resizable (Fix for issue 7158623).
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Manajit
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Best regards, Sergey.
>>>>>
>>>>>
>>>>> --
>>>>> Best regards, Sergey.
>>>
>>>
>>> --
>>> Best regards, Sergey.
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

Manajit Halder
Hi Sergey and Prasanta,

Thanks for your comment. The fullscreen test (i.e. case 3 in the test) is made to run only on Mac as the full screen property (“apple.awt.fullscreenable”) is specific to Mac OS.
Please review the updated webrev:

Regards,
Manajit

On 09-Jul-2018, at 6:33 PM, Prasanta Sadhukhan <[hidden email]> wrote:

Also, I guess JFrame handling should be in EDT.

Regards
Prasanta
On 7/9/2018 6:14 PM, Sergey Bylokhov wrote:
Hi, Manajit.
Did you run the test on other platforms?
I am not sure, but look like the line below can throw npe:
if(!prop1.equals(prop2)) {

On 06/07/2018 16:37, Manajit Halder wrote:
Hi Sergey,

Checked the behaviour of setCanFullscreen() and found the test was failing. The window was "FULLSCREENABLE” when it was not focusable.
Changed the code to fix this issue. After fix the window (JFrame) is not “FULLSCREENABLE” when it is not resizable and non-focusable.

Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.05/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.05/>

Regards,
Manajit

On 04-Jul-2018, at 5:52 PM, Sergey Bylokhov <[hidden email] <[hidden email]>> wrote:

Hi, Manajit.
Can you please recheck the usage of setCanFullscreen() method at line 691. Is it possible to make the window "FULLSCREENABLE" when it is not focusable? I guess it can be checked by something like this:


frame.setFocusableWindowState(false);
frame.setResizable(true);
v1 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
frame.setVisible(false);
frame.setVisible(true);
v2 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
if(!v1.equals(v2)) Error();


On 02/07/2018 20:12, Manajit Halder wrote:
Hi Sergey,
Thanks for your review comment. Code changed as per your suggestion.
Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.04/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.04/>
Regards,
Manajit
On 01-Jul-2018, at 4:45 AM, Sergey Bylokhov <[hidden email] <[hidden email]><[hidden email]>> wrote:

Hi, Manajit
Can you please simplify such new code in the fix:
(!isFocusable || !isResizable) ? false : true

"false: true" may be omitted.

Also please split this new long line:
486 return (target instanceof Frame) ? ((Frame)target).isResizable() : ((target instanceof Dialog) ? ((Dialog)target).isResizable() : false

On 28/06/2018 05:54, Manajit Halder wrote:
Hi Sergey,
Thanks for your review comment. I have modified the fix to incorporate your suggestions.
Please review the modified webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.03/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.03/>
Regards,
Manajit
On 26-Jun-2018, at 4:59 AM, Sergey Bylokhov <[hidden email] <[hidden email]><[hidden email]><[hidden email]>> wrote:

Hi, Manajit.
There is one more inconsistency, I have tested it using the test for teh previous fix: UnfocusableMaximizedFrameResizablity.java

In this case the window is zoomable(the green button is active, but does not work as expected):
       frame.setFocusableWindowState(false);
       frame.setResizable(true);

In this case the window is not zoomable(the green button is inactive):
       frame.setResizable(true);
       frame.setFocusableWindowState(false);

I suggest to update the testcase to cover all this cases which we found during review.

On 21/06/2018 12:26, Manajit Halder wrote:
Hi Phil and Sergey,
I have changed the code as per your suggestions. Now window is resized based on the following condition:
If window is non-focusable OR window is focusable but non-resizable THEN window is made non-resizable.
Otherwise window is made resizable (when window is resizable and focusable).
Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.02/>
Note: Fix was verified by running all the java/awt/ jtreg test cases and by executing the reported JCK interactive test case.
Regards,
Manajit
On 20-Jun-2018, at 6:27 PM, Manajit Halder <[hidden email] <[hidden email]>> wrote:

Hi Phil,

Please find my answer inline to your comment.

On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email] <[hidden email]>> wrote:

I would like to refer back to a comment you made in the previous fix
http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html

> It is not mentioned in the focus spec whether the unfocusable maximized frames should be resizable or not.

Yet there seems to be a JCK test that says it should not be resizable ?

Can you review the spec. again ?
JCK must have based the test on something .. else the test is not valid.

Yes, I checked FocusSpec.html and it doesn’t specify anything about resizable behaviour of non-focusable Frame.

The UnfocusableMaximizedFrameResizablity.java test passes on Window and Linux and fails on Mac OS.
Fix for issue 7158623 was done accordingly to make sure the behaviour is same on all platforms.

If this behaviour is not correct then Window and Linux code should be changed accordingly so that all three platforms behave same.


If we want that behaviour specified .. we should be specifying it ..
But I am not sure if it is actually enforceable on all window managers / desktops.

But I have the same issue with this fix as the previous one. Perhaps not the fix,
but the explanation. The code being changed can't be understood without seeing
the method it calls, and the native method it in turn calls.

Can you provide a detailed explanation as to how this change propagates down
and does the right thing ?

The call flow:

updateFocusableWindowState() calls setStyleBits with style bits to be set on the window.
setStyleBits() calls native method nativeSetNSWindowStyleBits. nativeSetNSWindowStyleBits passes mask and 0 as the second parameter in our case (for non-focusable windows).
Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits in AWTWindow.m generates newBits and applies it on the NSWindow.

My previous fix for issue 7158623 explains bits set on the window.
http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071


BTW stylistically - if this is the right fix - you could do :

setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable) ? RESIZABLE : 0), isFocusable);
Changed the code as per the suggestion. Please review the modified code.
http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/

Regards,
Manajit

-phil.

On 06/14/2018 11:37 PM, Manajit Halder wrote:
Hi All,

Kindly review the fix for JDK11.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8204860

Webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>

Fix:
Frame is focusable:
Retaining the existing frame resizable behaviour (Fixes the current issue).
Frame is non-focusable:
Making the Frame non-resizable (Fix for issue 7158623).

Regards,
Manajit




--
Best regards, Sergey.


--
Best regards, Sergey.


--
Best regards, Sergey.





Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a corner of the frame with the mouse

prasanta sadhukhan

+1

Regards
Prasanta
On 7/10/2018 5:03 PM, Manajit Halder wrote:
Hi Sergey and Prasanta,

Thanks for your comment. The fullscreen test (i.e. case 3 in the test) is made to run only on Mac as the full screen property (“apple.awt.fullscreenable”) is specific to Mac OS.
Please review the updated webrev:

Regards,
Manajit

On 09-Jul-2018, at 6:33 PM, Prasanta Sadhukhan <[hidden email]> wrote:

Also, I guess JFrame handling should be in EDT.

Regards
Prasanta
On 7/9/2018 6:14 PM, Sergey Bylokhov wrote:
Hi, Manajit.
Did you run the test on other platforms?
I am not sure, but look like the line below can throw npe:
if(!prop1.equals(prop2)) {

On 06/07/2018 16:37, Manajit Halder wrote:
Hi Sergey,

Checked the behaviour of setCanFullscreen() and found the test was failing. The window was "FULLSCREENABLE” when it was not focusable.
Changed the code to fix this issue. After fix the window (JFrame) is not “FULLSCREENABLE” when it is not resizable and non-focusable.

Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.05/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.05/>

Regards,
Manajit

On 04-Jul-2018, at 5:52 PM, Sergey Bylokhov <[hidden email] <[hidden email]>> wrote:

Hi, Manajit.
Can you please recheck the usage of setCanFullscreen() method at line 691. Is it possible to make the window "FULLSCREENABLE" when it is not focusable? I guess it can be checked by something like this:


frame.setFocusableWindowState(false);
frame.setResizable(true);
v1 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
frame.setVisible(false);
frame.setVisible(true);
v2 = frame.getRootPane().getClientProperty("apple.awt.fullscreenable");
if(!v1.equals(v2)) Error();


On 02/07/2018 20:12, Manajit Halder wrote:
Hi Sergey,
Thanks for your review comment. Code changed as per your suggestion.
Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.04/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.04/>
Regards,
Manajit
On 01-Jul-2018, at 4:45 AM, Sergey Bylokhov <[hidden email] <[hidden email]><[hidden email]>> wrote:

Hi, Manajit
Can you please simplify such new code in the fix:
(!isFocusable || !isResizable) ? false : true

"false: true" may be omitted.

Also please split this new long line:
486 return (target instanceof Frame) ? ((Frame)target).isResizable() : ((target instanceof Dialog) ? ((Dialog)target).isResizable() : false

On 28/06/2018 05:54, Manajit Halder wrote:
Hi Sergey,
Thanks for your review comment. I have modified the fix to incorporate your suggestions.
Please review the modified webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.03/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.03/>
Regards,
Manajit
On 26-Jun-2018, at 4:59 AM, Sergey Bylokhov <[hidden email] <[hidden email]><[hidden email]><[hidden email]>> wrote:

Hi, Manajit.
There is one more inconsistency, I have tested it using the test for teh previous fix: UnfocusableMaximizedFrameResizablity.java

In this case the window is zoomable(the green button is active, but does not work as expected):
       frame.setFocusableWindowState(false);
       frame.setResizable(true);

In this case the window is not zoomable(the green button is inactive):
       frame.setResizable(true);
       frame.setFocusableWindowState(false);

I suggest to update the testcase to cover all this cases which we found during review.

On 21/06/2018 12:26, Manajit Halder wrote:
Hi Phil and Sergey,
I have changed the code as per your suggestions. Now window is resized based on the following condition:
If window is non-focusable OR window is focusable but non-resizable THEN window is made non-resizable.
Otherwise window is made resizable (when window is resizable and focusable).
Please review the changes:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.02/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.02/>
Note: Fix was verified by running all the java/awt/ jtreg test cases and by executing the reported JCK interactive test case.
Regards,
Manajit
On 20-Jun-2018, at 6:27 PM, Manajit Halder <[hidden email] <[hidden email]>> wrote:

Hi Phil,

Please find my answer inline to your comment.

On 15-Jun-2018, at 11:24 PM, Phil Race <[hidden email] <[hidden email]>> wrote:

I would like to refer back to a comment you made in the previous fix
http://mail.openjdk.java.net/pipermail/awt-dev/2018-February/013626.html

> It is not mentioned in the focus spec whether the unfocusable maximized frames should be resizable or not.

Yet there seems to be a JCK test that says it should not be resizable ?

Can you review the spec. again ?
JCK must have based the test on something .. else the test is not valid.

Yes, I checked FocusSpec.html and it doesn’t specify anything about resizable behaviour of non-focusable Frame.

The UnfocusableMaximizedFrameResizablity.java test passes on Window and Linux and fails on Mac OS.
Fix for issue 7158623 was done accordingly to make sure the behaviour is same on all platforms.

If this behaviour is not correct then Window and Linux code should be changed accordingly so that all three platforms behave same.


If we want that behaviour specified .. we should be specifying it ..
But I am not sure if it is actually enforceable on all window managers / desktops.

But I have the same issue with this fix as the previous one. Perhaps not the fix,
but the explanation. The code being changed can't be understood without seeing
the method it calls, and the native method it in turn calls.

Can you provide a detailed explanation as to how this change propagates down
and does the right thing ?

The call flow:

updateFocusableWindowState() calls setStyleBits with style bits to be set on the window.
setStyleBits() calls native method nativeSetNSWindowStyleBits. nativeSetNSWindowStyleBits passes mask and 0 as the second parameter in our case (for non-focusable windows).
Java_sun_lwawt_macosx_CPlatformWindow_nativeSetNSWindowStyleBits in AWTWindow.m generates newBits and applies it on the NSWindow.

My previous fix for issue 7158623 explains bits set on the window.
http://openjdk.5641.n7.nabble.com/lt-AWT-Dev-gt-Subject-lt-AWT-dev-gt-11-Review-request-for-JDK-7158623-macosx-Should-an-unfocusable-m-td326691.html#a329071


BTW stylistically - if this is the right fix - you could do :

setStyleBits(SHOULD_BECOME_KEY | SHOULD_BECOME_MAIN | ((isFocusable) ? RESIZABLE : 0), isFocusable);
Changed the code as per the suggestion. Please review the modified code.
http://cr.openjdk.java.net/~mhalder/8204860/webrev.01/

Regards,
Manajit

-phil.

On 06/14/2018 11:37 PM, Manajit Halder wrote:
Hi All,

Kindly review the fix for JDK11.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8204860

Webrev:
http://cr.openjdk.java.net/~mhalder/8204860/webrev.00/ <http://cr.openjdk.java.net/%7Emhalder/8204860/webrev.00/>

Fix:
Frame is focusable:
Retaining the existing frame resizable behaviour (Fixes the current issue).
Frame is non-focusable:
Making the Frame non-resizable (Fix for issue 7158623).

Regards,
Manajit




--
Best regards, Sergey.


--
Best regards, Sergey.


--
Best regards, Sergey.