<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

Mon Jul 9 12:44:38 UTC 2018

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 
>> <Sergey.Bylokhov at oracle.com <mailto:Sergey.Bylokhov at oracle.com>> 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 
>>>> <Sergey.Bylokhov at oracle.com 
>>>> <mailto:Sergey.Bylokhov at oracle.com><mailto:Sergey.Bylokhov at oracle.com>> 
>>>> 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 
>>>>>> <sergey.bylokhov at oracle.com 
>>>>>> <mailto:sergey.bylokhov at oracle.com><mailto:sergey.bylokhov at oracle.com><mailto:sergey.bylokhov at oracle.com>> 
>>>>>> 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 
>>>>>>>> <manajit.halder at oracle.com <mailto:manajit.halder at oracle.com>> 
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Hi Phil,
>>>>>>>>
>>>>>>>> Please find my answer inline to your comment.
>>>>>>>>
>>>>>>>>> On 15-Jun-2018, at 11:24 PM, Phil Race <philip.race at oracle.com 
>>>>>>>>> <mailto:philip.race at oracle.com>> 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.