<AWT Dev> [9] Review request for 8032078: CPlatformWindow.setWindowState throws RuntimeException, if windowState=ICONIFIED|MAXIMIZED_BOTH

Sergey Bylokhov Sergey.Bylokhov at oracle.com
Fri Feb 21 05:47:10 PST 2014


Hello, Anton.
The fix looks good.
On 19.02.2014 12:39, Anton Litvinov wrote:
> Hello Sergey,
>
> Thank you for the review of the corrected version of the fix. It is a 
> good idea to verify workability of "setVisible" method, but I think 
> that "setVisible" method is not related to the original bug and we 
> should not include testing of this method into the fix. Also for me it 
> is not clear, what exactly should be tested in "setVisible" method 
> test, because:
>
> - "Frame.state" field is changed immediately during the call 
> "Frame.setExtendedState" for both visible and invisible frames.
> - According to the documentation of "Frame.setExtendedState" method, 
> when "setVisible(true)" is called, the frame will only attempt to 
> apply the state and reception of any 
> "WindowEvent.WINDOW_STATE_CHANGED" events is not guaranteed.
>
> I suggest to create a separate bug on creation of the test for 
> verification of "setVisible" method, if it is necessary to verify this 
> method and, if no analogous regression tests exist currently.
>
> Thank you,
> Anton
>
> On 2/18/2014 6:35 PM, Sergey Bylokhov wrote:
>> Hi, Anton.
>> The fix looks good to me. But since you change setVisible() method 
>> you need to check it in the test too.
>>
>> On 18.02.2014 18:25, Anton Litvinov wrote:
>>> Hello Sergey,
>>>
>>> Thank you for the review of this fix. The fix was corrected to 
>>> address your remarks. Could you please review the new version of the 
>>> fix.
>>>
>>> Webrev: http://cr.openjdk.java.net/~alitvinov/8032078/jdk9/webrev.01
>>>
>>> Changes in the fix are following:
>>>
>>> 1. Check for a compound state with ICONIFIED bit was moved from 
>>> "switch" block of "CPlatformWindow.setWindowState" method to the 
>>> separate "if" statement before "switch" block.
>>> 2. The same pattern was applied also to "CPlatformWindow.setVisible" 
>>> method.
>>> 3. The comment was changed.
>>> 4. The regression test was changed to also check cases with 
>>> undecorated frame and to verify that changes between the states 
>>> "ICONIFIED | MAXIMIZED_BOTH <--> NORMAL, ICONIFIED, MAXIMIZED_BOTH" 
>>> are committed properly and do not raise exceptions.
>>>
>>> Thank you,
>>> Anton
>>> On 2/17/2014 5:17 PM, Sergey Bylokhov wrote:
>>>> Hi, Anton.
>>>> I suppose that CPW should have the same behavior on setVisible() 
>>>> and setExtendedState() for compound states?
>>>> Also I suggest to change comment to describe that on macosx we 
>>>> support ICONIFY state only for compounds states, currently the 
>>>> comment is quite obvious. Probably the code could be improved and 
>>>> we can change windowState to ICONIFY before the switch for compound 
>>>> states?
>>>>
>>>> On 24.01.2014 17:43, Anton Litvinov wrote:
>>>>> Hello Petr,
>>>>>
>>>>> Thank you very much for review of this fix. I am not sure that the 
>>>>> behavior of "ICONIFIED | MAXIMIZED_BOTH" should be completely the 
>>>>> same as "ICONIFIED" on OS X, but I think that in this bitwise mask 
>>>>> the most important bit is "ICONIFIED". And, if this compound state 
>>>>> is valid, then as a result the window should be minimized.
>>>>>
>>>>> This combination of flags is handled absolutely differently in 
>>>>> Windows, Solaris/Linux implementation of JDK. Particularly on 
>>>>> Windows the frame becomes minimized and will be always maximized, 
>>>>> if the frame's icon on the taskbar is clicked. But 
>>>>> "Frame.getExtendedState", "WFramePeer.getState()" will not always 
>>>>> return "ICONIFIED | MAXIMIZED_BOTH", instead of this "ICONIFIED" 
>>>>> will be returned, when before a call to "Frame.setExtendedState" 
>>>>> method "Frame.state" was not "MAXIMIZED_BOTH".
>>>>>
>>>>> The common feature of Windows and Solaris/Linux code is the fact 
>>>>> that the implementation of "java.awt.peer.FramePeer.setState" does 
>>>>> not throw an exception unlike OS X code.
>>>>>
>>>>> Yes, this bug may be fixed in "Frame.isFrameStateSupported", but 
>>>>> the change will require introduction of code checking, either it 
>>>>> is executed on OS X platform or not, because this method should 
>>>>> not be changed for other platforms, since they depend on it for a 
>>>>> long time since 2004, when it was introduced by the fix for the 
>>>>> bug JDK-4987087. From my opinion, making 
>>>>> "Frame.isFrameStateSupported" return false for this compound state 
>>>>> on OS X platform will make the code of this method not truly 
>>>>> shared and will not let code calling "Frame.setExtendedState" with 
>>>>> the state "ICONIFIED | MAXIMIZED_BOTH" to minimize the frame.
>>>>>
>>>>> Thank you,
>>>>> Anton
>>>>>
>>>>> On 1/21/2014 4:26 PM, Petr Pchelko wrote:
>>>>>> Hello, Anton.
>>>>>>
>>>>>> Are we sure that the behavior of ICONIFIED | MAXIMIZED_BOTH 
>>>>>> should be the same as just ICONIFIED?
>>>>>> What does this combination of flags do on other platforms?
>>>>>>
>>>>>> I would expect that if the frame is not maximized, this 
>>>>>> combination would iconify it, but when you deiconify the frame
>>>>>> should go to maximized state. However, I’m quite sure you can’t 
>>>>>> do it like this on Mac OS X, because we use the native
>>>>>> zoom mechanism for maximization and do not have enough control 
>>>>>> over it. So, in my opinion this should be fixed by
>>>>>> making Frame.isFrameStateSupported return false for this 
>>>>>> combination. What do you think?
>>>>>>
>>>>>> With best regards. Petr.
>>>>>>
>>>>>> 21 ÿíâ. 2014 ã., â 3:27 ïîñëå ïîëóäíÿ, Anton 
>>>>>> Litvinov<anton.litvinov at oracle.com>  íàïèñàë(à):
>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Could you please review the following fix for the bug.
>>>>>>>
>>>>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8032078
>>>>>>> Webrev:http://cr.openjdk.java.net/~alitvinov/8032078/jdk9/webrev.00
>>>>>>>
>>>>>>> The bug consists in undocumented throwing of "RuntimeException" 
>>>>>>> from the method "Frame.setExtendedState" for the compound state 
>>>>>>> "ICONIFIED | MAXIMIZED_BOTH" that is supported according to a 
>>>>>>> result of "Frame.isFrameStateSupported" method call on OS X.
>>>>>>>
>>>>>>> The solution adds handling of the mask "ICONIFIED | 
>>>>>>> MAXIMIZED_BOTH" to "switch" block of the method 
>>>>>>> "sun.lwawt.macosx.CPlatformWindow.setWindowState" which 
>>>>>>> duplicates existing handling of the state "ICONIFIED" and 
>>>>>>> prevents from throwing of the exception.
>>>>>>>
>>>>>>> Thank you,
>>>>>>> Anton
>>>>>
>>>>
>>>>
>>>
>>
>>
>


-- 
Best regards, Sergey.



More information about the awt-dev mailing list