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

[Anton V. Tarasov]

On 22.07.2013 18:41, Anthony Petrov wrote:
> On 07/22/2013 05:27 PM, Anton V. Tarasov wrote:
>> On 22.07.2013 13:25, Anthony Petrov wrote:
>>> 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.
>
> As an atomic operation in a single runnable posted from FX code, you mean?

Right, resetting the layout bounds and executing the runnable from setContentImpl are synchronized 
by a thread.

> If that is the case, then I'm fine with the current fix. Thanks again for the clarifications.

Thanks for the careful review!

Anton.

>
> -- 
> best regards,
> Anthony
>
>>
>>> 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.
>>>>>>
>>>>
>>