Re: Setting graphics of a Labeled does not show the Label correctly
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 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 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 >> 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 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). > >> >
Re: Setting graphics of a Labeled does not show the Label correctly
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 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 > 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 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 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
Re: Setting graphics of a Labeled does not show the Label correctly
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 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 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 >>> 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 >>> >>>
Re: Setting graphics of a Labeled does not show the Label correctly
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 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 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 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
Re: Setting graphics of a Labeled does not show the Label correctly
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 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 > 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 >> 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 >> >>
Re: Setting graphics of a Labeled does not show the Label correctly
Yes, I always felt something was off about how graphics and clips worked and that in some cases these are actually inaccessible children (you can still look them up) -- it feels like they should allow sharing (clips especially) but things then break. On 01/12/2022 18:42, 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. And also when Nodes are shared between different types of properties: Label label; Button button; label.setGraphic(button); label.setClip(button); Even with the logic in `setClip` (using `clipParent`) this will still go undetected. --John On Thu, Dec 1, 2022 at 6:17 PM John Hendrikx 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
Re: Setting graphics of a Labeled does not show the Label correctly
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 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 > 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 > >
Re: Setting graphics of a Labeled does not show the Label correctly
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 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
Re: Setting graphics of a Labeled does not show the Label correctly
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 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
Re: Setting graphics of a Labeled does not show the Label correctly
I'm not sure wrapping it in a runLater would make much difference, and if it does, it would be fragile. I think that the best approach might be to disallow using the same Node as the "graphic" of two controls, in the same way that we disallow setting a Node as a clip of two different nodes. This would involve a CSR, since it's both a spec and a behavior change. The implementation would need to check whether the Node is in use, and if so, throw an exception (after first unbinding if bound). -- Kevin On 12/1/2022 9:20 AM, Andy Goryachev wrote: Does wrapping the action code in runLater() help? b1.setOnAction((ev) -> { Platform.runLater(() -> { if (b1.getParent() == cb1) { ... -andy *From: *openjfx-dev on behalf of John Hendrikx *Date: *Thursday, 2022/12/01 at 09:14 *To: *Nir Lisker *Cc: *openjfx-dev@openjdk.org *Subject: *Re: Setting graphics of a Labeled does not show the Label correctly 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 On 01/12/2022 15:21, Nir Lisker wrote: Technically it doesn't appear elsewhere in the scenegraph, it is the child of only one label. It's set as the graphics property of 2 labels though. The mismatch is that being set as a graphics property isn't a 1-to-1 relationship with being a child of the label. Something has to be fixed along this chain of inconsistency, I just wasn't sure where. It seems like the IAE that you mentioned should be thrown, but isn't. I can write a PR for that. One thing I'm not sure about is: in a situation where the graphic belongs to a label that is detached from a scene, and that graphic is set to a label that *is* part of a scene, should an IAE be thrown as well. Even if the Labeled is part of the Scene, the graphic may not be (depending on Skin used and on Content Display property for the default skin). By the way, changing to throwing an IAE is going to cause some new exceptions. There is a possibility that some VirtualFlow things will break because that mechanism recycles nodes and re-attaches their properties. Then again, it might just mean that it was done wrong. On Thu, Dec 1, 2022 at 3:49 PM John Hendrikx wrote: Sorry, I meant on the first click already. --John On 01/12/2022 14:46, John Hendrikx wrote: Setting the same Node for multiple graphics is not allowed. Your program should IMHO throw "IllegalArgumentException" on the 2nd click. * An optional icon for the Labeled. This can be positioned relative to the * text by using {@link #setContentDisplay}. The node specified for this * variable cannot appear elsewhere in the scene graph, otherwise * the {@code IllegalArgumentException} is thrown. See the class * description of {@link Node} for more detail. --John On 01/12/2022 13:38, Nir Lisker wrote: That's my point. Currently, a node can serve as the graphics property for multiple Labels, but will appear only in 1 because it can only have a single parent in the scenegraph. While this is technically fine, it causes issues like the one I showed above. I'm not sure if a node is supposed to be able to serve as multiple graphics (and displayed only in the last Label it was set to?), or removed from other Labels' graphic properties just like is done for children in the scenegraph. Personally, I find it confusing that label.getGraphics() will return a node that isn't shown in that label. On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx wrote:
Re: Setting graphics of a Labeled does not show the Label correctly
Does wrapping the action code in runLater() help? b1.setOnAction((ev) -> { Platform.runLater(() -> { if (b1.getParent() == cb1) { ... -andy From: openjfx-dev on behalf of John Hendrikx Date: Thursday, 2022/12/01 at 09:14 To: Nir Lisker Cc: openjfx-dev@openjdk.org Subject: Re: Setting graphics of a Labeled does not show the Label correctly 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 On 01/12/2022 15:21, Nir Lisker wrote: Technically it doesn't appear elsewhere in the scenegraph, it is the child of only one label. It's set as the graphics property of 2 labels though. The mismatch is that being set as a graphics property isn't a 1-to-1 relationship with being a child of the label. Something has to be fixed along this chain of inconsistency, I just wasn't sure where. It seems like the IAE that you mentioned should be thrown, but isn't. I can write a PR for that. One thing I'm not sure about is: in a situation where the graphic belongs to a label that is detached from a scene, and that graphic is set to a label that *is* part of a scene, should an IAE be thrown as well. Even if the Labeled is part of the Scene, the graphic may not be (depending on Skin used and on Content Display property for the default skin). By the way, changing to throwing an IAE is going to cause some new exceptions. There is a possibility that some VirtualFlow things will break because that mechanism recycles nodes and re-attaches their properties. Then again, it might just mean that it was done wrong. On Thu, Dec 1, 2022 at 3:49 PM John Hendrikx mailto:john.hendr...@gmail.com>> wrote: Sorry, I meant on the first click already. --John On 01/12/2022 14:46, John Hendrikx wrote: Setting the same Node for multiple graphics is not allowed. Your program should IMHO throw "IllegalArgumentException" on the 2nd click. * An optional icon for the Labeled. This can be positioned relative to the * text by using {@link #setContentDisplay}. The node specified for this * variable cannot appear elsewhere in the scene graph, otherwise * the {@code IllegalArgumentException} is thrown. See the class * description of {@link Node} for more detail. --John On 01/12/2022 13:38, Nir Lisker wrote: That's my point. Currently, a node can serve as the graphics property for multiple Labels, but will appear only in 1 because it can only have a single parent in the scenegraph. While this is technically fine, it causes issues like the one I showed above. I'm not sure if a node is supposed to be able to serve as multiple graphics (and displayed only in the last Label it was set to?), or removed from other Labels' graphic properties just like is done for children in the scenegraph. Personally, I find it confusing that label.getGraphics() will return a node that isn't shown in that label. On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx mailto:john.hendr...@gmail.com>> wrote: Internally the graphics is just a child node, and nodes can't be part of the scene graph twice (this is done in LabeledSkinBase). It showing up only once is probably because it is getting removed from the other labels. I think things are probably getting out of sync, where the graphics property may think it still has a certain node as its graphics, but it is no longer a child of the label. --John On 01/12/2022 11:23, Nir Lisker wrote: > Hi, > > Given the following code > > var cb1 = new Label("1"); > var cb2 = new Label ("2"); > var b1 = new Button("A"); > cb1.setGraphic(b1); > b1.setOnAction(e -> { > if (b1.getParent() == cb1) { > // cb1.setGraphic(null); > cb2.setGraphic(b1); > } else { > // cb2.setGraphic(null); > cb1.setGraphic(b1); > } > }); >
Re: Setting graphics of a Labeled does not show the Label correctly
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 On 01/12/2022 15:21, Nir Lisker wrote: Technically it doesn't appear elsewhere in the scenegraph, it is the child of only one label. It's set as the graphics property of 2 labels though. The mismatch is that being set as a graphics property isn't a 1-to-1 relationship with being a child of the label. Something has to be fixed along this chain of inconsistency, I just wasn't sure where. It seems like the IAE that you mentioned should be thrown, but isn't. I can write a PR for that. One thing I'm not sure about is: in a situation where the graphic belongs to a label that is detached from a scene, and that graphic is set to a label that *is* part of a scene, should an IAE be thrown as well. Even if the Labeled is part of the Scene, the graphic may not be (depending on Skin used and on Content Display property for the default skin). By the way, changing to throwing an IAE is going to cause some new exceptions. There is a possibility that some VirtualFlow things will break because that mechanism recycles nodes and re-attaches their properties. Then again, it might just mean that it was done wrong. On Thu, Dec 1, 2022 at 3:49 PM John Hendrikx wrote: Sorry, I meant on the first click already. --John On 01/12/2022 14:46, John Hendrikx wrote: Setting the same Node for multiple graphics is not allowed. Your program should IMHO throw "IllegalArgumentException" on the 2nd click. * An optional icon for the Labeled. This can be positioned relative to the * text by using {@link #setContentDisplay}. The node specified for this * variable cannot appear elsewhere in the scene graph, otherwise * the {@code IllegalArgumentException} is thrown. See the class * description of {@link Node} for more detail. --John On 01/12/2022 13:38, Nir Lisker wrote: That's my point. Currently, a node can serve as the graphics property for multiple Labels, but will appear only in 1 because it can only have a single parent in the scenegraph. While this is technically fine, it causes issues like the one I showed above. I'm not sure if a node is supposed to be able to serve as multiple graphics (and displayed only in the last Label it was set to?), or removed from other Labels' graphic properties just like is done for children in the scenegraph. Personally, I find it confusing that label.getGraphics() will return a node that isn't shown in that label. On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx wrote: Internally the graphics is just a child node, and nodes can't be part of the scene graph twice (this is done in LabeledSkinBase). It showing up only once is probably because it is getting removed from the other labels. I think things are probably getting out of sync, where the graphics property may think it still has a certain node as its graphics, but it is no longer a child of the label. --John On 01/12/2022 11:23, Nir Lisker wrote: > Hi, > > Given the following code > > var cb1 = new Label("1"); > var cb2 = new Label ("2"); > var b1 = new Button("A"); > cb1.setGraphic(b1); > b1.setOnAction(e -> { > if (b1.getParent() == cb1) { > // cb1.setGraphic(null); > cb2.setGraphic(b1); > } else { > // cb2.setGraphic(null); > cb1.setGraphic(b1); > } > }); > > Pressing the first button will move it (the graphic) to the second > label, however, pressing it again will not move it back to the first > label. It's required to set the graphics to null prior to moving them > as in the commented lines.
Re: Setting graphics of a Labeled does not show the Label correctly
Technically it doesn't appear elsewhere in the scenegraph, it is the child of only one label. It's set as the graphics property of 2 labels though. The mismatch is that being set as a graphics property isn't a 1-to-1 relationship with being a child of the label. Something has to be fixed along this chain of inconsistency, I just wasn't sure where. It seems like the IAE that you mentioned should be thrown, but isn't. I can write a PR for that. One thing I'm not sure about is: in a situation where the graphic belongs to a label that is detached from a scene, and that graphic is set to a label that *is* part of a scene, should an IAE be thrown as well. By the way, changing to throwing an IAE is going to cause some new exceptions. There is a possibility that some VirtualFlow things will break because that mechanism recycles nodes and re-attaches their properties. Then again, it might just mean that it was done wrong. On Thu, Dec 1, 2022 at 3:49 PM John Hendrikx wrote: > Sorry, I meant on the first click already. > > --John > On 01/12/2022 14:46, John Hendrikx wrote: > > Setting the same Node for multiple graphics is not allowed. Your program > should IMHO throw "IllegalArgumentException" on the 2nd click. > > * An optional icon for the Labeled. This can be positioned relative > to the > * text by using {@link #setContentDisplay}. The node specified for > this > * variable cannot appear elsewhere in the scene graph, otherwise > * the {@code IllegalArgumentException} is thrown. See the class > * description of {@link Node} for more detail. > > --John > On 01/12/2022 13:38, Nir Lisker wrote: > > That's my point. Currently, a node can serve as the graphics property for > multiple Labels, but will appear only in 1 because it can only have a > single parent in the scenegraph. While this is technically fine, it causes > issues like the one I showed above. I'm not sure if a node is supposed to > be able to serve as multiple graphics (and displayed only in the last Label > it was set to?), or removed from other Labels' graphic properties just like > is done for children in the scenegraph. Personally, I find it confusing > that label.getGraphics() will return a node that isn't shown in that label. > > On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx > wrote: > >> Internally the graphics is just a child node, and nodes can't be part of >> the scene graph twice (this is done in LabeledSkinBase). >> >> It showing up only once is probably because it is getting removed from >> the other labels. >> >> I think things are probably getting out of sync, where the graphics >> property may think it still has a certain node as its graphics, but it >> is no longer a child of the label. >> >> --John >> >> On 01/12/2022 11:23, Nir Lisker wrote: >> > Hi, >> > >> > Given the following code >> > >> > var cb1 = new Label("1"); >> > var cb2 = new Label ("2"); >> > var b1 = new Button("A"); >> > cb1.setGraphic(b1); >> > b1.setOnAction(e -> { >> > if (b1.getParent() == cb1) { >> > // cb1.setGraphic(null); >> > cb2.setGraphic(b1); >> > } else { >> > // cb2.setGraphic(null); >> > cb1.setGraphic(b1); >> > } >> > }); >> > >> > Pressing the first button will move it (the graphic) to the second >> > label, however, pressing it again will not move it back to the first >> > label. It's required to set the graphics to null prior to moving them >> > as in the commented lines. This looks like a bug to me. I would think >> > that when a graphic is moved, it will appear in its new label >> > immediately, like moving a child. Apparently a single node can be the >> > graphics for multiple Labeled nodes, but it will appear only in 1. Is >> > this intentional? >> > >> > - Nir >> >
Re: Setting graphics of a Labeled does not show the Label correctly
Sorry, I meant on the first click already. --John On 01/12/2022 14:46, John Hendrikx wrote: Setting the same Node for multiple graphics is not allowed. Your program should IMHO throw "IllegalArgumentException" on the 2nd click. * An optional icon for the Labeled. This can be positioned relative to the * text by using {@link #setContentDisplay}. The node specified for this * variable cannot appear elsewhere in the scene graph, otherwise * the {@code IllegalArgumentException} is thrown. See the class * description of {@link Node} for more detail. --John On 01/12/2022 13:38, Nir Lisker wrote: That's my point. Currently, a node can serve as the graphics property for multiple Labels, but will appear only in 1 because it can only have a single parent in the scenegraph. While this is technically fine, it causes issues like the one I showed above. I'm not sure if a node is supposed to be able to serve as multiple graphics (and displayed only in the last Label it was set to?), or removed from other Labels' graphic properties just like is done for children in the scenegraph. Personally, I find it confusing that label.getGraphics() will return a node that isn't shown in that label. On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx wrote: Internally the graphics is just a child node, and nodes can't be part of the scene graph twice (this is done in LabeledSkinBase). It showing up only once is probably because it is getting removed from the other labels. I think things are probably getting out of sync, where the graphics property may think it still has a certain node as its graphics, but it is no longer a child of the label. --John On 01/12/2022 11:23, Nir Lisker wrote: > Hi, > > Given the following code > > var cb1 = new Label("1"); > var cb2 = new Label ("2"); > var b1 = new Button("A"); > cb1.setGraphic(b1); > b1.setOnAction(e -> { > if (b1.getParent() == cb1) { > // cb1.setGraphic(null); > cb2.setGraphic(b1); > } else { > // cb2.setGraphic(null); > cb1.setGraphic(b1); > } > }); > > Pressing the first button will move it (the graphic) to the second > label, however, pressing it again will not move it back to the first > label. It's required to set the graphics to null prior to moving them > as in the commented lines. This looks like a bug to me. I would think > that when a graphic is moved, it will appear in its new label > immediately, like moving a child. Apparently a single node can be the > graphics for multiple Labeled nodes, but it will appear only in 1. Is > this intentional? > > - Nir
Re: Setting graphics of a Labeled does not show the Label correctly
Setting the same Node for multiple graphics is not allowed. Your program should IMHO throw "IllegalArgumentException" on the 2nd click. * An optional icon for the Labeled. This can be positioned relative to the * text by using {@link #setContentDisplay}. The node specified for this * variable cannot appear elsewhere in the scene graph, otherwise * the {@code IllegalArgumentException} is thrown. See the class * description of {@link Node} for more detail. --John On 01/12/2022 13:38, Nir Lisker wrote: That's my point. Currently, a node can serve as the graphics property for multiple Labels, but will appear only in 1 because it can only have a single parent in the scenegraph. While this is technically fine, it causes issues like the one I showed above. I'm not sure if a node is supposed to be able to serve as multiple graphics (and displayed only in the last Label it was set to?), or removed from other Labels' graphic properties just like is done for children in the scenegraph. Personally, I find it confusing that label.getGraphics() will return a node that isn't shown in that label. On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx wrote: Internally the graphics is just a child node, and nodes can't be part of the scene graph twice (this is done in LabeledSkinBase). It showing up only once is probably because it is getting removed from the other labels. I think things are probably getting out of sync, where the graphics property may think it still has a certain node as its graphics, but it is no longer a child of the label. --John On 01/12/2022 11:23, Nir Lisker wrote: > Hi, > > Given the following code > > var cb1 = new Label("1"); > var cb2 = new Label ("2"); > var b1 = new Button("A"); > cb1.setGraphic(b1); > b1.setOnAction(e -> { > if (b1.getParent() == cb1) { > // cb1.setGraphic(null); > cb2.setGraphic(b1); > } else { > // cb2.setGraphic(null); > cb1.setGraphic(b1); > } > }); > > Pressing the first button will move it (the graphic) to the second > label, however, pressing it again will not move it back to the first > label. It's required to set the graphics to null prior to moving them > as in the commented lines. This looks like a bug to me. I would think > that when a graphic is moved, it will appear in its new label > immediately, like moving a child. Apparently a single node can be the > graphics for multiple Labeled nodes, but it will appear only in 1. Is > this intentional? > > - Nir
Re: Setting graphics of a Labeled does not show the Label correctly
That's my point. Currently, a node can serve as the graphics property for multiple Labels, but will appear only in 1 because it can only have a single parent in the scenegraph. While this is technically fine, it causes issues like the one I showed above. I'm not sure if a node is supposed to be able to serve as multiple graphics (and displayed only in the last Label it was set to?), or removed from other Labels' graphic properties just like is done for children in the scenegraph. Personally, I find it confusing that label.getGraphics() will return a node that isn't shown in that label. On Thu, Dec 1, 2022 at 2:21 PM John Hendrikx wrote: > Internally the graphics is just a child node, and nodes can't be part of > the scene graph twice (this is done in LabeledSkinBase). > > It showing up only once is probably because it is getting removed from > the other labels. > > I think things are probably getting out of sync, where the graphics > property may think it still has a certain node as its graphics, but it > is no longer a child of the label. > > --John > > On 01/12/2022 11:23, Nir Lisker wrote: > > Hi, > > > > Given the following code > > > > var cb1 = new Label("1"); > > var cb2 = new Label ("2"); > > var b1 = new Button("A"); > > cb1.setGraphic(b1); > > b1.setOnAction(e -> { > > if (b1.getParent() == cb1) { > > // cb1.setGraphic(null); > > cb2.setGraphic(b1); > > } else { > > // cb2.setGraphic(null); > > cb1.setGraphic(b1); > > } > > }); > > > > Pressing the first button will move it (the graphic) to the second > > label, however, pressing it again will not move it back to the first > > label. It's required to set the graphics to null prior to moving them > > as in the commented lines. This looks like a bug to me. I would think > > that when a graphic is moved, it will appear in its new label > > immediately, like moving a child. Apparently a single node can be the > > graphics for multiple Labeled nodes, but it will appear only in 1. Is > > this intentional? > > > > - Nir >
Re: Setting graphics of a Labeled does not show the Label correctly
Internally the graphics is just a child node, and nodes can't be part of the scene graph twice (this is done in LabeledSkinBase). It showing up only once is probably because it is getting removed from the other labels. I think things are probably getting out of sync, where the graphics property may think it still has a certain node as its graphics, but it is no longer a child of the label. --John On 01/12/2022 11:23, Nir Lisker wrote: Hi, Given the following code var cb1 = new Label("1"); var cb2 = new Label ("2"); var b1 = new Button("A"); cb1.setGraphic(b1); b1.setOnAction(e -> { if (b1.getParent() == cb1) { // cb1.setGraphic(null); cb2.setGraphic(b1); } else { // cb2.setGraphic(null); cb1.setGraphic(b1); } }); Pressing the first button will move it (the graphic) to the second label, however, pressing it again will not move it back to the first label. It's required to set the graphics to null prior to moving them as in the commented lines. This looks like a bug to me. I would think that when a graphic is moved, it will appear in its new label immediately, like moving a child. Apparently a single node can be the graphics for multiple Labeled nodes, but it will appear only in 1. Is this intentional? - Nir
Setting graphics of a Labeled does not show the Label correctly
Hi, Given the following code var cb1 = new Label("1"); var cb2 = new Label ("2"); var b1 = new Button("A"); cb1.setGraphic(b1); b1.setOnAction(e -> { if (b1.getParent() == cb1) { // cb1.setGraphic(null); cb2.setGraphic(b1); } else { // cb2.setGraphic(null); cb1.setGraphic(b1); } }); Pressing the first button will move it (the graphic) to the second label, however, pressing it again will not move it back to the first label. It's required to set the graphics to null prior to moving them as in the commented lines. This looks like a bug to me. I would think that when a graphic is moved, it will appear in its new label immediately, like moving a child. Apparently a single node can be the graphics for multiple Labeled nodes, but it will appear only in 1. Is this intentional? - Nir