Setting graphics of a Labeled does not show the Label correctly

[Nir Lisker]

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 <nlisker at 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 <nlisker at 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.hendrikx at 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 <nlisker at 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.rushforth at 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.hendrikx at 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
>>>>>
>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20221218/2267d259/attachment-0001.htm>