-
Notifications
You must be signed in to change notification settings - Fork 492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8185831: Pseudo selectors do not appear to work in Node.lookupAll() #1245
8185831: Pseudo selectors do not appear to work in Node.lookupAll() #1245
Conversation
👋 Welcome back SaiPradeepDandem! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/reviewers 3 |
@kevinrushforth |
@SaiPradeepDandem This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
This looks more reasonable now. |
@kevinrushforth mentioned that
github actions pass all tests, and headful gradle tests all pass, at least on macOS. Are we ok then? |
Another question: this PR changes the behavior of Node.lookup() and Node.lookupAll() in respect to pseudo classes. |
|
||
nodes = root.lookupAll(".d"); | ||
assertEquals(2, nodes.size()); | ||
assertTrue(nodes.contains(d)); | ||
assertTrue(nodes.contains(d2)); | ||
} | ||
|
||
@Test | ||
public void lookupPsuedoTest(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling: lookupPseudoTest
(also in the bug and PR title)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the spelling issue in the class. However, as I have no permissions to edit the bug title, I cannot change the PR title unless the bug title is corrected by some one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andy-goryachev-oracle Thanks for correcting the bug title. Corrected the PR title now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second though, I think this change will not cause regression: when no pseudo class is specified in the lookup selector, the lookup ignores the pseudo classes as before.
What I strongly recommend is to clarify the behavior of lookup() and lookupAll() in their javadoc (which might require a CSR) with respect to pseudo classes.
@@ -96,4 +109,41 @@ public class Node_lookup_Test { | |||
assertTrue(nodes.contains(d)); | |||
assertTrue(nodes.contains(d2)); | |||
} | |||
|
|||
@Test | |||
public void lookupPseudoTest(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please insert space
public void lookupPseudoTest() {
nodes = root.lookupAll(":testPseudo1"); | ||
assertEquals(2, nodes.size()); | ||
assertTrue(nodes.contains(g)); | ||
assertTrue(nodes.contains(hg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add one more test case to verify lack of regression when no pseudo class is specified. May be something like this (feel free to expand, add negative cases etc.):
/**
* Verifies that the lookup ignores pseudo classes when selector contains no explicit pseudo class.
*/
@Test
public void lookupPseudoTest2() {
Group root = new Group();
Group ab = new Group();
ab.getStyleClass().addAll("a", "b");
Group a = new Group();
a.getStyleClass().addAll("a");
Group a1 = new Group();
a1.getStyleClass().addAll("a");
a1.pseudoClassStateChanged(PseudoClass.getPseudoClass("P1"), true);
ParentShim.getChildren(root).addAll(a, a1, ab);
Set<Node> rv;
rv = root.lookupAll(".a");
assertTrue(rv.contains(a));
assertTrue(rv.contains(a1));
rv = root.lookupAll(".a:P1");
assertTrue(rv.contains(a1));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added another test case to cover the scenario you mentioned. I used the same nodes (have pseudo classes set) which are used in previous test case. Tried to cover all possible scenarios to check for regression.
* Verifies that the lookup ignores pseudo classes when selector contains no explicit pseudo class. | ||
*/ | ||
@Test | ||
public void lookupPseudoTest2() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason I've used different nodes is because the situation you have with existing nodes might give false positive.
I wanted to make sure that selector "a." will collect nodes with ".a" and ".a:pseudo", but in the standard set of nodes with g and hg nodes - but they have the same :testPseudo1, so it's not exactly equivalent.
The other option is to add new nodes, one with ".x" and another with ".x:pseudo"
If you do that, it might be easier to change the javadoc format to do something like this:
└─ Pane
├─ VFlow (Pane) .flow
│ ├─ ClippedPane (Pane) .content
(but it's just a very minor suggestion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. Now I got the point what you are saying about.
Added a new test case to cover this scenario. Used x, y.. naming to make a clear separation from previous scenarios.
However do you still prefer to keep the lookupPseudoTest2() scenarios? This is almost similar to quickTest() and lookupAllTest() except that all the nodes used in this method have pseudo classes set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could keep it - the more the merrier.
ideally, we could use a more thorough approach: let's say, use two or three layers, and create as many nodes as there are combinations, to test every combination of the selector and search result.
but the main result we got is that there should be no regression w.r.t. the old implementation (I think).
two more people should definitely take a look at this PR still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Thank you :)
…asses when the nodes have same style class
|
||
nodes = root.lookupAll(".x:random"); | ||
assertEquals(0, nodes.size()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent! thank you.
could you please remove the extra newline before } here ?
*/ | ||
private boolean selectorMatches(Selector s) { | ||
boolean matches = s != null && s.applies(this); | ||
if(matches && !s.createMatch().getPseudoClasses().isEmpty()){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a space after if
and before {
.
ParentShim.getChildren(root).addAll(a, bc); | ||
f = new Group(); | ||
f.getStyleClass().add("f"); | ||
f.pseudoClassStateChanged(PseudoClass.getPseudoClass("testPseudo"),true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a space after the comma in this line and other lines after that?
* Verifies that the lookup ignores pseudo classes when selector contains no explicit pseudo class. | ||
*/ | ||
@Test | ||
public void lookupPseudoTest3() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider naming the test methods according to what they test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed the methods by referring to method names used in other classes. I noticed that descriptive long method names is accepted, so named similarly. The only thing I want to check is: I renamed existing methods also. Is it accepted? or I am not allowed to rename others method names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no problem renaming: there are no dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, although I do think this needs to be documented on the public methods (it should not be a separate PR).
I agree with @hjohn : it would be nice to clarify the behavior of Node.lookup() and Node.lookupAll() in regards to pseudo classes. I think it will not require a CSR since it may qualify as a clarification. |
* If multiple nodes have same style class "myStyle" and few nodes have | ||
* a pseudo state "myPseudo", then to find all nodes with "myPseudo" state, the lookupAll method can be used as follows: | ||
* <code>scene.lookupAll(".myStyle:myPseudo");</code> or <code>scene.lookupAll(":myPseudo");</code> | ||
* </p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added explanation is very good! I would add one more thing - that if no pseudo class is specified by the lookup selector, the result will contain nodes with pseudo classes (that is, pseudo classes are ignored).
Minor note: should we be using {@code ... } instead of < code > ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second example is good. I echo my above comments about adding "For example, ".
* @param results The results. This will never be null. | ||
* @param selector The Selector. | ||
* @param results The results. | ||
* @return List of matching nodes. The returned value can be null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: I don't think we should capitalize text in @ param and @ return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Please change "The" and "List" to lower case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple comments on the API docs.
* be used to find this node as follows: {@code scene.lookup("#myId");}. | ||
* </p> | ||
* <p> | ||
* If two nodes, lets say NodeA and NodeB have same style class "myStyle" and NodeA has |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a separate paragraph, please add "For example, " here to make it clear that this is an example (and not normative specification). Maybe something like this:
* For example, if two nodes, NodeA and NodeB, have the same ...
* If multiple nodes have same style class "myStyle" and few nodes have | ||
* a pseudo state "myPseudo", then to find all nodes with "myPseudo" state, the lookupAll method can be used as follows: | ||
* <code>scene.lookupAll(".myStyle:myPseudo");</code> or <code>scene.lookupAll(":myPseudo");</code> | ||
* </p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second example is good. I echo my above comments about adding "For example, ".
* @param results The results. This will never be null. | ||
* @param selector The Selector. | ||
* @param results The results. | ||
* @return List of matching nodes. The returned value can be null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Please change "The" and "List" to lower case.
* <p> | ||
* If two nodes, lets say NodeA and NodeB have same style class "myStyle" and NodeA has | ||
* a pseudo state "myPseudo", then to find NodeA, the lookup method can be used as follows: | ||
* {@code scene.lookup(".myStyle:myPseudo");} or {@code scene.lookup(":myPseudo");}. If no pseudo class is specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sentence about what happens when no pseudo class is specified is true in general and not really part of this example. I recommend moving it before the two examples. And I think it could be worded more clearly.
* If the lookup selector does not specify a pseudo class, the lookup will ignore pseudo class
* states; it will return the first matching node even if it contains pseudo classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or possibly even more clear:
* If the lookup selector does not specify a pseudo class, the lookup will ignore pseudo class
* states; it will return the first matching node whether or not it contains pseudo classes.
* {@code scene.lookupAll(".myStyle:myPseudo");} or {@code scene.lookupAll(":myPseudo");}. If no pseudo class is specified | ||
* by the lookupAll selector, irrespective of the nodes pseudo states the result will contain all nodes matching the selector. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the same change regarding the last sentence that you did in the lookup
method (move it before the examples and reword to match lookup
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry overlooked that... done now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updated docs look good. I also verified that the provided test fails without the fix and passes with the fix.
NOTE: There is a Skara bug that causes the state of /reviewers
command to be ignored. Do not integrate this until there are three (3) approving reviews.
|
@SaiPradeepDandem This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 29 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @andy-goryachev-oracle, @hjohn) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🕶 looks good
@SaiPradeepDandem This is now OK to integrate. |
@kevinrushforth thanks for confirming. But I am pretty not sure what steps I need to do next. Is it that I need to type a new comment with /integrate ? as mentioned in
|
yes, just type slash-integrate command |
/integrate |
@SaiPradeepDandem |
/sponsor |
Going to push as commit bb06b64.
Your commit was automatically rebased without conflicts. |
@andy-goryachev-oracle @SaiPradeepDandem Pushed as commit bb06b64. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
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.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1245/head:pull/1245
$ git checkout pull/1245
Update a local copy of the PR:
$ git checkout pull/1245
$ git pull https://git.openjdk.org/jfx.git pull/1245/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1245
View PR using the GUI difftool:
$ git pr show -t 1245
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1245.diff
Webrev
Link to Webrev Comment