On Wed, 8 Jan 2025 11:57:44 GMT, John Hendrikx <jhendr...@openjdk.org> 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 479: > >> 477: >> n.pseudoClassStateChanged(NTH_EVEN_CHILD_PSEUDO_CLASS, i % 2 != 0); >> 478: >> n.pseudoClassStateChanged(NTH_ODD_CHILD_PSEUDO_CLASS, i % 2 == 0); >> 479: } > > With this new code, Nodes with many children being manipulated may incur some > significant new costs now that these pseudo classes need to be changed for > potentially many/all children. Where before adding a new first child mostly > had a cost in shifting the child array with a copy, now we also potentially > have two listener call backs per child (and I'm unsure, but these may even > trigger some CSS recalculations or CSS cache invalidations). Nodes with many > children may suffer from this, and mostly needlessly as these pseudo classes > will see very limited use. > > I think there's some potential for performance regression here that is > perhaps sufficient to warrant the exclusion of this feature or making it > optional. I think this concern is out of proportion. Generally, nodes will not have such a huge number of children that simply flipping a few pseudo-classes will be prohibitively expensive. CSS recalculation will, in most cases, only happen once per pulse anyway. I don't know why we would be optimizing for pathological applications that register two pseudo-class listeners for each of their tens of thousands of children in a container, _and_ do heavy work in those listeners. > modules/javafx.graphics/src/test/java/test/javafx/scene/Parent_pseudoClasses_Test.java > line 60: > >> 58: assertNotPseudoClass(ONLY_CHILD, child1); >> 59: assertPseudoClass(ONLY_CHILD, child2); >> 60: } > > How about a case where the last child is removed, and seeing if the first and > now only child gets the correct states? I think there currently is > definitely a bug where the odd/even may not be reapplied in that case... I've added more tests, including the scenario you point out. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907558879 PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907559803
