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

[Sergey Bylokhov]

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
>>>>
>>
>>


-- 
Best regards, Sergey.