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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-awt-dev
Subject:    Re: <AWT Dev> <AWT dev>[11] Review request for JDK-8204860: The frame could be resized by dragging a
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2018-06-30 23:15:00
Message-ID: 0d5eda8c-45ea-84aa-4e87-7870b0403d0d () oracle ! com
[Download RAW message or body]

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 
> > <sergey.bylokhov@oracle.com <mailto:sergey.bylokhov@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/
> > > 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@oracle.com <mailto:manajit.halder@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@oracle.com 
> > > > > <mailto:philip.race@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.


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic