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.
