8154038: Spinner's converter should update its editor #1057

karthikpandelu · 2023-03-10T11:38:56Z

When converter was changed, setText method of spinner was not invoked to update the spinner text using new converter.

Added a listener to converter property of SpinnerValueFactory to call setText so that spinner text is updated using new converter.

Added unit test to validate the fix. Along with spinner of Double type, added a spinner of Integer type as well so that the fix is validated using 2 types of spinner.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8154038: Spinner's converter should update its editor

Reviewers

Ajit Ghaisas (@aghaisas - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1057/head:pull/1057
$ git checkout pull/1057

Update a local copy of the PR:
$ git checkout pull/1057
$ git pull https://git.openjdk.org/jfx.git pull/1057/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1057

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1057.diff

bridgekeeper · 2023-03-10T11:39:56Z

👋 Welcome back kpk! 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 · 2023-03-10T11:45:06Z

Webrevs

01: Full - Incremental (f4376178)
00: Full (a45ba023)

aghaisas · 2023-03-15T12:15:14Z

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

@@ -541,6 +541,10 @@ public final ReadOnlyObjectProperty<T> valueProperty() {
                        // this binding is what ensures the Spinner.valueProperty()
                        // properly represents the value in the value factory
                        value.bind(newFactory.valueProperty());
+                        // Update the spinner editor when converter is changed.
+                        newFactory.converterProperty().addListener((o, oldValue, newValue) -> {


The addition of ChangeListener is fine, but, this newly added ChangeListener should be removed from old valueFactory when valueFactory changes.
Please see the pattern used in TimelineController for rateListener.

Updated the code. Please check

aghaisas · 2023-03-15T12:16:14Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/SpinnerTest.java

@@ -1523,4 +1525,56 @@ public void test_jdk_8150946_testCommit_invalid_wrongType() {
        localTimeValueFactory.setValue(null);
        assertNull(localTimeSpinner.getValue());
    }
+
+    @Test public void testSpinnerEditorUpdateOnConverterChange() {


It is better to split this test case into two - one for each type of Spinner.

Created separate tests for each type of Spinner. Please check

andy-goryachev-oracle · 2023-03-15T18:10:38Z

Added SpinnerPage to the MonkeyTester

https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/monkey/MonkeyTesterApp.java
https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/monkey/pages/SpinnerPage.java

aghaisas

Looks good!

openjdk · 2023-03-20T08:58:08Z

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

8154038: Spinner's converter should update its editor

Reviewed-by: aghaisas

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

b1964d9: Merge
f2118a1: 8304359: Create release notes for JavaFX 20
6be8703: 8303680: Virtual Flow freezes after calling scrollTo and scrollPixels in succession
935c7b7: 8301009: Update libxml2 to 2.10.3
147d71f: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly
4051f16: 8299968: Second call to Stage.setScene() create sizing issue with uiScale > 1.0
df22c41: 8302355: Public API for Toolkit.canStartNestedEventLoop()
8cda7bc: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex()

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 (@aghaisas) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

karthikpandelu · 2023-03-21T05:28:18Z

/integrate

openjdk · 2023-03-21T05:28:53Z

@karthikpandelu
Your change (at version f437617) is now ready to be sponsored by a Committer.

aghaisas · 2023-03-21T05:55:28Z

/sponsor

openjdk · 2023-03-21T05:57:20Z

Going to push as commit 0ab54e2.
Since your change was applied there have been 8 commits pushed to the master branch:

b1964d9: Merge
f2118a1: 8304359: Create release notes for JavaFX 20
6be8703: 8303680: Virtual Flow freezes after calling scrollTo and scrollPixels in succession
935c7b7: 8301009: Update libxml2 to 2.10.3
147d71f: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly
4051f16: 8299968: Second call to Stage.setScene() create sizing issue with uiScale > 1.0
df22c41: 8302355: Public API for Toolkit.canStartNestedEventLoop()
8cda7bc: 8302797: ArrayIndexOutOfBoundsException in TextRun.getWrapIndex()

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-21T05:57:34Z

@aghaisas @karthikpandelu Pushed as commit 0ab54e2.

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

Fix spinner converter issue

a45ba02

openjdk bot added the rfr Ready for review label Mar 10, 2023

kevinrushforth requested a review from aghaisas March 11, 2023 14:19

aghaisas suggested changes Mar 15, 2023

View reviewed changes

Address review comments

f437617

aghaisas approved these changes Mar 20, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Mar 20, 2023

openjdk bot added the sponsor Ready to sponsor label Mar 21, 2023

openjdk bot added the integrated Pull request has been integrated label Mar 21, 2023

openjdk bot closed this Mar 21, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Mar 21, 2023

karthikpandelu deleted the spinner_issue_fix branch March 21, 2023 06:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8154038: Spinner's converter should update its editor #1057

8154038: Spinner's converter should update its editor #1057

karthikpandelu commented Mar 10, 2023 •

edited by openjdk bot

bridgekeeper bot commented Mar 10, 2023

mlbridge bot commented Mar 10, 2023 •

edited

aghaisas Mar 15, 2023

karthikpandelu Mar 16, 2023

aghaisas Mar 15, 2023

karthikpandelu Mar 16, 2023

andy-goryachev-oracle commented Mar 15, 2023

aghaisas left a comment

openjdk bot commented Mar 20, 2023 •

edited

karthikpandelu commented Mar 21, 2023

openjdk bot commented Mar 21, 2023

aghaisas commented Mar 21, 2023

openjdk bot commented Mar 21, 2023

openjdk bot commented Mar 21, 2023

8154038: Spinner's converter should update its editor #1057

8154038: Spinner's converter should update its editor #1057

Conversation

karthikpandelu commented Mar 10, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 10, 2023

mlbridge bot commented Mar 10, 2023 • edited

Webrevs

aghaisas Mar 15, 2023

Choose a reason for hiding this comment

karthikpandelu Mar 16, 2023

Choose a reason for hiding this comment

aghaisas Mar 15, 2023

Choose a reason for hiding this comment

karthikpandelu Mar 16, 2023

Choose a reason for hiding this comment

andy-goryachev-oracle commented Mar 15, 2023

aghaisas left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 20, 2023 • edited

karthikpandelu commented Mar 21, 2023

openjdk bot commented Mar 21, 2023

aghaisas commented Mar 21, 2023

openjdk bot commented Mar 21, 2023

openjdk bot commented Mar 21, 2023

karthikpandelu commented Mar 10, 2023 •

edited by openjdk bot

mlbridge bot commented Mar 10, 2023 •

edited

openjdk bot commented Mar 20, 2023 •

edited