<AWT Dev> [8] Request for review: 8003173 [macosx] Fullscreen on Mac leaves an empty rectangle

Alexander Scherbatiy alexandr.scherbatiy at oracle.com
Thu Jan 24 03:02:51 PST 2013

    The fix looks good for me.

On 1/23/2013 6:00 PM, Sergey Bylokhov wrote:
> Hi, Alexander.
> You workspace is not up to date. Pls update it.
           Yes. After updating the TCK test does not show the warning.


> 23.01.2013 17:56, Alexander Scherbatiy wrote:
>>    The api/java_awt/GraphicsDevice/indexTGF.html#SetDisplayMode TCK 
>> test shows the following warning after the fix:
>> 2013-01-23 17:54:27.027 java[4695:707] Cocoa AWT: Running on AppKit 
>> thread 0 when not expected. (
>>         0   libawt_lwawt.dylib                  0x000000011e662263 
>> Java_sun_lwawt_macosx_CPlatformWindow_nativeGetNSWindowInsets + 72
>>         1   ???                                 0x0000000110df9ce9 
>> 0x0 + 4578057449
>> )
>> 2013-01-23 17:54:27.053 java[4695:707]  Please file a bug report at 
>> http://java.net/jira/browse/MACOSX_PORT with this message and a 
>> reproducible test case.
>>    Thanks,
>>    Alexandr.
>> On 1/19/2013 5:32 PM, Sergey Bylokhov wrote:
>>> Hi, Anthony.
>>> Please review the new version of the fix:
>>> http://cr.openjdk.java.net/~serb/8003173/webrev.01
>>> Now only CPWindow is responsible for all move/resize/newinsets 
>>> notification, all related code was removed from CPView. 
>>> isFullScreenAnimationOn was added to make fullscreen animation smooth.
>>> See inline comments.
>>> 15.01.2013 20:47, Anthony Petrov wrote:
>>>> On 1/9/2013 19:57, Sergey Bylokhov wrote:
>>>>> 09.01.2013 18:40, Anthony Petrov wrote:
>>>>>> src/macosx/classes/sun/lwawt/LWWindowPeer.java:
>>>>>> Note that theoretically the insets can be changed w/o changing 
>>>>>> the content size. For example, if a user switches to a theme with 
>>>>>> enlarged window decorations. Not sure if this applies to Mac 
>>>>>> presently, but in theory this is possible. Will sending the 
>>>>>> COMPONENT_RESIZE event be equivalent to calling the 
>>>>>> replaceSurfaceData() in this case? Also, since the event is only 
>>>>>> posted but not processed yet, what is the point to call 
>>>>>> repaintPeer() before the surface data is replaced?
>>>>> In general surfaceData should include top level size of the 
>>>>> window(including insets) So it's not necessary to replace surface 
>>>>> here. Because there is no api for target notification about new 
>>>>> insets I use COMPONENT_RESIZE event.
>>>> Thanks for clarifying that.
>>>>>> src/macosx/classes/sun/lwawt/macosx/CPlatformView.java:
>>>>>> The actual fix seems to reside in this file. Why doesn't 
>>>>>> peer.getInsets() return zeros in the full screen mode? If it does 
>>>>>> actually, why do we need this change then? A generic, 
>>>>>> insets-accounting size calculation seems to be preferable in case 
>>>>>> we need a non-zero insets for some specific use-cases in the future.
>>>>> When we set NSView to full screen the native insets(as we 
>>>>> calculate it) does not change. Because of that we use synthetic 
>>>>> resize notifications in enterFullScreenMode() we just should not 
>>>>> include insets in this case.
>>>> At line 98 of CPlatformView.java in "peer.getInsets()", the peer is 
>>>> an LWWindowPeer instance associated with the view. In 
>>>> CPlatformWindow.enterFullScreenMode() you already call 
>>>> "peer.updateInsets(new Insets(0, 0, 0, 0))" which should zero out 
>>>> the insets stored in the same peer. So the peer.getInsets() call in 
>>>> CPlatformView will return these zeros, and hence they shouldn't 
>>>> affect the calculations you perform at lines 111-115 in CPlatformView.
>>> But why we need this calculation there? I guess CPlatformWindow is a 
>>> better place for insets calculation, otherwise in the 
>>> CPlatformView.exitFullScreenMode() I'll get something like:
>>> peer.updateInsets(peer.getPlatformWindow().getInsets());
>>> Which looks strange.
>>> In the new version of the fix LWWindowPeer fetch this information 
>>> when necessary, and getPlatformWindow().getInsets() return correct 
>>> value.
>>>> So I repeat my question, why do we need to change anything in 
>>>> CPlatformView then? Can we just revert the changes?
>>> Better to have related stuff in one class.
>>>>>> src/macosx/classes/sun/lwawt/macosx/CPlatformWindow.java:
>>>>>>>  876 peer.updateInsets(getInsets());
>>>>>> This will call a native method upon sending every move/resize 
>>>>>> event. Why do we actually have to do this? I assume the peer 
>>>>>> already calls the PlatformWindow.getInsets() whenever needed.
>>>>> No. It does not call, that's the problem.
>>>>>> Also, AFAIK, insets rarely change on Mac, and you already handle 
>>>>>> their manual changes when entering/exiting the full screen mode. 
>>>>>> Can we just remove this line?
>>>>> No. There are two different fullscreens.
>>>>> 1 The full screen based on Nsview, which is used via Graphics 
>>>>> device. For this we emulate insets and events.
>>>>> 2 The full screen in lion style. We can handle it in 
>>>>> windowWillEnterFullScreen/windowDidEnterFullScreen. In the 
>>>>> WillEnterXX window have old insets and in the DidEnterXX window 
>>>>> have new insets. DidEnterXX comes after Resize events, which will 
>>>>> repaint the window ==> we get content's jumping.
>>>> So, why not simply call "peer.updateInsets(new Insets(0, 0, 0, 0))" 
>>>> from windowWillEnter... then? 
>>> It is not necessary to make them empty, because native system 
>>> changes decoration and insets for us. But this happens after 
>>> willEnter and before didEnter.
>>>> Otherwise this looks inconsistent because in the case of the 
>>>> GraphicsDevice-based full screen mode you zero out the insets 
>>>> manually, but they remain being non-zero in the native full screen 
>>>> mode.
>>> In both cases insets will be empty, but in GraphicsDevice-based full 
>>> screen mode we emulate insets/notification manually.
>>>> -- 
>>>> best regards,
>>>> Anthony
>>>>>> src/macosx/native/sun/awt/AWTWindow.m
>>>>>>>  821     [ThreadUtilities performOnMainThreadWaiting:YES block:^(){
>>>>>>>  822         AWT_ASSERT_APPKIT_THREAD;
>>>>>> This ASSERT statement may safely be removed since the 
>>>>>> ThreadUtilities already guarantee us that we're running on the 
>>>>>> main thread.
>>>>> I just use standard template from AWTWindow.m, so it will be 
>>>>> simple to change this template at once.
>>>>>> -- 
>>>>>> best regards,
>>>>>> Anthony
>>>>>> On 1/9/2013 17:32, Sergey Bylokhov wrote:
>>>>>>> Hello,
>>>>>>> Please review the fix.
>>>>>>> The reason why we have an empty space in the full screen mode is 
>>>>>>> that we did not update our insets.
>>>>>>>  - I update insets in the deliverMoveResizeEvent not in the 
>>>>>>> windowDidEnterFullScreen, in this case animation became more 
>>>>>>> smooth(since we update insets just before paint action). So all 
>>>>>>> old fullscreen handle methods in CPlatformWindow were removed.
>>>>>>>  - LWWindowPeer.updateInsets will post ComponentEvent if insets 
>>>>>>> were changed.
>>>>>>>  - CGraphicsDevice.setDisplayMode now stores/restores full 
>>>>>>> screen window, because otherwise NSView looks shifted on screen.
>>>>>>>  - CPlatformView.enterFullScreenMode(): code related to insets 
>>>>>>> was removed, since NSVIew uses the whole screen unlike jdk 
>>>>>>> 6.(see related changes in CPlatformWindow.enter/exitFullScreenMode)
>>>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8003173
>>>>>>> Webrev can be found at: 
>>>>>>> http://cr.openjdk.java.net/~serb/8003173/webrev.00

More information about the awt-dev mailing list