8258777: SkinBase: add api to un-/register invalidation-/listChange listeners #409

kleopatra · 2021-02-22T14:58:45Z

Changes in Lambda..Handler:

added api and implemenation to support invalidation and listChange listeners in the same way as changeListeners
added java doc
added tests

Changes in SkinBase

added api (and implementation delegating to the handler)
copied java doc from the change listener un/register methods
added tests to verify that the new (and old) api is indeed delegating to the handler

Note that the null handling is slightly extended: all methods now can handle both null consumers (as before) and null observables (new) - this allows simplified code on rewiring "path" properties (see reference example in the issue)

Progress

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

Issue

JDK-8258777: SkinBase: add api to un-/register invalidation-/listChange listeners

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/409/head:pull/409
$ git checkout pull/409

Update a local copy of the PR:
$ git checkout pull/409
$ git pull https://git.openjdk.java.net/jfx pull/409/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 409

View PR using the GUI difftool:
$ git pr show -t 409

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/409.diff

listeners

bridgekeeper · 2021-02-22T14:58:58Z

👋 Welcome back fastegal! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2021-02-22T15:03:13Z

Webrevs

kevinrushforth · 2021-02-22T20:22:51Z

/reviewers 2
/csr

openjdk · 2021-02-22T20:23:16Z

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

openjdk · 2021-02-22T20:23:47Z

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@kleopatra please create a CSR request and add link to it in JDK-8258777. This pull request cannot be integrated until the CSR request is approved.

kevinrushforth

Not yet reviewed. All of the new API methods need to have an @since 17 javadoc tag.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java

kleopatra · 2021-03-23T11:41:23Z

wondering about the sequence of next steps: should I create the csr before or after (partial) review of this? Doing before feels like expecting duplicate work (whenever doc changes are required, I'll have to change the code in the pr and the csr draft as well, I assume). If that's the way, I'll go it, naturally :)

kevinrushforth · 2021-03-23T15:18:06Z

I'll review the API changes in the next day or so. Then you can update the CSR and move to Proposed. I'll also ask Ambarish to be the second reviewer on this.

kevinrushforth · 2021-03-24T22:47:48Z

The API looks good to me, so go ahead and create a Draft CSR (go to your JBS issue and select "More --> Create CSR"). I don't think it will change much, if any, so you won't be duplicating work. Since you are adding new methods to an existing class, you can use the format from JDK-8259868 for the Specification section, just listing the new methods and their API docs, rather than diffs, which would be needed if changing existing javadoc comments.

nlisker

I reviewed only the public API methods.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java

nlisker · 2021-03-24T23:53:24Z

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java

+     * @param observable The observable for which all listeners should be removed.
+     * @return A single chained {@link Consumer} consisting of all {@link Consumer consumers} registered through
+     *      {@link #registerInvalidationListener(Observable, Consumer)}. If no consumers have been registered on this
+     *      property, null will be returned.


"null" --> {@code null}

"property" --> "observable"

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java

.../src/main/java/com/sun/javafx/scene/control/LambdaMultiplePropertyChangeListenerHandler.java

kleopatra · 2021-03-25T11:25:18Z

@nlisker thanks for the detailed doc review - I really like your suggestions, which I think are considerable improvements (I did part of it in the respective methods of the handler) over the original doc :) Because that's what the doc of the new methods are: c&p from the un/registerChangeListener javadoc, adjusted to the specific type of the listener.

For consistency, all related method doc should be improved along your lines. How to get there? Options

use old doc with new methods, leave improving the doc for all methods to a follow-up issue
keep old doc for old method, use improved doc for the new methods, leave improving the doc for the old method to a follow-up issue
use improved doc in old and new methods in this issue, changing the old

Last would be doing it right once and for all .. my reluctant (against my lazy self ;) preference would be the last.

nlisker · 2021-03-25T13:14:45Z

Can you list the other affected methods?

kleopatra · 2021-03-25T13:29:44Z

Can you list the other affected methods?

at line 211 (in the changed skinBase)

/**
     * Subclasses can invoke this method to register that they want to listen to
     * property change events for the given property. Registered {@link Consumer} instances
     * will be executed in the order in which they are registered.
     * @param property the property
     * @param consumer the consumer
     */
    protected final void registerChangeListener(ObservableValue<?> property, Consumer<ObservableValue<?>> consumer) {

and at line 255

/**
     * Unregisters all change listeners that have been registered using {@link #registerChangeListener(ObservableValue, Consumer)}
     * for the given property. The end result is that the given property is no longer observed by any of the change
     * listeners, but it may still have additional listeners registered on it through means outside of
     * {@link #registerChangeListener(ObservableValue, Consumer)}.
     *
     * @param property The property for which all listeners should be removed.
     * @return A single chained {@link Consumer} consisting of all {@link Consumer consumers} registered through
     *      {@link #registerChangeListener(ObservableValue, Consumer)}. If no consumers have been registered on this
     *      property, null will be returned.
     * @since 9
     */
    protected final Consumer<ObservableValue<?>> unregisterChangeListeners(ObservableValue<?> property) {

nlisker · 2021-03-25T13:52:20Z

I see. I recommend that they be improved in this PR. I don't know if this will need to be part of the CSR, though.

kevinrushforth · 2021-03-25T13:59:16Z

Making this change seems fine to me, and better than using different language in the new methods without fixing the existing.

I don't know if this will need to be part of the CSR, though.

It would be best to include them.

kleopatra · 2021-03-25T14:07:24Z

@nlisker and @kevin so we agree, thanks :)

my plan:

will work on the exact doc along the lines of Nir's suggestion for the un-/registerInvalidationListener methods
once that's basically agreed upon, will c&p the spec (with adjustments) to the methods for the other listener types
at a last step, create the csr draft (that would be a patch for the changeListener methods and simply added spec for the new methods, I assume?)

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java

kleopatra · 2021-03-30T10:06:58Z

hmm ... failing checks, why?

kevinrushforth · 2021-03-30T12:43:41Z

hmm ... failing checks, why?

The failure on Windows is because your branch isn't up-to-date with master, and is missing a recent fix to the Window build. You can either git merge master (after updating your master branch from the upstream repo) or ignore the error.

nlisker · 2021-03-30T19:25:10Z

I think that the docs are ready for the CSR now.

kevinrushforth

One comment on the API that should be resolved before submitting the CSR.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java

kleopatra · 2021-04-12T12:02:20Z

created a csr draft JDK-8265063 - waiting for all monsters in hell breaking out and heavens falling down ;-)

kevinrushforth · 2021-04-12T13:24:18Z

created a csr draft JDK-8265063 - waiting for all monsters in hell breaking out and heavens falling down ;-)

