8298728: Cells in VirtualFlow jump after resizing #974

johanvos · 2022-12-14T10:03:29Z

When recalculating sizes, we often don't want the current index and/or offset to change.

Allow to fix the index/offset when doing recalculations.

Fix JDK-8298728

Progress

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

Issue

JDK-8298728: Cells in VirtualFlow jump after resizing

Reviewers

Ajit Ghaisas (@aghaisas - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 974

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

Using diff file

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

…r offset to change. Allow to fix the index/offset when doing recalculations. Fix JDK-8298728

bridgekeeper · 2022-12-14T10:04:35Z

👋 Welcome back jvos! 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 · 2022-12-14T10:12:01Z

Webrevs

andy-goryachev-oracle · 2022-12-14T20:54:26Z

I am trying to understand what the behavioral difference is between this branch and the current master. The ticket JDK-8298728 does not seem to specify the exact scenario, so I tried the following code with the large model (3000 lines) with a wrapping Text as graphic node. Resizing both vertically and horizontally produces what I this is identical result where the top visible selected cell scrolls out of the view in both cases.

cell factory:

                       TableColumn<String,String> c = new TableColumn<>();
                        table.getColumns().add(c);
                        c.setText("C" + table.getColumns().size());
                        c.setCellValueFactory((f) -> new SimpleStringProperty(describe(c)));
                        c.setCellFactory((r) -> {
                            return new TableCell<>() {
                                @Override
                                protected void updateItem(String item, boolean empty) {
                                    super.updateItem(item, empty);
                                    Text t = new Text("11111111111111111111111111111111111111111111111111111111111111111111111111111111111111\n2\n3\n");
                                    t.wrappingWidthProperty().bind(widthProperty());
                                    setPrefHeight(USE_COMPUTED_SIZE);
                                    setGraphic(t);
                                }
                            };
                        });

I can attache the whole program to the jira ticket.

andy-goryachev-oracle · 2022-12-14T18:06:28Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java

+        int oldIndex = computeCurrentIndex();
+        double cellLength = getOrCreateCellSize(index);
+        if (index > 0) getOrCreateCellSize(index - 1);
+        if (index < getCellCount() -1) getOrCreateCellSize(index + 1);


I think it's better to keep one statement per line - for stepping through in a debugger, and also if an exception gets thrown it will be easier to see where.

True -- I'll change that.

johanvos · 2022-12-14T21:18:33Z

I am trying to understand what the behavioral difference is between this branch and the current master.

I added a new test in VirtualFlowTest. It fails without the patch, and succeeds with the patch. The change is that if a cell is shown with some given content, then a cell is resized, and then a scroll operation is performed, the result of this scroll could be unexpected. I added comments in the added test so that the flow should be clear. Does this help?

andy-goryachev-oracle · 2022-12-14T21:59:31Z

I think this might be the case when unit tests are insufficient.
With this change, I notice something else: select a cell somewhere in the middle of the table, click on the scroll bar to page up, then click on the other side of the scroll bar to page down. The existing code works correctly, the new code does not - the position is different, sometimes the selected cell is scrolled outside of the view.
see "does not page back to the same spot.mp4" in jira

kevinrushforth · 2022-12-14T22:06:07Z

/reviewers 2

openjdk · 2022-12-14T22:06:53Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

johanvos · 2022-12-15T13:17:01Z

That sounds exactly like something that should be a unit test.

Add another failing test, and a fix: when the cell that is positioned at the "current index" is resized, we also need to modify the offset (wich is calculated from the top of that cell to the start of the viewport).

andy-goryachev-oracle · 2022-12-21T23:43:03Z

I see the same issue with page up/page down described earlier (see the video clip attached to the jira ticket https://bugs.openjdk.org/secure/attachment/102048/does%20not%20page%20back%20to%20the%20same%20spot.mov )
I am still not clear what exactly the problem is. The jira ticket contains conditionals ("if we want..."), rather than a typical steps to reproduce -> expected to see -> actual behavior steps.

Could you comment on #1 and #2 please?

FlorianKirmaier · 2022-12-22T11:57:59Z

I can confirm that this fixes every remaining issue related to ListView to my knowledge.

My test application (which was an extended version of the test in this ticket: https://bugs.openjdk.org/browse/JDK-8277785) works now 100% correctly.

The application at Zeiss works now without any ListView-related workaround.

So, on my side - with this fix, all ListView issues known to me are fixed.

johanvos · 2022-12-23T07:48:16Z

I see the same issue with page up/page down described earlier (see the video clip attached to the jira ticket https://bugs.openjdk.org/secure/attachment/102048/does%20not%20page%20back%20to%20the%20same%20spot.mov )

I am still not clear what exactly the problem is. The jira ticket contains conditionals ("if we want..."), rather than a typical steps to reproduce -> expected to see -> actual behavior steps.

Could you comment on #1 and #2 please?

#1: I had a look at that video and I don't think this is a bug. Nowhere in the spec or javadoc it is mentioned that a page-up followed by a page-down should give the same position. This would imho actually be a bad requirement, as in case sizes are changing (which happens to be the case in most real-world scenario's I've seen so far) this would conflict with other requirements. One might argue that "in case the size of individual cells didn't change", the scroll-up/scroll-down should result in a same "offset" as before the operation but that is not a requirement so far. Could be discussed in another issue.

#2. I don't see what is missing. It seems to me both the ticket and the new test in the PR show the scenario of what goes wrong before the patch, and what is expected (in the assert). I can copy it here again, but I'm not understanding your question I think.

andy-goryachev-oracle · 2022-12-23T17:25:50Z

Thank you for commenting, Johan.

Interesting. Why would sizes change in #1 scenario, since there are no changes in the component width and the scroll bar state? But I might disagree - it's the most natural case when pgup/pgdn results in the same state, as any text editor would demonstrate. In fact, I have a similar problem with "If we want the position of the thumb constant," - I'd rather see the thumb reflecting reality as much as it could, so it is ok for it to jump, as long as the selected cell remains in the same position on screen. It looks like the ticket talks about the "first cell" instead which would be an acceptable replacement.

As for #2 - it might be just me, I cannot translate these words into a scenario. Are you saying that, for example, resizing and immediately issuing pgup/pgdn (via click on a scrollbar) would shift the viewport too far/not enough? Or is there another scenario? What are the exact steps that result in erroneous behavior, in human words?

aghaisas

Overall fix looks good to me.

I have identified two very minor typos.

aghaisas · 2023-01-03T11:58:22Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java

+        assertEquals(300, thirdCell.getLayoutY(), 1.);
+
+
+        for (int i =0 ; i < heights.length; i++) {


Minor : Need a space after =

aghaisas · 2023-01-03T11:59:27Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java

+        if (index > 0) {
+            getOrCreateCellSize(index - 1);
+        }
+        if (index < getCellCount() -1) {


Minor : Need a space after -

andy-goryachev-oracle

lgtm

openjdk · 2023-01-05T16:32:48Z

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

8298728: Cells in VirtualFlow jump after resizing

Reviewed-by: aghaisas, angorya

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

e866a6c: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
0dbc448: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
94fb7ed: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
012fa16: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
a35c3bf: 8292922: [Linux] No more drag events when new Stage is created in drag handler
5b96d34: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
1d9e2af: 8297068: Update boot JDK to 19.0.1
ac3f60c: 8297554: Remove Scene.KeyHandler
9c52605: 8252863: Spinner keeps spinning if removed from Scene
... and 6 more: https://git.openjdk.org/jfx/compare/8763e8b0d67db33fc3aa40c459058957e3230346...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.

johanvos · 2023-01-05T20:16:46Z

/integrate

openjdk · 2023-01-05T20:19:26Z

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

e866a6c: 8296413: Tree/TableView with null focus model throws NPE in queryAccessibleAttribute()
0dbc448: 8231864: JavaFX Labels in Tab's VBox is not displayed until it is clicked
94fb7ed: 8216507: StyleablePropertyFactory: example in class javadoc does not compile
012fa16: 8209017: CheckBoxTreeCell: graphic on TreeItem not always showing
a35c3bf: 8292922: [Linux] No more drag events when new Stage is created in drag handler
5b96d34: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
1d9e2af: 8297068: Update boot JDK to 19.0.1
ac3f60c: 8297554: Remove Scene.KeyHandler
9c52605: 8252863: Spinner keeps spinning if removed from Scene
... and 6 more: https://git.openjdk.org/jfx/compare/8763e8b0d67db33fc3aa40c459058957e3230346...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-05T20:19:48Z

@johanvos Pushed as commit ca29cc6.

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

When recalculating sizes, we often don't want the current index and/o…

5fb56af

…r offset to change. Allow to fix the index/offset when doing recalculations. Fix JDK-8298728

openjdk bot added the rfr Ready for review label Dec 14, 2022

hohonuuli mentioned this pull request Dec 14, 2022

Jumping between records mbari-org/vars-annotation#128

Closed

andy-goryachev-oracle reviewed Dec 14, 2022

View reviewed changes

kevinrushforth requested a review from aghaisas December 14, 2022 22:05

move statements to new lines.

5d4474f

Add another failing test, and a fix: when the cell that is positioned at the "current index" is resized, we also need to modify the offset (wich is calculated from the top of that cell to the start of the viewport).

kevinrushforth requested a review from andy-goryachev-oracle December 21, 2022 12:45

aghaisas reviewed Jan 3, 2023

View reviewed changes

Process reviewer comments: fix typos (add space)

4ce32b7

aghaisas approved these changes Jan 5, 2023

View reviewed changes

andy-goryachev-oracle approved these changes Jan 5, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jan 5, 2023

openjdk bot added the integrated Pull request has been integrated label Jan 5, 2023

openjdk bot closed this Jan 5, 2023

openjdk bot removed the ready Ready to be integrated label Jan 5, 2023

openjdk bot removed the rfr Ready for review label Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8298728: Cells in VirtualFlow jump after resizing #974

8298728: Cells in VirtualFlow jump after resizing #974

johanvos commented Dec 14, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 14, 2022

mlbridge bot commented Dec 14, 2022 •

edited

Loading

andy-goryachev-oracle commented Dec 14, 2022

andy-goryachev-oracle Dec 14, 2022

johanvos Dec 15, 2022

johanvos commented Dec 14, 2022

andy-goryachev-oracle commented Dec 14, 2022

kevinrushforth commented Dec 14, 2022

openjdk bot commented Dec 14, 2022

johanvos commented Dec 15, 2022

andy-goryachev-oracle commented Dec 21, 2022

FlorianKirmaier commented Dec 22, 2022

johanvos commented Dec 23, 2022

andy-goryachev-oracle commented Dec 23, 2022

aghaisas left a comment

aghaisas Jan 3, 2023

johanvos Jan 5, 2023

aghaisas Jan 3, 2023

johanvos Jan 5, 2023

andy-goryachev-oracle left a comment

openjdk bot commented Jan 5, 2023

johanvos commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

		assertEquals(300, thirdCell.getLayoutY(), 1.);


		for (int i =0 ; i < heights.length; i++) {

8298728: Cells in VirtualFlow jump after resizing #974

8298728: Cells in VirtualFlow jump after resizing #974

Conversation

johanvos commented Dec 14, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 14, 2022

mlbridge bot commented Dec 14, 2022 • edited Loading

Webrevs

andy-goryachev-oracle commented Dec 14, 2022

andy-goryachev-oracle Dec 14, 2022

Choose a reason for hiding this comment

johanvos Dec 15, 2022

Choose a reason for hiding this comment

johanvos commented Dec 14, 2022

andy-goryachev-oracle commented Dec 14, 2022

kevinrushforth commented Dec 14, 2022

openjdk bot commented Dec 14, 2022

johanvos commented Dec 15, 2022

andy-goryachev-oracle commented Dec 21, 2022

FlorianKirmaier commented Dec 22, 2022

johanvos commented Dec 23, 2022

andy-goryachev-oracle commented Dec 23, 2022

aghaisas left a comment

Choose a reason for hiding this comment

aghaisas Jan 3, 2023

Choose a reason for hiding this comment

johanvos Jan 5, 2023

Choose a reason for hiding this comment

aghaisas Jan 3, 2023

Choose a reason for hiding this comment

johanvos Jan 5, 2023

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 5, 2023

johanvos commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

openjdk bot commented Jan 5, 2023

johanvos commented Dec 14, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 14, 2022 •

edited

Loading