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

[Anthony Petrov]

Hi Sergey,

Thanks for the comments and the updated fix. It looks great.

--
best regards,
Anthony

On 1/19/2013 17:32, 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
>>>>>
>>>
>>>
> 
>