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

[Sergey Bylokhov]

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.