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/~mhalder/8204860/webrev.04/>
Regards, Manajit > On 01-Jul-2018, at 4:45 AM, Sergey Bylokhov <[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/~mhalder/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/ >>>> 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.
