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:
* MenuButton: * MenuButtonTest::testSetContentDisplayGraphicOnly - deliberately sets the same graphic to two MenuButtons. Doesn't look correct. * 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 LabeledImplOtherTest. test_RT_21357 [1]. Also seems like an odd implementation choice. 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. [1] https://bugs.openjdk.org/browse/JDK-8119175 On Mon, Dec 12, 2022 at 6:10 PM Nir Lisker <nlis...@gmail.com> wrote: > 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. > > On Thu, Dec 1, 2022 at 11:53 PM Nir Lisker <nlis...@gmail.com> wrote: > >> 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. >> >> On Thu, Dec 1, 2022 at 11:41 PM John Hendrikx <john.hendr...@gmail.com> >> wrote: >> >>> 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)? >>> >>> --John >>> On 01/12/2022 20:26, Nir Lisker wrote: >>> >>> 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. >>> >>> On Thu, Dec 1, 2022 at 8:47 PM Nir Lisker <nlis...@gmail.com> wrote: >>> >>>> By the way, these issues are caused by this inconsistent behavior (they >>>> are probably duplicates): >>>> >>>> https://bugs.openjdk.org/browse/JDK-8209017 >>>> https://bugs.openjdk.org/browse/JDK-8190331 >>>> >>>> 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. >>>> >>>> On Thu, Dec 1, 2022 at 8:40 PM Kevin Rushforth < >>>> kevin.rushfo...@oracle.com> wrote: >>>> >>>>> This seems related, but somewhat tangential. A Control's "graphic" >>>>> isn't >>>>> a child node, just like a Shape's "clip" isn't a child node. >>>>> >>>>> Creating a separate "document graph" (or "logical graph") sounds like >>>>> an >>>>> interesting idea, but it would bring its own set of challenges. And it >>>>> wouldn't directly solve this case anyway. >>>>> >>>>> -- Kevin >>>>> >>>>> >>>>> On 12/1/2022 9:42 AM, Michael Strauß wrote: >>>>> > There's a larger picture here: from a user perspective, there's a >>>>> > difference between the scene graph and the "document graph". >>>>> > The document graph is what users actually work with, for example by >>>>> > setting the `Labeled.graphic` property. In some cases, document nodes >>>>> > don't correspond to scene nodes at all (`MenuItem` or `Tab` come to >>>>> > mind). >>>>> > The document graph is later inflated into a scene graph of unknown >>>>> > structure (because skins are mostly black boxes with regards to their >>>>> > internal structure). >>>>> > >>>>> > I've proposed an enhancement that would make the document graph a >>>>> > first-class citizen: >>>>> > https://mail.openjdk.org/pipermail/openjfx-dev/2022-June/034417.html >>>>> > >>>>> > With this in place, we could simply disallow the same node appearing >>>>> > multiple times in the document graph, which would not only solve the >>>>> > problem for `Labeled`, but for all controls with a similar problem. >>>>> > >>>>> > >>>>> > On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx < >>>>> john.hendr...@gmail.com> wrote: >>>>> >> The mechanism does seem like it is a bit poorly designed, as it is >>>>> easy to create inconsistencies. >>>>> >> >>>>> >> Luckily it seems that you can't remove a graphic yourself from a >>>>> Control (getChildren is protected). >>>>> >> >>>>> >> I don't think there is an easy solution though... >>>>> >> >>>>> >> 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). >>>>> >> >>>>> >> Perhaps Skins should always add the graphic and just hide it >>>>> (visible false, managed false), but that's going to be hard to enforce. >>>>> >> >>>>> >> Marking the graphic as "in use" could be done with: >>>>> >> >>>>> >> - a property in `getProperties` >>>>> >> - a new "owner" or "ownedBy" property >>>>> >> - allowing "parent" to be set without adding it to children >>>>> (probably going to mess up stuff) >>>>> >> >>>>> >> --John >>>>> >>>>>