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: http://cr.openjdk.java.net/~mhalder/8204860/webrev.06/ <http://cr.openjdk.java.net/~mhalder/8204860/webrev.06/>
Regards, Manajit > On 09-Jul-2018, at 6:33 PM, Prasanta Sadhukhan > <[email protected]> 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 <[email protected] >>>> <mailto:[email protected]>> 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 <[email protected] >>>>>> <mailto:[email protected]><mailto:[email protected]>> >>>>>> 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 >>>>>>>> <[email protected] >>>>>>>> <mailto:[email protected]><mailto:[email protected]><mailto:[email protected]>> >>>>>>>> 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 >>>>>>>>>> <[email protected] <mailto:[email protected]>> wrote: >>>>>>>>>> >>>>>>>>>> Hi Phil, >>>>>>>>>> >>>>>>>>>> Please find my answer inline to your comment. >>>>>>>>>> >>>>>>>>>>> On 15-Jun-2018, at 11:24 PM, Phil Race <[email protected] >>>>>>>>>>> <mailto:[email protected]>> 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. >>> >> >> >
