Skip to content
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

8214699: Node.getPseudoClassStates must return the same instance on every call #253

Closed
wants to merge 2 commits into from

Conversation

@arapte
Copy link
Member

arapte commented Jun 18, 2020

Node.getPseudoClassStates() returns a new UnmodifiableObservableSet of PseudoClassState on each call. So in order to listen to any changes in this set, user must call the method Node.getPseudoClassStates() only once and keep a strong reference to the returned UnmodifiableObservableSet.

So the fix is that the method Node.getPseudoClassStates() should return the same UnmodifiableObservableSet on every call.
As the returned set is an UnmodifiableObservableSet, it will not have any impact on it's usage.

Added a small unit test. and,
Altered(minor) a test which was modified in past(0ac9869) (62323e0) along with this method and should be updated in view of this change.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8214699: Node.getPseudoClassStates must return the same instance on every call

Reviewers

  • Jeanette Winzenburg (fastegal - Committer)
  • Kevin Rushforth (kcr - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/253/head:pull/253
$ git checkout pull/253

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 18, 2020

👋 Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Jun 18, 2020
@mlbridge
Copy link

mlbridge bot commented Jun 18, 2020

Webrevs

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 18, 2020

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jun 18, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 19, 2020

The fix looks good, given that FXCollections.unmodifiableObservableSet() wraps an observable set listens to changes to the backing set. Can you add a test to ensure that the listeners are getting called?

public void testGetPseudoClassStatesShouldReturnSameSet() {
Rectangle node = new Rectangle();
assertSame("getPseudoClassStates() should always return the same instance",
node.getPseudoClassStates(), node.getPseudoClassStates());

This comment has been minimized.

@kevinrushforth

kevinrushforth Jun 19, 2020 Member

To make this more clear, I recommend to store the expected value in a local variable and then call assert.

This comment has been minimized.

@kleopatra

kleopatra Jun 22, 2020 Collaborator

would suggest some more tests (overcautious maybe, be have seen horses vomit at unbelievable places ;)

Test that the returned set

  • is indeed unmodifiable
  • not gc'ed
  • is a wrapper around the backing set

Test code:

@Test(expected=UnsupportedOperationException.class)
public void testPseudoClassesUnmodifiable() {
    Node node = new Rectangle();
    node.getPseudoClassStates().add(PseudoClass.getPseudoClass("dummy"));
}

@Test
public void testPseudoClassesNotGCed() {
    Node node = new Rectangle();
    WeakReference<Set<?>> weakRef = new WeakReference<>(node.getPseudoClassStates());
    ControlSkinFactory.attemptGC(weakRef);
    assertNotNull("pseudoClassStates must not be gc'ed", weakRef.get());
}

// requires additional method in NodeShim
@Test
public void testUnmodifiablePseudoClassStatesEqualsBackingStates() {
    Rectangle node = new Rectangle();
    PseudoClass pseudo = PseudoClass.getPseudoClass("somepseudo");
    node.pseudoClassStateChanged(pseudo, true);
    assertEquals(1, node.getPseudoClassStates().size());
    assertEquals(NodeShim.pseudoClassStates(node).size(), node.getPseudoClassStates().size());
    assertTrue(NodeShim.pseudoClassStates(node).contains(pseudo));
    assertTrue(node.getPseudoClassStates().contains(pseudo));
}

This comment has been minimized.

@arapte

arapte Jun 23, 2020 Author Member

Corrected the existing test and added these new tests, with minor changes.
ControlSkinFactory is a file from control tests. So have added a copy of attemptGC() method in test.javafx.scene.shape.TestUtils class. The class does not seem to be a perfect location for attemptGC(). But wanted to avoid a new util kind off class just for this one method.

Copy link
Collaborator

kleopatra left a comment

forgot to comment the test (can't seem to be able to add to a review?)

public void testGetPseudoClassStatesShouldReturnSameSet() {
Rectangle node = new Rectangle();
assertSame("getPseudoClassStates() should always return the same instance",
node.getPseudoClassStates(), node.getPseudoClassStates());

This comment has been minimized.

@kleopatra

kleopatra Jun 22, 2020 Collaborator

would suggest some more tests (overcautious maybe, be have seen horses vomit at unbelievable places ;)

Test that the returned set

  • is indeed unmodifiable
  • not gc'ed
  • is a wrapper around the backing set

Test code:

@Test(expected=UnsupportedOperationException.class)
public void testPseudoClassesUnmodifiable() {
    Node node = new Rectangle();
    node.getPseudoClassStates().add(PseudoClass.getPseudoClass("dummy"));
}

@Test
public void testPseudoClassesNotGCed() {
    Node node = new Rectangle();
    WeakReference<Set<?>> weakRef = new WeakReference<>(node.getPseudoClassStates());
    ControlSkinFactory.attemptGC(weakRef);
    assertNotNull("pseudoClassStates must not be gc'ed", weakRef.get());
}

// requires additional method in NodeShim
@Test
public void testUnmodifiablePseudoClassStatesEqualsBackingStates() {
    Rectangle node = new Rectangle();
    PseudoClass pseudo = PseudoClass.getPseudoClass("somepseudo");
    node.pseudoClassStateChanged(pseudo, true);
    assertEquals(1, node.getPseudoClassStates().size());
    assertEquals(NodeShim.pseudoClassStates(node).size(), node.getPseudoClassStates().size());
    assertTrue(NodeShim.pseudoClassStates(node).contains(pseudo));
    assertTrue(node.getPseudoClassStates().contains(pseudo));
}
Copy link
Collaborator

kleopatra left a comment

looks fine :)

@openjdk
Copy link

openjdk bot commented Jun 23, 2020

@arapte This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8214699: Node.getPseudoClassStates must return the same instance on every call

Reviewed-by: fastegal, kcr
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

There are currently no new commits on the master branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate 8440b64b1ffc7a959e26fc4a299e14b62abd5ae9.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jun 23, 2020
@arapte
Copy link
Member Author

arapte commented Jun 23, 2020

/integrate

@openjdk openjdk bot closed this Jun 23, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 23, 2020
@openjdk
Copy link

openjdk bot commented Jun 23, 2020

@arapte
Pushed as commit a5878e0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.