The Draft CSR looks good, and should keep the monsters contained. 😄 Go ahead and move it to Proposed.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java

nlisker · 2021-04-12T14:00:48Z

I looked at all the new public docs and they look good (I left one minor comment). I didn't review anything else.

kleopatra · 2021-04-12T14:46:46Z

The Draft CSR looks good, and should keep the monsters contained. 😄 Go ahead and move it to Proposed.

thanks, done :) Also updated to the most recent state (including Nir's suggestion).

kevinrushforth

I finished reviewing the API changes, and it all looks good. I noted a follow-up bug that I need to file.

I'll review the CSR (to make sure it matches exactly the latest API docs in the code) in parallel with reviewing the implementation.

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java

kevinrushforth · 2021-04-15T12:44:01Z

I filed JDK-8265277 to add the missing @since 9 javadoc tag.

kevinrushforth

The fix and tests looks good, with one suggestion for the test (and a couple minor comments about formatting and one naming question) left inline.

I'll review the CSR now, you can then move it to Finalized when you are ready.

.../src/main/java/com/sun/javafx/scene/control/LambdaMultiplePropertyChangeListenerHandler.java

kevinrushforth · 2021-04-16T18:53:57Z

.../src/main/java/com/sun/javafx/scene/control/LambdaMultiplePropertyChangeListenerHandler.java


+    @SuppressWarnings("rawtypes")
+    private  static final Consumer EMPTY_CONSUMER = e -> {};


Minor: remove extra space between public and static.

kevinrushforth · 2021-04-16T18:53:57Z

...vafx.controls/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleHandlerTest.java

+        Consumer<Change<?>> consumer = change -> changes.add(change);
+        WeakReference<LambdaMultiplePropertyChangeListenerHandler> ref =
+                new WeakReference<>(new LambdaMultiplePropertyChangeListenerHandler());
+        ref.get().registerListChangeListener(items, consumer);


This is fragile. It is possible, although unlikely, that the referent could be GCed between its construction in the previous statement (after which it goes out of scope), and this statement. If this rare event happened, it would cause an NPE here. I recommend to keep a local reference to the referent and then set it to null after this (and before the attemptGC).

changed as suggested. I used a similar pattern in BehaviorMemoryTest - should I file a testbug to change it in the same way?

Yes, please file a new bug (not high priority, since it is a corner case). I took a look and there are a few other controls tests that use this pattern as well (BehaviorCleanupTest, ListViewTest, TabPaneTest, SkinCleanupTest, SkinLabeledCleanupTest, SkinMemoryLeakTest), so the new bug could cover fixing this for all of them.

done and taken (should cleanup my own dirt ;) JDK-8265406

...vafx.controls/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleHandlerTest.java

...ols/src/test/java/test/com/sun/javafx/scene/control/LambdaMultipleObservableHandlerTest.java

kevinrushforth

Looks good.

kevinrushforth · 2021-04-22T21:40:26Z

Pending a second reviewer.

openjdk · 2021-04-28T11:12:50Z

@kleopatra 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:

8258777: SkinBase: add api to un-/register invalidation-/listChange listeners

Reviewed-by: kcr, arapte

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 57 new commits pushed to the master branch:

33bbf3f: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene
483f171: 8264677: MemoryLeak: Progressindicator leaks, when treeShowing is false
6b63bf5: 8265669: AccumCell should not be visible
ed080c8: 8262276: Debug build of WebKit fails
db30e71: 8265513: Openjfx graphics build broken (Eclipse)
b50ce94: 8264952: [TestBug] Controls unit tests - ControlTest and SpinnerTest fail for non US Locale
27e57d3: 8265469: Allow to build media and webkit for Linux-AArch64
dfda00d: 8265514: Openjfx controls running tests broken (Eclipse)
1b407cc: 8239880: CSS tests should cleanup any global state they modify
fab638a: 8265425: Hard failure when building OpenJFX for Linux AArch64
... and 47 more: https://git.openjdk.java.net/jfx/compare/9e42eea46d735f80235effdebb535ee66b59f7d1...master

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.

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

kleopatra · 2021-04-28T11:39:21Z

/integrate

openjdk · 2021-04-28T11:40:28Z

@kleopatra Since your change was applied there have been 57 commits pushed to the master branch:

33bbf3f: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene
483f171: 8264677: MemoryLeak: Progressindicator leaks, when treeShowing is false
6b63bf5: 8265669: AccumCell should not be visible
ed080c8: 8262276: Debug build of WebKit fails
db30e71: 8265513: Openjfx graphics build broken (Eclipse)
b50ce94: 8264952: [TestBug] Controls unit tests - ControlTest and SpinnerTest fail for non US Locale
27e57d3: 8265469: Allow to build media and webkit for Linux-AArch64
dfda00d: 8265514: Openjfx controls running tests broken (Eclipse)
1b407cc: 8239880: CSS tests should cleanup any global state they modify
fab638a: 8265425: Hard failure when building OpenJFX for Linux AArch64
... and 47 more: https://git.openjdk.java.net/jfx/compare/9e42eea46d735f80235effdebb535ee66b59f7d1...master

Your commit was automatically rebased without conflicts.

Pushed as commit cc70cdf.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8258777: SkinBase: add api to un-/register invalidation-/listChange

da0820a

listeners

openjdk bot added the rfr Ready for review label Feb 22, 2021

openjdk bot added the csr Need approved CSR to integrate pull request label Feb 22, 2021

kevinrushforth reviewed Feb 22, 2021

View changes

fixed missing/incorrect @SInCE tags in new api doc

a342eba

openjdk bot mentioned this pull request Mar 17, 2021

8252936: Optimize removal of listeners from ExpressionHelper.Generic #108

Draft

3 tasks

kevinrushforth requested a review from arapte March 23, 2021 15:18

nlisker reviewed Mar 24, 2021

View changes

SkinBase: changed javadoc for xxInvalidationListener

f93b6d8

openjdk bot removed the rfr Ready for review label Mar 29, 2021

fixed trailing whitespace

68d70c6

openjdk bot added the rfr Ready for review label Mar 29, 2021

nlisker reviewed Mar 29, 2021

View changes

xxInvalidationListener: changed doc as per review

cf3a022

kevinrushforth reviewed Apr 1, 2021

View changes

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java Outdated Show resolved Hide resolved

kleopatra added 2 commits April 6, 2021 15:57

changed javadoc of unregister (invalidation only)

e7849b2

updated doc of list/Change methods that of invalidation methods

0736fe5

nlisker reviewed Apr 7, 2021

View changes

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java Outdated Show resolved Hide resolved

kevinrushforth self-requested a review April 9, 2021 16:08

replaced param name in changeListener methods

c19c8f9

nlisker reviewed Apr 12, 2021

View changes

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java Outdated Show resolved Hide resolved

changed description as suggested in review

7e31073

kevinrushforth reviewed Apr 15, 2021

View changes

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java Show resolved Hide resolved

modules/javafx.controls/src/main/java/javafx/scene/control/SkinBase.java Show resolved Hide resolved

kevinrushforth reviewed Apr 16, 2021

View changes

changed according to review

99ffcc4

kevinrushforth approved these changes Apr 17, 2021

View changes

openjdk bot removed the csr Need approved CSR to integrate pull request label Apr 17, 2021

arapte approved these changes Apr 28, 2021

View changes

openjdk bot added the ready Ready to be integrated label Apr 28, 2021

openjdk bot closed this Apr 28, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Apr 28, 2021

kleopatra deleted the bug-fix-8258777 branch April 28, 2021 11:55


		@SuppressWarnings("rawtypes")
		private static final Consumer EMPTY_CONSUMER = e -> {};

8258777: SkinBase: add api to un-/register invalidation-/listChange listeners #409

8258777: SkinBase: add api to un-/register invalidation-/listChange listeners #409

Conversation

kleopatra commented Feb 22, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 22, 2021

mlbridge bot commented Feb 22, 2021 • edited

Webrevs

kevinrushforth commented Feb 22, 2021

openjdk bot commented Feb 22, 2021

openjdk bot commented Feb 22, 2021

kevinrushforth left a comment

Choose a reason for hiding this comment

kleopatra commented Mar 23, 2021

kevinrushforth commented Mar 23, 2021

kevinrushforth commented Mar 24, 2021

nlisker left a comment

Choose a reason for hiding this comment

nlisker Mar 24, 2021

Choose a reason for hiding this comment

kleopatra commented Mar 25, 2021

nlisker commented Mar 25, 2021

kleopatra commented Mar 25, 2021

nlisker commented Mar 25, 2021

kevinrushforth commented Mar 25, 2021

kleopatra commented Mar 25, 2021 • edited

kleopatra commented Mar 30, 2021

kevinrushforth commented Mar 30, 2021

nlisker commented Mar 30, 2021

kevinrushforth left a comment

Choose a reason for hiding this comment

kleopatra commented Apr 12, 2021 • edited

kevinrushforth commented Apr 12, 2021

nlisker commented Apr 12, 2021

kleopatra commented Apr 12, 2021

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth commented Apr 15, 2021

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Apr 16, 2021

Choose a reason for hiding this comment

kleopatra Apr 17, 2021

Choose a reason for hiding this comment

kevinrushforth Apr 16, 2021

Choose a reason for hiding this comment

kleopatra Apr 17, 2021

Choose a reason for hiding this comment

kevinrushforth Apr 17, 2021

Choose a reason for hiding this comment

kleopatra Apr 18, 2021

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth commented Apr 22, 2021

openjdk bot commented Apr 28, 2021

kleopatra commented Apr 28, 2021

openjdk bot commented Apr 28, 2021

kleopatra commented Feb 22, 2021 •

edited by openjdk bot

mlbridge bot commented Feb 22, 2021 •

edited

kleopatra commented Mar 25, 2021 •

edited

kleopatra commented Apr 12, 2021 •

edited