<AWT Dev> [8] Request for review: JDK-8020927 JLightweightFrame API should export layout properties change notifications

[Anthony Petrov]

Hi Anton,

Thanks for the clarification. Yes, I agree that there are many cases to 
consider wrt. layout. What concerns me though, is a period of time 
between a call to setContent() and a moment the componentAdded() handler 
is invoked. Is it possible that the component's pref/min/max sizes 
change during this period? If so,

1) Should we call the content.*SizeChanged() methods again after adding 
the listeners in the componentAdded() handler?, and

2) Do we really have to call the *SizeChanged() methods in the 
setContent() method? Is the pref/min/max sizes information really needed 
during the period between setContent() and componentAdded() calls?

Both these questions directly relate to the JDK part of the fix.

--
best regards,
Anthony

On 08/25/2013 07:29 PM, Anton V. Tarasov wrote:
> On 19.07.2013 19:05, Anthony Petrov wrote:
>> The updated fix looks fine. Thanks.
>>
>> I'd only suggest to test whether the initial content.*SizeChanged()
>> calls in the setContent() are enough, or they should be
>> moved/duplicated somewhere else in case creating and showing of a
>> SwingNode instance is stretched in time, so as to make sure the FX
>> code always operates with valid values when the SwingNode gets added
>> to a live scene or gets modified while still being inserted into the
>> scene.
>
> What you're talking about relates to the javafx part of the fix (which
> I'll submit for review separately via the javafx review tool, and I'll
> add you as a reviewer =).
> Just to answer your question. The layout data is stored in SwingNode.
> When it's added to scene after the content has been set on it, it will
> return valid values via its pref/max/minWidth/Heigh() methods which its
> containing layout pane will ask for while laying it out as a newly added
> node. If a content is set/replaced on already visible SwingNode, it will
> ask for relayout via calling impl_notifyLayoutBoundsChanged(). If you're
> suggesting to test it, let me leave that to SQE as they have "layout" in
> their SwingNode test plan. Because layout means quite a lot testcases...
>
> Thanks,
> Anton.
>
>>
>> --
>> best regards,
>> Anthony
>>
>> On 08/25/2013 06:53 PM, Anton V. Tarasov wrote:
>>> Hi Anthony,
>>>
>>> Thanks for the review.
>>>
>>> On 19.07.2013 17:43, Anthony Petrov wrote:
>>>> Hi Anton,
>>>>
>>>> The fix looks good overall.
>>>>
>>>> I'm just not sure about the exact symmetry between the setContent()
>>>> method and the componentRemoved() listener wrt. the layoutSizeListener
>>>> adding/removing. In theory, the setContent() may be called several
>>>> times with different (or same) content objects. Similarly, the
>>>> contentPane may in theory be added or removed manually several times
>>>> (or even transferred between different JLFs).
>>>
>>> setContent() is the API method which we don't control (and which indeed,
>>> in theory, may be called several times with any content).
>>> But the content pane is what we manage internally. It's tight to the JLF
>>> instance and follows its life cycle. (Yes, a developer may hack it, but
>>> this is strongly discouraged =)
>>>
>>>>
>>>> Perhaps both adding and removing the layoutSizeListener should happen
>>>> in componentAdded() and componentRemoved() correspondingly?
>>>
>>> I like the idea. Here's the new version:
>>>
>>> http://cr.openjdk.java.net/~ant/JDK-8020927/webrev.1
>>>
>>> Thanks,
>>> Anton.
>>>
>>>>
>>>> --
>>>> best regards,
>>>> Anthony
>>>>
>>>> On 07/19/2013 04:27 PM, Anton V. Tarasov wrote:
>>>>> Please, review a fix.
>>>>>
>>>>> jira: https://jbs.oracle.com/bugs/browse/JDK-8020927
>>>>> webrev: http://cr.openjdk.java.net/~ant/JDK-8020927/webrev.0
>>>>>
>>>>> Layout bounds notifications are added to internal JLightweightFrame
>>>>> API.
>>>>>
>>>>> (Just FYI, related fx changes are here:
>>>>> http://cr.openjdk.java.net/~ant/RT-30650/webrev.0)
>>>>>
>>>>> Thanks,
>>>>> Anton.
>>>
>