8290348: TreeTableView jumping to top

[johanvos]

Do not recalculate total estimated size recursively.

In the (unlikely) event that the recalculation triggers a new recalculation (e.g. when the height of a Cell is changed), do not start this recalculation.
The cache and cache size may become inconsistent if a recursive calculation is started.
This fixes JDK-8290348

---

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-8290348: TreeTableView jumping to top

Reviewers

• Ajit Ghaisas (@aghaisas - Reviewer)
• Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 836

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

Using diff file

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

[bridgekeeper]

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

[mlbridge]

Webrevs

• 03: Full - Incremental (879fb3da)
• 02: Full - Incremental (080ae765)
• 01: Full - Incremental (2afb1d37)
• 00: Full (b183a841)

[kevinrushforth]

/reviewers 2

[openjdk]

@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).

[aghaisas]

I tested on macOS. It is observed that the newly added test passes even without the VirtualFlow.java code change done in this PR.

[johanvos]

I'll make the test more deterministic.
FWIW, the following UI code allows to reproduce the failure (and fix) easier, but it is UI-based and has a Thread.sleep() so I won't add this in a unit test.
(the generateChildren method is the same as in the unit test)

        TreeItem<String> root = new TreeItem<>("Root Node");
        root.setExpanded(true);
        root.getChildren().addAll(generateChildren(1));

        TreeView<String> treeView = new TreeView<>(root);

        treeView.scrollTo(100);
        Thread t = new Thread() {
            @Override public void run() {
                try {
                    Thread.sleep(3000);
                    Platform.runLater(() -> {
                        root.getChildren().get(1).setExpanded(false);
                    });
                } catch (Exception e) {
                    e.printStackTrace();
                }
            }
        };
        t.start();
        Scene scene = new Scene(treeView, 640, 480);
        stage.setScene(scene);
        stage.show();

[aghaisas]

Looks good to me!

[kevinrushforth]

Looks good. I confirm that both the visual test and updated unit test fail without the fix and pass with the fix. I left one question inline, but I doubt it is a problem.

[kevinrushforth]

Is it likely that this method might get an exception? I was wondering whether the assignment to false should be in a try / finally. If there is no probable way an exception could occur, or if an exception is non-recoverable (which is quite possible), then no need for a try / finally.

[johanvos]

Good question. There should be no non-recoverable exceptions in the VirtualFlow specific code. However, since IndexedCell.updateItem() and others might get called from this method, exceptions there might propagate.
My initial thought was to fail-fast, but it is probably better to use a try/finally so that at least this method is still "working" somehow. I'll add that.

[openjdk]

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

8290348: TreeTableView jumping to top

Reviewed-by: aghaisas, kcr

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

• 25e8605: Merge
• 69ae421: Merge
• 929c1c7: Merge
• e8b9ff1: Merge
• a7c345a: 8285691: Additional fix for JDK-8282121
• bee2e55: Merge
• 445b36b: 8282121: Improve WebKit referencing

Please see this link for an up-to-date comparison between the source branch of this pull request and the jfx19 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.

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

[johanvos]

/integrate

[openjdk]

Going to push as commit 6da05c2.
Since your change was applied there have been 7 commits pushed to the jfx19 branch:

• 25e8605: Merge
• 69ae421: Merge
• 929c1c7: Merge
• e8b9ff1: Merge
• a7c345a: 8285691: Additional fix for JDK-8282121
• bee2e55: Merge
• 445b36b: 8282121: Improve WebKit referencing

Your commit was automatically rebased without conflicts.

[openjdk]

@johanvos Pushed as commit 6da05c2.

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