Hi Anthony.
See my comments inline:
09.06.2012 17:34, Anthony Petrov wrote:
Hi Sergey,
Thanks for clarifications. Please find my comments below.
On 6/5/2012 8:13 PM, Sergey Bylokhov wrote:
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
What kind of nightmare do you mean? To me it looks logical to perform
some tasks before, and some other tasks after disaplying a component.
Hence the need for per- and post- initializers.
Because in general nobody know correct place for initialization.
Moreover we can do something after component showing in setVisble(true)
only.
updateInsets() should be called by some of the native callbacks, like
notifyExpose and reshape?
Nope. It's only called from the initialize() method once since on the
Mac insets never change. Therefore, it is critical to call it only
after showing the window on the screen.
But what happens when the peer is invisible by default? Probably the
better place for updateInsets is setVisible()?
On 6/5/2012 10:17 PM, Sergey Bylokhov wrote:
In this case In current implementation it works wrong too, because it
can be called when the window initially invisible.
So far, it works fine. Please run insets-related tests (simply all
automatic tests for Window, Frame, and Dialog both open and closed) to
makes sure they pass.
See previous comment.
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.
Could you clarify this further please? What exactly is the reason to
move the replaceSurfaceData() call from setVisible() to initializeImpl()?
Just because it is unnecessary in setVisble() because surface already
created at the end of LWWindow.initializeImpl(). Why we should try to
replace surface each time in setVisible()?
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
I guess so too. Nevertheless, I still suggest to add such a check.
ok I will add.
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.
Interesting. This needs to be investigated, perhaps under a separate CR.
I will create new CR.
--
best regards,
Anthony
--
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.
