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

[Anton Litvinov]

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
>>>>
>>>
>>>
>>
>
>