Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Tue, 8 Aug 2023 23:44:58 GMT, Jose Pereda wrote:
>> So far, BorderPane does the calculation for the children min/pref
>> width/height taken into account only the margin applied to them, if any, but
>> not the total padding that could be applied as well to the BorderPane itself.
>>
>> However, this padding needs to be taken into account as well, and this PR
>> modifies BorderPane to subtract its insets from its size while doing the
>> children min/pref width/height calculations.
>>
>> A parameterized test has been included.
>>
>> It is a simplified version of the test case attached to
>> https://bugs.openjdk.org/browse/JDK-8313709, but still shows how without
>> this patch, two of the cases (padding with or without margin) fail, while
>> pass with it.
>
> Jose Pereda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Migrate old tests to JUnit 5
modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line
413:
> 411: @Override protected double computeMinHeight(double width) {
> 412: final Insets insets = getInsets();
> 413: if (width != -1) {
You could use the constant `Region.USE_COMPUTED_SIZE` here to make the intent
here more clear
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1379931322
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Thu, 26 Oct 2023 00:31:52 GMT, Michael Strauß wrote:
>> Jose Pereda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Migrate old tests to JUnit 5
>
> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java
> line 414:
>
>> 412: final Insets insets = getInsets();
>> 413: if (width != -1) {
>> 414: width -= (insets.getLeft() + insets.getRight());
>
> Let's say we call `computeMinHeight(10)`, but the left and right insets are
> 20. This means that `width` is now -10, which probably means "ignore the
> value" (the spec isn't entirely clear about that, but -10 is not a valid
> width in any case).
>
> I think the following code might be better:
>
> if (width >= 0) {
> width = Math.max(0, width - insets.getLeft() - insets.getRight());
> }
The `width` value passed to `computeMinHeight` (if not -1) should be the result
of a call to `computeMinWidth(-1)` (depending on the result of
`getContentBias`; this is specified somewhere); if that function always
includes the insets, then subtracting them here should not result in a negative
value.
Of course, one can never be too careful.
I don't like the changing of the meaning of the `width` parameter here though
using parameter assignment.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1372711490
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Fri, 18 Aug 2023 18:04:14 GMT, Andy Goryachev wrote: >> Please, see my edited comment. If we are to change to snapped values >> _everywhere_ (not only in the proposed fix of this PR), that would be a >> broader change... > > no, I did not propose to change it everywhere. > In this PR, in the code we change, we probably should [start to] use snapped > coordinates. wouldn't you agree? Hard to say. Unless it's somehow related to the changes being made, I can see a good argument for addressing the snapping in a follow-up issue, so as not to introduce what is essentially an unrelated fix. - PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1298740921
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Fri, 18 Aug 2023 18:00:48 GMT, Jose Pereda wrote: >> Following the great insight @hjohn vocalized in one of the earlier PRs, at >> the end we are always dealing with integer pixel coordinates. >> >> so, to be correct, any computation that returns pixel coordinates must use >> snapped values. (it also means that any computation that does not result in >> pixel coordinates, might use unsnapped values, such as when we try to >> distribute a single pixel among many columns or nodes). > > Please, see my edited comment. If we are to change to snapped values > _everywhere_ (not only in the proposed fix of this PR), that would be a > broader change... no, I did not propose to change it everywhere. In this PR, in the code we change, we probably should [start to] use snapped coordinates. wouldn't you agree? - PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1298721309
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Fri, 18 Aug 2023 17:48:16 GMT, Andy Goryachev wrote:
>> You might be right.
>>
>> In this case, `topPrefHeight` comes from `getAreaHeight()`, that calls
>> `computeChildPrefAreaHeight()` that ultimately uses `snapSpaceY()`.
>>
>> However, this would also mean that the returned value should use snapped
>> insets as well?
>>
>> return insets.getLeft() +
>> Math.max(leftPrefWidth + centerMinWidth + rightPrefWidth,
>> Math.max(topMinWidth,bottomMinWidth)) +
>> insets.getRight();
>>
>> should change into
>>
>> return snappedLeftInset() +
>> Math.max(leftPrefWidth + centerMinWidth + rightPrefWidth,
>> Math.max(topMinWidth, bottomMinWidth)) +
>> snappedRightInset();
>>
>>
>> Checking `AnchorPane`, it does use the latter, but not to get the pref size:
>>
>> private double computeHeight(final boolean minimum, final double width) {
>> double max = 0;
>> double contentWidth = width != -1 ? width - getInsets().getLeft()-
>> getInsets().getRight() : -1;
>> ...
>> return snappedTopInset() + max + snappedBottomInset();
>> }
>
> Following the great insight @hjohn vocalized in one of the earlier PRs, at
> the end we are always dealing with integer pixel coordinates.
>
> so, to be correct, any computation that returns pixel coordinates must use
> snapped values. (it also means that any computation that does not result in
> pixel coordinates, might use unsnapped values, such as when we try to
> distribute a single pixel among many columns or nodes).
Please, see my edited comment. If we are to change to snapped values
_everywhere_ (not only in the proposed fix of this PR), that would be a broader
change...
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1298718488
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Fri, 18 Aug 2023 17:34:28 GMT, Jose Pereda wrote: >> modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java >> line 395: >> >>> 393: >>> 394: double middleAreaHeight = Math.max(0, >>> 395: height - insets.getTop() - insets.getBottom() - >>> topPrefHeight - bottomPrefHeight); >> >> should these be snapped? snappedBottomInset() etc.? > > You might be right. > > In this case, `topPrefHeight` comes from `getAreaHeight()`, that calls > `computeChildPrefAreaHeight()` that ultimately uses `snapSpaceY()`. > > However, this would also mean that the returned value should use snapped > insets as well? > > return insets.getLeft() + > Math.max(leftPrefWidth + centerMinWidth + rightPrefWidth, > Math.max(topMinWidth,bottomMinWidth)) + > insets.getRight(); > > and also for the layout call? Following the great insight @hjohn vocalized in one of the earlier PRs, at the end we are always dealing with integer pixel coordinates. so, to be correct, any computation that returns pixel coordinates must use snapped values. (it also means that any computation that does not result in pixel coordinates, might use unsnapped values, such as when we try to distribute a single pixel among many columns or nodes). - PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1298708502
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Wed, 16 Aug 2023 21:26:21 GMT, Andy Goryachev wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Migrate old tests to JUnit 5 > > modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java > line 395: > >> 393: >> 394: double middleAreaHeight = Math.max(0, >> 395: height - insets.getTop() - insets.getBottom() - >> topPrefHeight - bottomPrefHeight); > > should these be snapped? snappedBottomInset() etc.? You might be right. In this case, `topPrefHeight` comes from `getAreaHeight()`, that calls `computeChildPrefAreaHeight()` that ultimately uses `snapSpaceY()`. However, this would also mean that the returned value should use snapped insets as well? return insets.getLeft() + Math.max(leftPrefWidth + centerMinWidth + rightPrefWidth, Math.max(topMinWidth,bottomMinWidth)) + insets.getRight(); and also for the layout call? - PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1298697529
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Tue, 8 Aug 2023 23:44:58 GMT, Jose Pereda wrote: >> So far, BorderPane does the calculation for the children min/pref >> width/height taken into account only the margin applied to them, if any, but >> not the total padding that could be applied as well to the BorderPane itself. >> >> However, this padding needs to be taken into account as well, and this PR >> modifies BorderPane to subtract its insets from its size while doing the >> children min/pref width/height calculations. >> >> A parameterized test has been included. >> >> It is a simplified version of the test case attached to >> https://bugs.openjdk.org/browse/JDK-8313709, but still shows how without >> this patch, two of the cases (padding with or without margin) fail, while >> pass with it. > > Jose Pereda has updated the pull request incrementally with one additional > commit since the last revision: > > Migrate old tests to JUnit 5 FYI: the test code shows 2x2 arrangement on windows at 100%, but 1x4 arrangement at 125%. Is this expected? In both cases scrolling and resizing seems ok. - PR Comment: https://git.openjdk.org/jfx/pull/1203#issuecomment-1681333792
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Tue, 8 Aug 2023 23:44:58 GMT, Jose Pereda wrote:
>> So far, BorderPane does the calculation for the children min/pref
>> width/height taken into account only the margin applied to them, if any, but
>> not the total padding that could be applied as well to the BorderPane itself.
>>
>> However, this padding needs to be taken into account as well, and this PR
>> modifies BorderPane to subtract its insets from its size while doing the
>> children min/pref width/height calculations.
>>
>> A parameterized test has been included.
>>
>> It is a simplified version of the test case attached to
>> https://bugs.openjdk.org/browse/JDK-8313709, but still shows how without
>> this patch, two of the cases (padding with or without margin) fail, while
>> pass with it.
>
> Jose Pereda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Migrate old tests to JUnit 5
modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line
395:
> 393:
> 394: double middleAreaHeight = Math.max(0,
> 395: height - insets.getTop() - insets.getBottom() -
> topPrefHeight - bottomPrefHeight);
should these be snapped? snappedBottomInset() etc.?
modules/javafx.graphics/src/main/java/javafx/scene/layout/BorderPane.java line
415:
> 413: if (width != -1) {
> 414: width -= (insets.getLeft() + insets.getRight());
> 415: }
ditto here
modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderPaneTest.java
line 938:
> 936: @ParameterizedTest
> 937: public void testFlowPaneCenterChildWithPaddingAndMargin(double
> width, double minHeight, boolean useMargin, boolean usePadding) {
> 938: FlowPane center = new FlowPane(10, 10);
I wonder if we should also test the layout-related code using fractional scale
+ snapping.
what do you think?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1296446580
PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1296447169
PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1296450320
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
On Tue, 8 Aug 2023 12:25:53 GMT, Kevin Rushforth wrote: >> Jose Pereda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Migrate old tests to JUnit 5 > > modules/javafx.graphics/src/test/java/test/javafx/scene/layout/BorderPaneTest.java > line 40: > >> 38: import org.junit.Test; >> 39: import org.junit.jupiter.params.ParameterizedTest; >> 40: import org.junit.jupiter.params.provider.CsvSource; > > We don't want to mix JUnit4 and JUnit5 API calls in the same test class file. Fixed, migrated the existing tests to JUnit 5 - PR Review Comment: https://git.openjdk.org/jfx/pull/1203#discussion_r1287786105
Re: RFR: 8313709: Wrong layout of a FlowPane in a BorderPane in a ScrollPane, if padding is too big [v2]
> So far, BorderPane does the calculation for the children min/pref > width/height taken into account only the margin applied to them, if any, but > not the total padding that could be applied as well to the BorderPane itself. > > However, this padding needs to be taken into account as well, and this PR > modifies BorderPane to subtract its insets from its size while doing the > children min/pref width/height calculations. > > A parameterized test has been included. > > It is a simplified version of the test case attached to > https://bugs.openjdk.org/browse/JDK-8313709, but still shows how without this > patch, two of the cases (padding with or without margin) fail, while pass > with it. Jose Pereda has updated the pull request incrementally with one additional commit since the last revision: Migrate old tests to JUnit 5 - Changes: - all: https://git.openjdk.org/jfx/pull/1203/files - new: https://git.openjdk.org/jfx/pull/1203/files/ce6c0dad..e0c79da5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1203=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1203=00-01 Stats: 9 lines in 1 file changed: 5 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1203.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1203/head:pull/1203 PR: https://git.openjdk.org/jfx/pull/1203
