« Return to Thread:
Re: <AWT Dev> [8] Review request for 7142091: [macosx] RFE: Refactoring of peer initialization/disposing
by Sergey Bylokhov-3
::
Rate this Message:
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.
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?
>
> 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.
« Return to Thread:
| Free embeddable forum powered by Nabble | Forum Help |
