Re: RFR: 8185831: Psuedo selectors do not appear to work in Node.lookupAll() [v2]
On Thu, 19 Oct 2023 05:47:35 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1993:
>>
>>> 1991: */
>>> 1992: List lookupAll(Selector selector, List 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 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
Re: RFR: 8185831: Psuedo selectors do not appear to work in Node.lookupAll() [v2]
On Wed, 18 Oct 2023 18:07:53 GMT, Andy Goryachev wrote:
>> Sai Pradeep Dandem has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8185831: Fixed test failing issues and code review comments
>
> modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1993:
>
>> 1991: */
>> 1992: List lookupAll(Selector selector, List 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(!)).
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1364940463
Re: RFR: 8185831: Psuedo selectors do not appear to work in Node.lookupAll() [v2]
On Wed, 18 Oct 2023 03:45:32 GMT, Sai Pradeep Dandem wrote:
>> **Issue:**
>> Using pseudo classes in programmatic query using Node.lookupAll() or
>> Node.lookup() gives unexpected results.
>>
>> **Cause:**
>> There is no check for checking the psuedo states matching in the applies()
>> method of SimpleSelector.java. So checking for "applies()" alone is not
>> sufficient in lookup() method.
>>
>> **Fix:**
>> Included an extra check for the psuedo states to match.
>
> Sai Pradeep Dandem has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8185831: Fixed test failing issues and code review comments
Another question: this PR changes the behavior of Node.lookup() and
Node.lookupAll() in respect to pseudo classes.
Now, wouldn't that pose a regression risk for applications?
Should we create a new method with the new semantics instead (+ boolean
considerPseudoClasses or some such)?
modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1993:
> 1991: */
> 1992: List lookupAll(Selector selector, List 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.
modules/javafx.graphics/src/test/java/test/javafx/scene/Node_lookup_Test.java
line 108:
> 106:
> 107: @Test
> 108: public void lookupPsuedoTest(){
spelling: lookupPseudoTest
(also in the bug and PR title)
-
PR Comment: https://git.openjdk.org/jfx/pull/1245#issuecomment-1769133936
PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1364317319
PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1364312507
Re: RFR: 8185831: Psuedo selectors do not appear to work in Node.lookupAll() [v2]
On Tue, 19 Sep 2023 12:43:12 GMT, Kevin Rushforth wrote: >> Sai Pradeep Dandem has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8185831: Fixed test failing issues and code review comments > > This causes many test failures, which suggests that this is not the right > fix. And no, the solution is not to change the existing tests. @kevinrushforth mentioned that > This causes many test failures, which suggests that this is not the right > fix. And no, the solution is not to change the existing tests. github actions pass all tests, and headful gradle tests all pass, at least on macOS. Are we ok then? - PR Comment: https://git.openjdk.org/jfx/pull/1245#issuecomment-1769126652
Re: RFR: 8185831: Psuedo selectors do not appear to work in Node.lookupAll() [v2]
On Wed, 18 Oct 2023 03:45:32 GMT, Sai Pradeep Dandem wrote: >> **Issue:** >> Using pseudo classes in programmatic query using Node.lookupAll() or >> Node.lookup() gives unexpected results. >> >> **Cause:** >> There is no check for checking the psuedo states matching in the applies() >> method of SimpleSelector.java. So checking for "applies()" alone is not >> sufficient in lookup() method. >> >> **Fix:** >> Included an extra check for the psuedo states to match. > > Sai Pradeep Dandem has updated the pull request incrementally with one > additional commit since the last revision: > > 8185831: Fixed test failing issues and code review comments This looks more reasonable now. - PR Comment: https://git.openjdk.org/jfx/pull/1245#issuecomment-1768355060
Re: RFR: 8185831: Psuedo selectors do not appear to work in Node.lookupAll() [v2]
> **Issue:** > Using pseudo classes in programmatic query using Node.lookupAll() or > Node.lookup() gives unexpected results. > > **Cause:** > There is no check for checking the psuedo states matching in the applies() > method of SimpleSelector.java. > > **Fix:** > Included the check for the states for matching. Sai Pradeep Dandem has updated the pull request incrementally with one additional commit since the last revision: 8185831: Fixed test failing issues and code review comments - Changes: - all: https://git.openjdk.org/jfx/pull/1245/files - new: https://git.openjdk.org/jfx/pull/1245/files/524db831..75323661 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1245=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1245=00-01 Stats: 69 lines in 3 files changed: 30 ins; 7 del; 32 mod Patch: https://git.openjdk.org/jfx/pull/1245.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1245/head:pull/1245 PR: https://git.openjdk.org/jfx/pull/1245
