On Thu, 8 Jun 2023 07:18:22 GMT, Marius Hanl <[email protected]> wrote:
>> This PR does two small improvements to `VirtualFlow`.
>> Until now, the `VirtualFlow` sometimes called the `computeHeight` or
>> `computeWidth` methods, although a fixed cell size is set and we therefore
>> don't need to call those method (and never should do, as we may don't get
>> the expected result and mix computed sizes with the fixed cell size).
>>
>> Added tests that fail before and pass now. They check that the
>> `computeHeight` or `computeWidth` (non vertical flow) are never called when
>> a fixed cell size is set.
>
> Marius Hanl has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JDK-8309470: Improved Tests
Tested the changes in MacOS 13.3.1, I'm seeing failure of
`testComputeHeightShouldNotBeCalledWhenFixedCellSizeIsSet` and
`testComputeWidthShouldNotBeCalledWhenFixedCellSizeIsSet` with and without the
changes of this PR.
VirtualFlowTest > testComputeHeightShouldNotBeCalledWhenFixedCellSizeIsSet
FAILED
java.lang.AssertionError: expected:<24.0> but was:<1337.0>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:555)
at org.junit.Assert.assertEquals(Assert.java:685)
at
test.javafx.scene.control.skin.VirtualFlowTest.testComputeHeightShouldNotBeCalledWhenFixedCellSizeIsSet(VirtualFlowTest.java:1566)
VirtualFlowTest > testComputeWidthShouldNotBeCalledWhenFixedCellSizeIsSet FAILED
java.lang.AssertionError: expected:<24.0> but was:<1337.0>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:555)
at org.junit.Assert.assertEquals(Assert.java:685)
at
test.javafx.scene.control.skin.VirtualFlowTest.testComputeWidthShouldNotBeCalledWhenFixedCellSizeIsSet(VirtualFlowTest.java:1622)
Changes requested by kpk (Committer).
modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
line 3065:
> 3063: }
> 3064:
> 3065: // if we have a valid cell, we can populate the cache
Do we need this comment now?
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/VirtualFlowTest.java
line 1542:
> 1540: @Override
> 1541: protected double computeMinHeight(double width) {
> 1542: return 1337;
Can we add a final variable for this number since it is repeated many times?
-------------
Changes requested by kpk (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1150#pullrequestreview-1469946391
PR Review: https://git.openjdk.org/jfx/pull/1150#pullrequestreview-1469948040
PR Review Comment: https://git.openjdk.org/jfx/pull/1150#discussion_r1223061740
PR Review Comment: https://git.openjdk.org/jfx/pull/1150#discussion_r1223062537
