[jfx17u] RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
Reviewed-by: kcr, mstrauss - Commit messages: - 8276553: ListView scrollTo() is broken after fix for JDK-8089589 Changes: https://git.openjdk.java.net/jfx17u/pull/50/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx17u=50=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276553 Stats: 29 lines in 5 files changed: 19 ins; 2 del; 8 mod Patch: https://git.openjdk.java.net/jfx17u/pull/50.diff Fetch: git fetch https://git.openjdk.java.net/jfx17u pull/50/head:pull/50 PR: https://git.openjdk.java.net/jfx17u/pull/50
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v4]
On Fri, 3 Dec 2021 20:46:46 GMT, Johan Vos wrote: >> 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. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > add checks on positive values on counters Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v4]
On Fri, 3 Dec 2021 20:46:46 GMT, Johan Vos wrote: >> 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. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > add checks on positive values on counters Marked as reviewed by mstrauss (Author). - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v4]
> 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. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: add checks on positive values on counters - Changes: - all: https://git.openjdk.java.net/jfx/pull/683/files - new: https://git.openjdk.java.net/jfx/pull/683/files/1a25a6d5..35713395 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=683=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=683=02-03 Stats: 6 lines in 2 files changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/683.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/683/head:pull/683 PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
On Fri, 3 Dec 2021 18:47:24 GMT, Kevin Rushforth wrote: > 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. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
On Fri, 3 Dec 2021 11:25:43 GMT, Johan Vos wrote: >> 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. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > process reviewer comment (simplify call) Marked as reviewed by mstrauss (Author). - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
On Fri, 3 Dec 2021 18:55:29 GMT, Michael Strauß wrote: > 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](https://bugs.openjdk.java.net/browse/JDK-8278064), was filed a couple days ago. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
On Fri, 3 Dec 2021 11:25:43 GMT, Johan Vos wrote: >> 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. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > process reviewer comment (simplify call) 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. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v2]
On Fri, 3 Dec 2021 15:10:35 GMT, Johan Vos wrote: >>> 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? > > 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. 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? - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
On Fri, 3 Dec 2021 11:25:43 GMT, Johan Vos wrote: >> 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. > > Johan Vos has updated the pull request incrementally with one additional > commit since the last revision: > > process reviewer comment (simplify call) 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. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v2]
On Fri, 3 Dec 2021 14:41:11 GMT, Ajit Ghaisas wrote: >> 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. > >> 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? 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. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v2]
On Fri, 3 Dec 2021 11:16:28 GMT, Johan Vos wrote: > 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? - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v2]
On Fri, 3 Dec 2021 10:24:52 GMT, Ajit Ghaisas wrote: >> Johan Vos has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Move functionality in the setCellCount() to the invalidated block. >> 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. > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java > line 860: > >> 858: int cellCount = get(); >> 859: resetSizeEstimates(); >> 860: recalculateAndImproveEstimatedSize(2); > > We can use recalculateEstimatedSize() instead of this method. > The effect is the same improvement with a size of 2. True, good suggestion. I pushed that change. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v3]
> 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. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: process reviewer comment (simplify call) - Changes: - all: https://git.openjdk.java.net/jfx/pull/683/files - new: https://git.openjdk.java.net/jfx/pull/683/files/b3fe2756..1a25a6d5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=683=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=683=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/683.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/683/head:pull/683 PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v2]
On Fri, 3 Dec 2021 10:20:43 GMT, Ajit Ghaisas wrote:
>> Johan Vos has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Move functionality in the setCellCount() to the invalidated block.
>> 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.
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
> line 1124:
>
>> 1122: Platform.runLater(() -> {
>> 1123: Toolkit.getToolkit().firePulse();
>> 1124: assertTrue(rt_35395_counter < 7);
>
> 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.
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.
-
PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v2]
On Tue, 30 Nov 2021 11:02:41 GMT, Johan Vos wrote:
>> 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.
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move functionality in the setCellCount() to the invalidated block.
> 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.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 860:
> 858: int cellCount = get();
> 859: resetSizeEstimates();
> 860: recalculateAndImproveEstimatedSize(2);
We can use recalculateEstimatedSize() instead of this method.
The effect is the same improvement with a size of 2.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java
line 1124:
> 1122: Platform.runLater(() -> {
> 1123: Toolkit.getToolkit().firePulse();
> 1124: assertTrue(rt_35395_counter < 7);
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.
-
PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v2]
> 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. Johan Vos has updated the pull request incrementally with one additional commit since the last revision: Move functionality in the setCellCount() to the invalidated block. 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. - Changes: - all: https://git.openjdk.java.net/jfx/pull/683/files - new: https://git.openjdk.java.net/jfx/pull/683/files/1a0ce697..b3fe2756 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=683=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=683=00-01 Stats: 18 lines in 4 files changed: 6 ins; 4 del; 8 mod Patch: https://git.openjdk.java.net/jfx/pull/683.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/683/head:pull/683 PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589 [v2]
On Mon, 29 Nov 2021 17:00:10 GMT, Kevin Rushforth wrote: >> Exactly. This ensures consistent behavior regardless of how a property value >> is set. > > I should add that this looks like a preexisting problem, but one that would > be good to fix if possible. Thanks for catching. I moved the additional code into the invalidated block, at the appropriate places. Note that this caused some changes in the flow at runtime, where the number of evaluations of `updateItem` changed. Those changes are of constant order (and not linear with the amount of cells), and I made some changes to the test as they depended on hard numbers. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
On Mon, 29 Nov 2021 16:59:02 GMT, Kevin Rushforth wrote: >> good catch! the "additional" stuff typically is done in the properties' >> invalidated .. > > Exactly. This ensures consistent behavior regardless of how a property value > is set. I should add that this looks like a preexisting problem, but one that would be good to fix if possible. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
On Mon, 29 Nov 2021 16:55:16 GMT, Jeanette Winzenburg wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java >> line 903: >> >>> 901: recalculateAndImproveEstimatedSize(2); >>> 902: >>> 903: cellCount.set(value); >> >> Can this be implemented in a way that doesn't violate the property contract? >> Since this property is public API, `setCellCount(int)` should just call >> `cellCountProperty().set(int)`. Maybe you can extract the composite >> operation here into a private method and use that instead of >> `setCellCount(int)`. >> >> `position` has the same problem. > > good catch! the "additional" stuff typically is done in the properties' > invalidated .. Exactly. This ensures consistent behavior regardless of how a property value is set. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
On Mon, 29 Nov 2021 14:38:10 GMT, Michael Strauß wrote: >> 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. > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java > line 903: > >> 901: recalculateAndImproveEstimatedSize(2); >> 902: >> 903: cellCount.set(value); > > Can this be implemented in a way that doesn't violate the property contract? > Since this property is public API, `setCellCount(int)` should just call > `cellCountProperty().set(int)`. Maybe you can extract the composite operation > here into a private method and use that instead of `setCellCount(int)`. > > `position` has the same problem. good catch! the "additional" stuff typically is done in the properties' invalidated .. - PR: https://git.openjdk.java.net/jfx/pull/683
Re: RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
On Mon, 29 Nov 2021 11:56:45 GMT, Johan Vos wrote: > 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. modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java line 903: > 901: recalculateAndImproveEstimatedSize(2); > 902: > 903: cellCount.set(value); Can this be implemented in a way that doesn't violate the property contract? Since this property is public API, `setCellCount(int)` should just call `cellCountProperty().set(int)`. Maybe you can extract the composite operation here into a private method and use that instead of `setCellCount(int)`. `position` has the same problem. - PR: https://git.openjdk.java.net/jfx/pull/683
RFR: 8276553: ListView scrollTo() is broken after fix for JDK-8089589
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. - Commit messages: - After (re)setting the number of elements, make sure to do at least some estimation of the total size. Changes: https://git.openjdk.java.net/jfx/pull/683/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=683=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276553 Stats: 13 lines in 3 files changed: 11 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/683.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/683/head:pull/683 PR: https://git.openjdk.java.net/jfx/pull/683
