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

[Anthony Petrov]

BTW, should we change the URL at which a user should file a bug?

--
best regards,
Anthony

On 1/23/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
>>>>>>
>>>>
>>>>
>>
>>
>