Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Fri, 10 Dec 2021 14:58:25 GMT, Johan Vos wrote: >> This part was originally copied from the 'VirtualContainerBase'. There was >> this comment: _special-case the 'greater than row count' condition to have >> it be perfectly at position 1_. >> >> Maybe to avoid throwing the IndexOutOfBoundsException or simply to scroll to >> the end. > > I am ok with this behavior, but then it should be specified in the javadoc > for `@parameter index` that the value can be greater than row count, and that > in that case the view will be positioned at the very end. > @kevinrushforth do you have an opinion on this? > This is unrelated to the current fix, so that should not stop this PR. This seems reasonable to me, too. And I agree that it should be specified via a follow-up issue, which will need a CSR. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Mon, 6 Dec 2021 14:58:52 GMT, eduardsdv wrote: >> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last >> element but to the bottom of the last element. > > eduardsdv has updated the pull request incrementally with one additional > commit since the last revision: > > 8276170: Add junit for VirtualFlow.scrollToTop(int) Please review. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Fri, 10 Dec 2021 15:00:56 GMT, Johan Vos wrote: >> Without this line the test fails in the line 1865 with the message >> 'expected:<7> but was:<8>'. >> I think this is because in the line 1850 'listView.scrollTo(99)' is >> executed, which now does not set the position to 1. >> >> It can also be caused by the >> https://bugs.openjdk.java.net/browse/JDK-8277785. When it is solved, this >> call will probably no longer be necessary. > > That is very well possible indeed. Since we estimate the size of the > different cells, it is hard to deterministically put values on the counts. > Therefor, I changed a number of tests from `equals` to `<` . I would > personally suggest not adding the `pulse` here, but rather change the test in > line 1865 with `... < 10 ` to make sure that there is no abnormal amount of > processing. (and maybe also `> 0 ` to make sure that the updateItem is at > least counted once). I removed the Toolkit.getToolkit().firePulse() and adjusted the expected values in the test. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Fri, 10 Dec 2021 13:34:53 GMT, eduardsdv wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
>> line 1584:
>>
>>> 1582: boolean posSet = false;
>>> 1583:
>>> 1584: if (index > getCellCount() - 1) {
>>
>> I agree that the previous code violates the contract that the flow should
>> show the specified cell aligned to the top.
>>
>> I wonder though if this test is needed at all. What is the goal of providing
>> an index that is larger than getCellCount() -1?
>
> This part was originally copied from the 'VirtualContainerBase'. There was
> this comment: _special-case the 'greater than row count' condition to have it
> be perfectly at position 1_.
>
> Maybe to avoid throwing the IndexOutOfBoundsException or simply to scroll to
> the end.
I am ok with this behavior, but then it should be specified in the javadoc for
`@parameter index` that the value can be greater than row count, and that in
that case the view will be positioned at the very end.
@kevinrushforth do you have an opinion on this?
This is unrelated to the current fix, so that should not stop this PR.
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java
>> line 1844:
>>
>>> 1842: }
>>> 1843: listView.setPrefHeight(130); // roughly room for four rows
>>> 1844: Toolkit.getToolkit().firePulse();
>>
>> Why is this needed here?
>
> Without this line the test fails in the line 1865 with the message
> 'expected:<7> but was:<8>'.
> I think this is because in the line 1850 'listView.scrollTo(99)' is executed,
> which now does not set the position to 1.
>
> It can also be caused by the
> https://bugs.openjdk.java.net/browse/JDK-8277785. When it is solved, this
> call will probably no longer be necessary.
That is very well possible indeed. Since we estimate the size of the different
cells, it is hard to deterministically put values on the counts. Therefor, I
changed a number of tests from `equals` to `<` . I would personally suggest not
adding the `pulse` here, but rather change the test in line 1865 with `... < 10
` to make sure that there is no abnormal amount of processing. (and maybe also
`> 0 ` to make sure that the updateItem is at least counted once).
-
PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Fri, 10 Dec 2021 12:33:29 GMT, Johan Vos wrote: >> eduardsdv has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8276170: Add junit for VirtualFlow.scrollToTop(int) > > modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java > line 1844: > >> 1842: } >> 1843: listView.setPrefHeight(130); // roughly room for four rows >> 1844: Toolkit.getToolkit().firePulse(); > > Why is this needed here? Without this line the test fails in the line 1865 with the message 'expected:<7> but was:<8>'. I think this is because in the line 1850 'listView.scrollTo(99)' is executed, which now does not set the position to 1. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Fri, 10 Dec 2021 12:33:19 GMT, Johan Vos wrote:
>> eduardsdv has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8276170: Add junit for VirtualFlow.scrollToTop(int)
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
> line 1584:
>
>> 1582: boolean posSet = false;
>> 1583:
>> 1584: if (index > getCellCount() - 1) {
>
> I agree that the previous code violates the contract that the flow should
> show the specified cell aligned to the top.
>
> I wonder though if this test is needed at all. What is the goal of providing
> an index that is larger than getCellCount() -1?
This part was originally copied from the 'VirtualContainerBase'. There was this
comment: _special-case the 'greater than row count' condition to have it be
perfectly at position 1_.
Maybe to avoid throwing the IndexOutOfBoundsException or simply to scroll to
the end.
-
PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Mon, 6 Dec 2021 14:58:52 GMT, eduardsdv wrote:
>> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last
>> element but to the bottom of the last element.
>
> eduardsdv has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8276170: Add junit for VirtualFlow.scrollToTop(int)
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 1584:
> 1582: boolean posSet = false;
> 1583:
> 1584: if (index > getCellCount() - 1) {
I agree that the previous code violates the contract that the flow should show
the specified cell aligned to the top.
I wonder though if this test is needed at all. What is the goal of providing an
index that is larger than getCellCount() -1?
modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewKeyInputTest.java
line 1844:
> 1842: }
> 1843: listView.setPrefHeight(130); // roughly room for four rows
> 1844: Toolkit.getToolkit().firePulse();
Why is this needed here?
-
PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Mon, 6 Dec 2021 14:58:52 GMT, eduardsdv wrote: >> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last >> element but to the bottom of the last element. > > eduardsdv has updated the pull request incrementally with one additional > commit since the last revision: > > 8276170: Add junit for VirtualFlow.scrollToTop(int) Fix looks good. I verified that the tests fail without fix and pass with it. - Marked as reviewed by aghaisas (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
On Mon, 6 Dec 2021 14:58:52 GMT, eduardsdv wrote: >> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last >> element but to the bottom of the last element. > > eduardsdv has updated the pull request incrementally with one additional > commit since the last revision: > > 8276170: Add junit for VirtualFlow.scrollToTop(int) I added a junit which fails without patch and succeeds with it. - PR: https://git.openjdk.java.net/jfx/pull/656
Re: RFR: 8276167: VirtualFlow.scrollToTop doesn't scroll to the top of the last element [v3]
> Fix VirtualFlow.scrollToTop(int) doesn't scroll to the top of the last > element but to the bottom of the last element. eduardsdv has updated the pull request incrementally with one additional commit since the last revision: 8276170: Add junit for VirtualFlow.scrollToTop(int) - Changes: - all: https://git.openjdk.java.net/jfx/pull/656/files - new: https://git.openjdk.java.net/jfx/pull/656/files/a7820448..588a7e41 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=656&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=656&range=01-02 Stats: 37 lines in 1 file changed: 37 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/656.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/656/head:pull/656 PR: https://git.openjdk.java.net/jfx/pull/656
