<AWT Dev> [8] Review request for 7142091: [macosx] RFE: Refactoring of peer initialization/disposing

[Sergey Bylokhov]

>
> 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.)
>
In this case In current implementation it works wrong too, because it 
can be called when the window initially invisible.

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?
>
> 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.)
>
> 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?
>
> 5.
>>  983                     ((Graphics2D) 
>> g).setBackground(getBackground());
>
> I suggest to add an instanceof check before this call.
>
> 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.
>
> -- 
> 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.