<div dir="ltr">I tried to change the behavior of setting a graphic for Labeled to one that removes the graphics from its previous labeled (if exists). This resulted in test failures in a few places:<div><br></div><div>* MenuButton:</div><div> * MenuButtonTest::testSetContentDisplayGraphicOnly - deliberately sets the same graphic to two MenuButtons. Doesn't look correct.</div><div> * LabeledImpl.Shuttler::invalidated - used by MenuButtonSkinBase, sets the graphics of the Labeled to that of its own Label. That is, it doesn't use the MenuButton's own Label, but creates a different one to use as a child and copies the properties to it. The test that fails is</div>LabeledImplOtherTest. test_RT_21357 [1]. Also seems like an odd implementation choice.<div><br></div><div>Also note that other controls that are not a Labeled have graphic properties, like Tab (as mstr mentioned), so this change doesn't solve all the cases, only those within Labeled.</div><div><br></div><div>[1] <a href="https://bugs.openjdk.org/browse/JDK-8119175" target="_blank">https://bugs.openjdk.org/browse/JDK-8119175</a></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 12, 2022 at 6:10 PM Nir Lisker <<a href="mailto:nlisker@gmail.com" target="_blank">nlisker@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Another idea is to use a static map that maps a node to its "graphic parent". That will save on memory at the expense of a lookup.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Dec 1, 2022 at 11:53 PM Nir Lisker <<a href="mailto:nlisker@gmail.com" target="_blank">nlisker@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Are we convinced that a node can't be both a graphic and a clip, or something else? The docs for clip specify that the node is not a child in the scenegraph.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Dec 1, 2022 at 11:41 PM John Hendrikx <<a href="mailto:john.hendrikx@gmail.com" target="_blank">john.hendrikx@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p>Adding another field doesn't seem ideal, would it be possible to
generalize the clipParent field to function for both (the ownedBy
or owner field that I suggested earlier)?</p>
<p>--John<br>
</p>
<div>On 01/12/2022 20:26, Nir Lisker wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Michael's idea could solve the problem if it's
about more than just traversing, it needs to set rules for
allowing a node to serve only 1 logical role (or 1 role type,
like clip and graphic?) at the same time. In any case, these
rules need to be decided upon before starting to work on
anything. I can do a quick fix for now that can be reverted
later if needed. From what I gather, I will need to add a
graphicsParent field like clipParent does.<br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Dec 1, 2022 at 8:47 PM
Nir Lisker <<a href="mailto:nlisker@gmail.com" target="_blank">nlisker@gmail.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">By the way, these issues are caused by this
inconsistent behavior (they are probably duplicates):
<div><br>
</div>
<div><a href="https://bugs.openjdk.org/browse/JDK-8209017" target="_blank">https://bugs.openjdk.org/browse/JDK-8209017</a><br>
</div>
<div><a href="https://bugs.openjdk.org/browse/JDK-8190331" target="_blank">https://bugs.openjdk.org/browse/JDK-8190331</a><br>
</div>
<div><br>
</div>
<div>The graphic of the checkbox of a CheckBoxTreeItem is
not set correctly on the new CheckBox that is provided
with the cell when virtual flow switches it. It might
happen with other controls that use virtual flow.</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Dec 1, 2022 at
8:40 PM Kevin Rushforth <<a href="mailto:kevin.rushforth@oracle.com" target="_blank">kevin.rushforth@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This seems related, but
somewhat tangential. A Control's "graphic" isn't <br>
a child node, just like a Shape's "clip" isn't a child
node.<br>
<br>
Creating a separate "document graph" (or "logical graph")
sounds like an <br>
interesting idea, but it would bring its own set of
challenges. And it <br>
wouldn't directly solve this case anyway.<br>
<br>
-- Kevin<br>
<br>
<br>
On 12/1/2022 9:42 AM, Michael Strauß wrote:<br>
> There's a larger picture here: from a user
perspective, there's a<br>
> difference between the scene graph and the "document
graph".<br>
> The document graph is what users actually work with,
for example by<br>
> setting the `Labeled.graphic` property. In some
cases, document nodes<br>
> don't correspond to scene nodes at all (`MenuItem` or
`Tab` come to<br>
> mind).<br>
> The document graph is later inflated into a scene
graph of unknown<br>
> structure (because skins are mostly black boxes with
regards to their<br>
> internal structure).<br>
><br>
> I've proposed an enhancement that would make the
document graph a<br>
> first-class citizen:<br>
> <a href="https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html" rel="noreferrer" target="_blank">https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html</a><br>
><br>
> With this in place, we could simply disallow the same
node appearing<br>
> multiple times in the document graph, which would not
only solve the<br>
> problem for `Labeled`, but for all controls with a
similar problem.<br>
><br>
><br>
> On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx <<a href="mailto:john.hendrikx@gmail.com" target="_blank">john.hendrikx@gmail.com</a>>
wrote:<br>
>> The mechanism does seem like it is a bit poorly
designed, as it is easy to create inconsistencies.<br>
>><br>
>> Luckily it seems that you can't remove a graphic
yourself from a Control (getChildren is protected).<br>
>><br>
>> I don't think there is an easy solution though...<br>
>><br>
>> I think that once you set a graphic on a Labeled,
you need to somehow mark it as "in use". Normally you
could just check parent != null for this, but it is
trickier than that. The Skin ultimately determines if it
adds the graphic as child, which may be delayed or may
even be disabled (content display property is set to
showing TEXT only).<br>
>><br>
>> Perhaps Skins should always add the graphic and
just hide it (visible false, managed false), but that's
going to be hard to enforce.<br>
>><br>
>> Marking the graphic as "in use" could be done
with:<br>
>><br>
>> - a property in `getProperties`<br>
>> - a new "owner" or "ownedBy" property<br>
>> - allowing "parent" to be set without adding it
to children (probably going to mess up stuff)<br>
>><br>
>> --John<br>
<br>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote></div>
</blockquote></div>
</blockquote></div>