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

[Anthony Petrov]

Hi Sergey,

On 6/10/2012 1:40 AM, Sergey Bylokhov wrote:
>> 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. 

This depends on how well we document our code.

>>> 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()?

Hmmm... No, because insets may be required earlier. Please see below.

>> 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.

OK. Could you please run the tests with your fix and see if they pass?


>>>> 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()?

The size (and other attributes) of a window may change while it's been 
hidden and then gets shown again, which may require this call in 
setVisible(). If you're sure this is OK to not call it in setVisible(), 
then I'm fine with this change.


>>>> 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.

Thanks!

--
best regards,
Anthony

>>
>> -- 
>> 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/
>>>>>
>>>
>>>
> 
>