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

8276553: ListView scrollTo() is broken after fix for JDK-8089589 #683

Closed
wants to merge 4 commits into from

Conversation

johanvos
Copy link
Collaborator

@johanvos johanvos commented Nov 29, 2021

After (re)setting the number of elements, make sure to do at least some estimation of the total size.
Added a testcase for this scenario.


Progress

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

Issue

  • JDK-8276553: ListView scrollTo() is broken after fix for JDK-8089589

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 683

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 29, 2021

👋 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.

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

mlbridge bot commented Nov 29, 2021

Webrevs

@kevinrushforth kevinrushforth self-requested a review Nov 29, 2021
Some hard numbers used in tests (counters for evaluations) were changed because of this.
Instead of relying on hard values, I modified the failing was into relative ones.
@johanvos johanvos requested a review from aghaisas Dec 1, 2021
@@ -1121,7 +1121,7 @@ public void updateItem(String color, boolean empty) {
items.set(30, "yellow");
Platform.runLater(() -> {
Toolkit.getToolkit().firePulse();
assertEquals(0, rt_35395_counter);
assertTrue(rt_35395_counter < 7);
Copy link
Collaborator

@aghaisas aghaisas Dec 3, 2021

Choose a reason for hiding this comment

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

I see that you have modified assertions to use "lesser than" some expected value. This may hide some incorrect test outcomes.
Along with "lesser than" assertion, do you think we should add a "greater than" assertion as well? This will have a range bounded expected value.
This is applicable for all assertion changes in this PR.

Copy link
Collaborator Author

@johanvos johanvos Dec 3, 2021

Choose a reason for hiding this comment

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

The hard values have been changed a number of times, and I believe it is not really a good metric.
What we want to ensure is that there is no functional regression and no performance penalty. The tests calculate the number of updateItem invocations and require those to be a strict number. With JDK-8089589, we fixed a number of issues that are related to the fact that the total size of the view (in case all cells are rendered with their preferred height) is not known. This is done by using an estimate of the total size. The estimate is 100% correct if we call updateItem for every item, but that would lead to a huge performance penalty in case the list contains a large amount of items with equal height.
Hence, there is a balance between minimizing the updateItem calls but still have a fair estimate of the total dimensions. Rather than requiring that the amount of calls should be a fixed number, I think it makes more sense to ensure that the amount of calls stays within reasonable boundaries, and is not growing exponentially when we add linearly more items, for example.

Copy link
Collaborator

@aghaisas aghaisas Dec 3, 2021

Choose a reason for hiding this comment

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

The hard values have been changed a number of times, and I believe it is not really a good metric.

I agree completely.

Rather than requiring that the amount of calls should be a fixed number, I think it makes more sense to ensure that the amount of calls stays within reasonable boundaries, and is not growing exponentially when we add linearly more items, for example.

My point is exactly this. I see that as part of this PR, you have added the upper boundary (rather than a fixed number) for assertions. I am asking whether we need a lower boundary as well?

Copy link
Collaborator Author

@johanvos johanvos Dec 3, 2021

Choose a reason for hiding this comment

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

I don't think we need a lower boundary. A value of 0 is not a bad thing, as long as the functional tests are ok.
The lowest possible value is implicitly determined by the usecase, which is covered by the functional tests.

Copy link
Member

@kevinrushforth kevinrushforth Dec 3, 2021

Choose a reason for hiding this comment

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

And in this case, since the value used to be 0 before this fix, I think that's fine. For the other cases would it make sense to have a > 0 check? Or are those cases for which 0 would be an OK value?

@@ -856,6 +856,8 @@ public String getName() {

@Override protected void invalidated() {
int cellCount = get();
resetSizeEstimates();
recalculateAndImproveEstimatedSize(2);
Copy link
Collaborator

@aghaisas aghaisas Dec 3, 2021

Choose a reason for hiding this comment

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

We can use recalculateEstimatedSize() instead of this method.
The effect is the same improvement with a size of 2.

Copy link
Collaborator Author

@johanvos johanvos Dec 3, 2021

Choose a reason for hiding this comment

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

True, good suggestion. I pushed that change.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The fix and new test looks fine. I added one comment/question regarding the existing tests that check a count, but I'm OK with either answer.

Approved.

@@ -1121,7 +1121,7 @@ public void updateItem(String color, boolean empty) {
items.set(30, "yellow");
Platform.runLater(() -> {
Toolkit.getToolkit().firePulse();
assertEquals(0, rt_35395_counter);
assertTrue(rt_35395_counter < 7);
Copy link
Member

@kevinrushforth kevinrushforth Dec 3, 2021

Choose a reason for hiding this comment

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

And in this case, since the value used to be 0 before this fix, I think that's fine. For the other cases would it make sense to have a > 0 check? Or are those cases for which 0 would be an OK value?

@openjdk
Copy link

openjdk bot commented Dec 3, 2021

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

8276553: ListView scrollTo() is broken after fix for JDK-8089589

Reviewed-by: kcr, mstrauss

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

  • aa045c5: 8272118: ListViewSkin et al: must not cancel edit on scrolling
  • 5bd72a7: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler")
  • 3d57213: 8277475: Update JDK_DOCS property to point to JDK 17 docs
  • b257647: 8276847: JSException: ReferenceError: Can't find variable: IntersectionObserver
  • fc3792d: 8276206: Rename TextBinding class to better express its purpose
  • 0dbdec4: 8276490: Incorrect path for duplicate x and y values, when path falls outside axis bound
  • d14be6a: 8274061: Tree-/TableRowSkin: misbehavior on switching skin
  • 423e1be: 8277133: Dragboard contents retrieved all over again during a DND process on WebView
  • 6e6c711: 8160597: IllegalArgumentException when we initiate drag on Image
  • e694fb5: 8254956: [REDO] Memoryleak: Closed focused Stages are not collected with Monocle
  • ... and 2 more: https://git.openjdk.java.net/jfx/compare/13c24d22463436bc953ec5159beec7a78017019f...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.

@openjdk openjdk bot added the ready Ready to be integrated label Dec 3, 2021
@mstr2
Copy link
Collaborator

mstr2 commented Dec 3, 2021

Would it be a good idea to fix the implementation of the position property as part of this PR (it has the same problem as cellCount before the fix)? If not, a follow-up ticket should be filed.

@kevinrushforth
Copy link
Member

kevinrushforth commented Dec 3, 2021

/reviewers 2

@openjdk
Copy link

openjdk bot commented Dec 3, 2021

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

@openjdk openjdk bot removed the ready Ready to be integrated label Dec 3, 2021
@kevinrushforth
Copy link
Member

kevinrushforth commented Dec 3, 2021

Would it be a good idea to fix the implementation of the position property as part of this PR (it has the same problem as cellCount before the fix)? If not, a follow-up ticket should be filed.

A follow-up issue, JDK-8278064, was filed a couple days ago.

mstr2
mstr2 approved these changes Dec 3, 2021
@openjdk openjdk bot added the ready Ready to be integrated label Dec 3, 2021
@openjdk openjdk bot removed the ready Ready to be integrated label Dec 3, 2021
@johanvos
Copy link
Collaborator Author

johanvos commented Dec 3, 2021

The fix and new test looks fine. I added one comment/question regarding the existing tests that check a count, but I'm OK with either answer.

The suggestion to test for positive values of the counters is a good idea. The lower this counter the better, but if the counter is 0, this means there has been no evaluation of updateItem. This should cause other tests to fail, but it helps if it would trigger a failure in this test as well.
I added these asserts on the places where previously the expected value was not 0.

mstr2
mstr2 approved these changes Dec 3, 2021
@openjdk openjdk bot added the ready Ready to be integrated label Dec 3, 2021
@johanvos
Copy link
Collaborator Author

johanvos commented Dec 3, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Dec 3, 2021

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

  • aa045c5: 8272118: ListViewSkin et al: must not cancel edit on scrolling
  • 5bd72a7: 8277457: AccessControlException: access denied ("java.net.NetPermission" "getCookieHandler")
  • 3d57213: 8277475: Update JDK_DOCS property to point to JDK 17 docs
  • b257647: 8276847: JSException: ReferenceError: Can't find variable: IntersectionObserver
  • fc3792d: 8276206: Rename TextBinding class to better express its purpose
  • 0dbdec4: 8276490: Incorrect path for duplicate x and y values, when path falls outside axis bound
  • d14be6a: 8274061: Tree-/TableRowSkin: misbehavior on switching skin
  • 423e1be: 8277133: Dragboard contents retrieved all over again during a DND process on WebView
  • 6e6c711: 8160597: IllegalArgumentException when we initiate drag on Image
  • e694fb5: 8254956: [REDO] Memoryleak: Closed focused Stages are not collected with Monocle
  • ... and 2 more: https://git.openjdk.java.net/jfx/compare/13c24d22463436bc953ec5159beec7a78017019f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 3, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Dec 3, 2021
@openjdk
Copy link

openjdk bot commented Dec 3, 2021

@johanvos Pushed as commit d3fbb51.

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

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
5 participants