Re: AWT Dev [8] Review request for 7177173: [macosx] JFrame.setExtendedState(JFrame.MAXIMIZED_BOTH) not working as expected in JDK 7
Hi, Anthony. Fix looks good. 04.07.2012 19:30, 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. -- Best regards, Sergey.
Re: AWT Dev [8] Review request for 7177173: [macosx] JFrame.setExtendedState(JFrame.MAXIMIZED_BOTH) not working as expected in JDK 7
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.
Re: AWT Dev [8] Review request for 7177173: [macosx] JFrame.setExtendedState(JFrame.MAXIMIZED_BOTH) not working as expected in JDK 7
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.
Re: AWT Dev [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.
AWT Dev [8] Review request for 7177173: [macosx] JFrame.setExtendedState(JFrame.MAXIMIZED_BOTH) not working as expected in JDK 7
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
Re: AWT Dev [8] Review request for 7177173: [macosx] JFrame.setExtendedState(JFrame.MAXIMIZED_BOTH) not working as expected in JDK 7
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.
Re: AWT Dev [8] Review request for 7177173: [macosx] JFrame.setExtendedState(JFrame.MAXIMIZED_BOTH) not working as expected in JDK 7
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. -- Best regards, Sergey.
Re: AWT Dev [8] Review request for 7177173: [macosx] JFrame.setExtendedState(JFrame.MAXIMIZED_BOTH) not working as expected in JDK 7
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.