RFR: 8185831: Psuedo selectors do not appear to work in Node.lookupAll() [v2]

[Andy Goryachev]

On Thu, 19 Oct 2023 05:47:35 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1993:
>> 
>>> 1991:      */
>>> 1992:     List<Node> lookupAll(Selector selector, List<Node> results) {
>>> 1993:         if (selectorMatches(selector)) {
>> 
>> line 1990 might be incorrect: the return value **can** be null.
>> We probably should just correct the javadoc, and since it's a private API no CSR is needed.  All callers of this method do check for null.
>
> I must say I'm again baffled at how this is implemented.  
> 
> Red flag one: it uses a `LinkedList` which are known to be rarely the right choice, and I doubt this case is any different.  
> 
> Red flag two: a `List` is converted to a `Set`; being backed by a `LinkedList` means set type operations will be terribly slow if that list has any kind of size.  
> 
> Red flag three: The choice to return a `Set` in the public API seems to be only motivated to indicate there won't be any duplicates, which is a terrible reason.
> 
> Red flag four: `UnmodifiableListSet` talks about insertion speed and hashing overhead, yet uses a `LinkedList` which are slower than `ArrayList` when it comes to insertion speed (not to mention that they require more object allocations and more memory(!)).

You are right, @hjohn - LinkedList seems like a bad choice!  It should be HashSet from the very beginning, shouldn't it?

But Set<Node> as a return value for lookups is correct, I think.

We could (should?) fix it in a separate PR.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1365705477