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
