On Fri, 10 Dec 2021 13:34:53 GMT, eduardsdv <d...@openjdk.java.net> 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
