[8] Review request for 7177173: [macosx] JFrame.setExtendedState(JFrame.MAXIMIZED_BOTH) not working as expected in JDK 7

[Anthony Petrov]

Hi Artem,

Thanks for the review. Please find an updated webrev at:

http://cr.openjdk.java.net/~anthony/8-36-MaximizedBoth-7177173.3/

All suggestions have been applied.

--
best regards,
Anthony

On 07/05/12 14:46, Artem Ananiev wrote:
>
> That's not the clearest code I've ever seen... Is it possible to
> refactor it to make easier to review and support? For example, replace
> zoom() with two separate methods, maximize() and restore(), or zoom()
> and unzoom(), or whatever. Each of those two methods would be no-op, if
> the window is already in the required state.
>
> isZoomed() method is not that trivial as well. In each of the two
> methods, I would have isUndecorated() condition, and based on the
> result, check for "isZoomed" or "normalBounds" field... Something like
> that.
>
> I would also enhance the test to include both decorated and undecorated
> frames.
>
> Thanks,
>
> Artem
>
> On 7/4/2012 7:30 PM, Anthony Petrov wrote:
>> Hi Sergey,
>>
>> Please find an updated webrev at:
>>
>> http://cr.openjdk.java.net/~anthony/8-36-MaximizedBoth-7177173.2/
>>
>> I renamed the local variable from isZoomed to doZoom.
>>
>> --
>> best regards,
>> Anthony
>>
>> On 7/4/2012 7:26 PM, Sergey Bylokhov wrote:
>>> Hi,Anthony.
>>> I just point to the same name "isZoomed", which contains different
>>> state:
>>> first is "true" for this.normalBounds == null and the second "true"
>>> for this.normalBounds != null.
>>> This is strange.
>>> I believe that correct code should be isZoomed = isZoomed();?
>>>
>>> 04.07.2012 19:15, Anthony Petrov wrote:
>>>> Hi Sergey,
>>>>
>>>> Thanks for the review.
>>>>
>>>> On 7/4/2012 7:07 PM, Sergey Bylokhov wrote:
>>>>> Why there are two different methods for isZoomed?
>>>>>
>>>>> 481 final boolean isZoomed = *this.normalBounds == null*;
>>>>
>>>> Here we determine a new state that needs to be set to an undecorated
>>>> window. This is what isZoomed() will return after the zoom() method
>>>> returns.
>>>>
>>>>> But in the method we use another code.
>>>>>
>>>>> 471 private boolean isZoomed() {
>>>>> 472 return zoomed || *this.normalBounds != null*;
>>>>> 473 }
>>>>
>>>> This is a method that returns the current zoomed state for any window
>>>> regardless whether it is decorated or not.
>>>>
>>>>> Probably it can be unified?
>>>>
>>>> No. These are completely different operations. We might change the
>>>> first line to "final boolean isZoomed = !isZoomed();", but the
>>>> current code looks a lot clearer to me.
>>>>
>>>> --
>>>> best regards,
>>>> Anthony
>>>>
>>>>
>>>>>
>>>>>
>>>>> 04.07.2012 18:54, Anthony Petrov wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review a fix for
>>>>>> http://bugs.sun.com/view_bug.do?bug_id=7177173 at:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~anthony/8-36-MaximizedBoth-7177173.1/
>>>>>>
>>>>>> The bug synopsis may sound misleading: the original problem with
>>>>>> applying the extended state after showing a frame has been resolved
>>>>>> a while back with another fix. The only issue currently left is
>>>>>> that the state is reset when a frame is disposed/hidden, which
>>>>>> prevents apps from tracking the last known state of a window.
>>>>>>
>>>>>> With this fix we avoid canceling/reapplying the maximized state to
>>>>>> windows on each showing/hiding operation. Instead we track the
>>>>>> state in the CPlatformWindow instance, and apply it on showing of
>>>>>> the window if the state differs from what shared code expects. The
>>>>>> behavior now matches that of Apple JDK (including handling of the
>>>>>> ICONIFIED state).
>>>>>>
>>>>>> --
>>>>>> best regards,
>>>>>> Anthony
>>>>>
>>>>>
>>>>> --
>>>>> Best regards, Sergey.
>>>
>>>