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

[Anthony Petrov]

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? If that is the case, then I'm fine with the current fix. Thanks 
again for the clarifications.

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