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

[Anton V. Tarasov]

Hi Anthony,

On 22.07.2013 13:25, Anthony Petrov wrote:
> 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 the app follows the "access/modify swing components on EDT only" rule, then it is not possible, 
as all that stuff is executed on EDT.

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

So, we shouldn't.

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

Unfortunately, the listeners are not (at least for a number of standard swing components) called 
initially... The pref/min/max sizes are (or, may be) component's initial properties, it's not a 
layout manager which sets them. Even if they're calculated internally, corresponding set*Size() 
methods are not (or, may not be) called. So, we have to forward the initial values to fx and only 
then rely on the listeners.

Thanks,
Anton.

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