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

8277122: SplitPane divider drag can hang the layout #669

Conversation

Maran23
Copy link
Member

@Maran23 Maran23 commented Nov 15, 2021

When a divider is moved via drag or code it will call requestLayout() for the SplitPane.
While this is fine, it is also called when the SplitPaneSkin#layoutChildren(..) method is repositioning the divider.

This makes no sense since we are currently layouting everything, so we don't need to request it again. (The divider positioning is the first part of layoutChildren(..). In the second part the SplitPane content is layouted based off those positions)

-> With this behaviour the layout may hang under some conditions (check attached source). The problem is that the requestLayout() will mark the SplitPane dirty but won't propagate to the parent since the SplitPane is currently doing a layout.

This PR fixes this by not requesting layout when we are currently doing it (and thus repositioning the dividers as part of it).

Note: I also fixed a simple typo of a private method in SplitPaneSkin:
initializeDivderEventHandlers -> initializeDividerEventHandlers


Progress

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

Issue

  • JDK-8277122: SplitPane divider drag can hang the layout

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 669

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 15, 2021

👋 Welcome back mhanl! 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.

@openjdk openjdk bot added the rfr Ready for review label Nov 15, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 15, 2021

Webrevs

@kevinrushforth kevinrushforth requested a review from aghaisas Nov 18, 2021
@kleopatra
Copy link
Collaborator

kleopatra commented Nov 19, 2021

interesting issue - and fix :) Verified the mis-behaviour before the fix and its working after.

Wondering, though (read: I don't understand 😀)

  • why the layout details in the splitpane hinders the visual update of a completely unrelated component (like the combo)?
  • why does it only happen on increasing the divider pos, not on decreasing?

As to the test: would prefer to also see a test of the fixed effect (vs. the fix implementation of not re-entering layout) - might be a bit tricky, though.

@Maran23
Copy link
Member Author

Maran23 commented Nov 20, 2021

interesting issue - and fix :) Verified the mis-behaviour before the fix and its working after.

Wondering, though (read: I don't understand 😀)

  • why the layout details in the splitpane hinders the visual update of a completely unrelated component (like the combo)?
  • why does it only happen on increasing the divider pos, not on decreasing?

As to the test: would prefer to also see a test of the fixed effect (vs. the fix implementation of not re-entering layout) - might be a bit tricky, though.

  • the ComboBox is a child of the SplitPane, so therefore it is affected by the invalid layout state of the SplitPane.
    -> With that said, is doesn't need to be a ComboBox but can also be another SplitPane(divider drag won't work), TableView(scroll won't work), TitledPane(the collapse/expand won't work) and probably other controls as well.

  • it can also happen when decreasing, it might be that you need to set the min width to 0 of the left content of the SplitPane.
    -> The bug is only happening if you start drag the divider 'out of bounds', when the drag starts to have no effect since the divider is already at the min/max position. The layout code whoever will in t his case adjust the divider a second time since it would now be out of bounds, which will result in this bug.

Do you mean something like checking the ComboBox display text?

EDIT: I added another test for decreasing the divider position to a negatiave number. :)

@aghaisas
Copy link
Collaborator

aghaisas commented Nov 26, 2021

/reviewers 2

@openjdk
Copy link

openjdk bot commented Nov 26, 2021

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

@kevinrushforth kevinrushforth self-requested a review Nov 30, 2021
Copy link
Collaborator

@kleopatra kleopatra left a comment

fix looks good (though it feels a bit upside down that it is needed ;) - verified example/tests failing/passing before/after fixing

left minor inline comments (just to make it easier to understand :)

stageLoader.dispose();
}
Copy link
Collaborator

@kleopatra kleopatra Jan 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the stageLoader is not disposed if the test fails - a (recently introduced :) general pattern is to use an instance field that's disposed in the test cleanup

Copy link
Member Author

@Maran23 Maran23 Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, is this really needed? Not sure, are there any side effects by the StageLoader like this when a test failed?
Just asking since the StageLoader is used a lot like this.
And since the tests normally run green unless you may change something locally (on the JavaFX Pipeline it should never be red), it would probably never affect anything (or maybe it does?).

Copy link
Collaborator

@kleopatra kleopatra Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, is this really needed?

yes, IMO, we want the exact same cleanup for passing/failing tests. So either dispose is required always (then need to make sure it's called on failure) or not required always (then all its calls would be noise).

Not sure, are there any side effects by the StageLoader like this when a test failed? Just asking since the StageLoader is used a lot like this.

don't now (and doesn't matter, what matters is the guaranteed cleanup) - and aware of those slightly fishy patterns, we all learn :) Faintly remember having discussed the point in a PR (can't find it right now, though), and just as faintly remember the outcome was to guarantee the cleanup in new tests.

Copy link
Member Author

@Maran23 Maran23 Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah okay. Was just confusing for me since I never read that and I think some recent PRs still had this pattern, e.g. also the touch table scrolling PR I had a look at yesterday.

Maybe for future it makes sense to have an abstract test class with the stage loader setup already in place.

Copy link
Collaborator

@kleopatra kleopatra Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for reference - found the source of my faintly remember

Copy link
Member Author

@Maran23 Maran23 Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Unfortunately though that was not done consistently during the past PRs.

Copy link
Member Author

@Maran23 Maran23 Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also adjusted the copyright year ;)

Copy link
Collaborator

@kleopatra kleopatra left a comment

looks good now :)

Copy link
Collaborator

@aghaisas aghaisas left a comment

The fix looks good. +1.

@openjdk
Copy link

openjdk bot commented Jan 31, 2022

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

8277122: SplitPane divider drag can hang the layout

Reviewed-by: fastegal, 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 62 new commits pushed to 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 (@kleopatra, @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).

@openjdk openjdk bot added the ready Ready to be integrated label Jan 31, 2022
@Maran23
Copy link
Member Author

Maran23 commented Jan 31, 2022

/integrate

@openjdk openjdk bot added the sponsor Ready to sponsor label Jan 31, 2022
@openjdk
Copy link

openjdk bot commented Jan 31, 2022

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

@kleopatra
Copy link
Collaborator

kleopatra commented Jan 31, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Jan 31, 2022

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jan 31, 2022
@openjdk openjdk bot closed this Jan 31, 2022
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Jan 31, 2022
@openjdk
Copy link

openjdk bot commented Jan 31, 2022

@kleopatra @Maran23 Pushed as commit ee52d14.

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

@Maran23 Maran23 deleted the jdk-8277122-SplitPane-divider-drag-can-hang-the-layout branch Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants