On Wed, 8 Jan 2025 12:02:45 GMT, John Hendrikx <[email protected]> wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into feature/root-pseudo-class
>> - whitespace error
>> - add child-indexed pseudo-classes
>> - add pseudo-class table to Parent
>> - update cssref
>> - Set :root pseudo-class on sub-scene root node
>> - Set :root pseudo-class on root node
>
> modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 381:
>
>> 379: // the index of the first change to separate
>> unchanged from changed elements.
>> 380: if (firstDirtyChildIndex < 0) {
>> 381: firstDirtyChildIndex = from;
>
> Does this need some special casing in case the first element is modified
> indirectly? Let's say I have two elements, I remove the last element.
> Indirectly, the first element now gets the "only" and "last" pseudo classes;
> this may need to be communicated as a modification to the peer for example?
As long as the pseudoclasses are set correctly with `pseudoClassStateChanged`,
the peers will be notified as necessary. I don't think there is any
special-casing required here.
> modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 470:
>
>> 468: // Add the "last-child" pseudo-class to the last child.
>> 469: if (size() > 0) {
>> 470:
>> getLast().pseudoClassStateChanged(LAST_CHILD_PSEUDO_CLASS, true);
>
> These code paths seem to do double work. Would it not be better to treat
> each flag separately, instead of having some paths manipulate multiple flags,
> that then may get set/reset again individually? The first `if/else if` in
> this block will reset all flags, only to have some of them be set/reset again
> later.
>
> This may also lead to listener confusion; let's say I have 2 children. I
> remove the last child. The "only child" block (`size() == 1`) triggers; it
> removes the `odd` pseudo class; a listener may trigger on this; however, just
> a bit later, this `odd` tag is then re-added (at least, I hope it is as I'm
> not entirely sure the `firstDirtyChildIndex` will be set low enough...)
I've refactored the implementation here a bit to prevent repeatedly clearing
and setting a pseudo-class.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907549197
PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907551351
