Re: [8] Review request for 7177173: [macosx] JFrame.setExtendedState(JFrame.MAXIMIZED_BOTH) not working as expected in JDK 7
On 7/5/2012 6:08 PM, Anthony Petrov wrote:
> 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.
That's much clearer! Thanks, it now looks fine.
Artem
> --
> 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.
>>>>
>>>>
RSS Feed