Hi Anthony.
Thanks for review. See comments inline.
05.06.2012 19:47, Anthony Petrov wrote:
Hi Sergey,
1. src/macosx/classes/sun/lwawt/LWComponentPeer.java
196 delegate = createDelegate();
197 if (delegate != null) {
198 delegate.setVisible(false);
The call at line 198 looks unnatural here. It looks as if the delegate
is created visible initially which isn't true, is it?
Delegate is visible by default, but our awt peer is not.
2.
204 delegate.setOpaque(true);
Related to the above comment, does this call mean that a delegate is
created non-opaque initially? I feel uncomfortable with these calls.
Do we workaround something with these calls? Can we give them more
appropriate and meaningful names then?
Most of aqua components are non opaque by default, but our awt peer not.
3. src/macosx/classes/sun/lwawt/LWWindowPeer.java
179 updateInsets(platformWindow.getInsets());
The system may report wrong insets before a window is shown on the
screen. Perhaps, instead of initializeImpl() we should introduce
preInitialize() (== current initializeImpl()), and postInitialize()
(to where this, and the subsequent replaceSurfaceData() calls might go.)
Like it was done in XAWT? It is just a nightmare. I assume that
updateInsets() should be called by some of the native callbacks, like
notifyExpose and reshape?
4.
220 protected void setVisibleImpl(final boolean visible) {
221 super.setVisibleImpl(visible);
Why do we remove a call to replaceSurfaceData() in the beginning of
the method?
Before the fix, setVisible() can be called before surface creation, but
after the fix it will be called after.
5.
983 ((Graphics2D)
g).setBackground(getBackground());
I suggest to add an instanceof check before this call.
I guess that Buffered image cannot return something except Graphics2D
6.
227 // invokeLater() can be deleted, but in this case we get
a lag between
228 // windows showing and content painting.
Is the lag so very noticeable? On other platforms we don't actually do
this, and I don't recall any issues about such lags.
Yes The difference is noticeable. So I update the comments and leave it
as is for now.
--
best regards,
Anthony
On 5/31/2012 5:43 PM, Sergey Bylokhov wrote:
Hi Everyone,
Please review the fix.
Notes from the bug and comments:
1. setVisible() should be called at the end of the peers
initialization. We can move super.initialize() to the end of the
peers initializations.
Initialize() was split to initialize() and initializeImpl(). In the
initialize() we call initializeImpl and then we call to setVisible().
initializeImpl overridden in subclasses.
2. Invokelater in the initialization/disposing is a tricky.
Left it as is. Probably later it will be changed. Comments was updated.
3. replaceSurfacedata() should be moved outside of
LWWindowPeer.setVisible()
Done. Also duplicate code was extracted to setVisible() method which
call setVisibleImpl().
4. Backbuffer in replaceSurfacedata() should be initialized by
clearRect instead of fillrect(composite is important).
Done. related to composite.
5. During lwwindowpeer initialization we call two similar methods
nativeSetNSWindowAlpha() and setAlphaValue().
nativeSetNSWindowAlpha() removed from CPlatformWindow.java.
Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7142091
Webrev can be found at:
http://cr.openjdk.java.net/~serb/7142091/webrev.00/
--
Best regards, Sergey.
