[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.
Thanks,
Alexandr.
>
>
> 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 macosx-port-dev
mailing list