8186188: TableColumHeader: initial auto-size broken if has graphic #1405

Maran23 · 2024-03-16T15:47:18Z

This PR fixes the issue that the initial column autosizing is wrong under some circumstances.
The following things will break the initial autosizing:

Bold Column text (that is where I initially found this problem)
Another font / font size
Graphic
Graphic Content Display (which can be styled via CSS)

The reason is actually quite simple: The CSS is not (yet) applied initially, we therefore ALWAYS take the default font into account + the graphic is not yet layouted as well.

It was not so easy to write tests for this, also for me the test_resizeColumnToFitContentHeader is always failing locally. I don't know what happens here, but he seems to not find a (Stub?) Font for me.
EDIT: Found out the cause and fixed it. I will check if I can write more tests since it works now. :)

The test I wrote now is checking if the css is applied after we triggered the autosize, which is what we would expect here since we measure text.

I also copied the TableColumnHeaderTest and rewrote the tests for TreeTableView as well, so we can catch any errors here as well since they both use different code (although it is technically the same - C&P errors can happen very easy).

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8186188: TableColumHeader: initial auto-size broken if has graphic (Bug - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Robert Lichtenberger (@effad - Author)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1405/head:pull/1405
$ git checkout pull/1405

Update a local copy of the PR:
$ git checkout pull/1405
$ git pull https://git.openjdk.org/jfx.git pull/1405/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1405

View PR using the GUI difftool:
$ git pr show -t 1405

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1405.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-16T15:47:51Z

👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-03-16T15:48:08Z

@Maran23 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8186188: TableColumHeader: initial auto-size broken if has graphic

Reviewed-by: angorya, rlichten

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 16 new commits pushed to the master branch:

0eb4d71: 8329705: Add missing Application thread checks to platform specific a11y methods
0d33606: 8324939: Editable TableView loses focus after commit
483c640: 8328752: Fix missing @OVERRIDES in javafx.web
c41b000: 8328742: Remove unused imports in manual tests
c08a6b7: 8328745: Remove unused imports in system tests
4e0a229: 8328718: Remove unused imports in javafx.controls
01a1d90: 8328746: Remove unused imports in demo apps
edbb88f: 8328820: Remove unused imports in javafx.swing
3761d37: 8328739: Remove unused imports in javafx.graphics
a85af13: 8328749: Remove unused imports in javafx.web
... and 6 more: https://git.openjdk.org/jfx/compare/6adbcffafd15f9f771c09afb03649e83e9e0b02a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2024-03-16T22:30:29Z

Webrevs

…the width

…ed with Amble)

… the font is now always there

andy-goryachev-oracle

the main question is whether this PR depends on #1422, or whether the unrelated changes need to be removed.

andy-goryachev-oracle · 2024-03-22T22:55:06Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/TableColumnHeader.java

-        double headerGraphicWidth = graphic == null ? 0 : graphic.prefWidth(-1) + header.label.getGraphicTextGap();
-        double headerWidth = headerTextWidth + headerGraphicWidth + 10 + header.snappedLeftInset() + header.snappedRightInset();
+        header.applyCss();
+        double headerWidth = header.label.prefWidth(-1) + 10;


should we extract this magic constant (along with the comment)?
although this might be outside of the scope of this PR, lines 651, 744.

andy-goryachev-oracle · 2024-03-22T22:56:44Z

modules/javafx.controls/src/test/java/test/javafx/scene/chart/AreaChartTest.java

                .map(lineTo -> new Point2D(
-                        xAxis.getValueForDisplay(lineTo.getX()).doubleValue(),
-                        yAxis.getValueForDisplay(lineTo.getY()).doubleValue())
+                        Math.round(xAxis.getValueForDisplay(lineTo.getX()).doubleValue()),


please revert the unrelated changes (moved to #1422)

andy-goryachev-oracle · 2024-03-22T22:57:07Z

modules/javafx.controls/src/test/java/test/javafx/scene/chart/StackedBarChartTest.java

        String bounds = computeBoundsString((Region)childrenList.get(0), (Region)childrenList.get(1),
                (Region)childrenList.get(2));
-        assertEquals("10 478 234 37 254 432 234 83 499 375 234 140 ", bounds);
+        assertEquals("10 453 218 35 238 409 218 79 465 355 218 133 ", bounds);


andy-goryachev-oracle · 2024-03-22T22:57:41Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java

        Scene s = new Scene(textInput);
        textInput.applyCss();
-        assertEquals(Font.font("Helvetica", 24), textInput.getFont());
+        assertEquals(Font.font("Amble", 24), textInput.getFont());


andy-goryachev-oracle · 2024-03-22T22:59:34Z

...fx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTreeTableTest.java

+import javafx.scene.input.MouseEvent;
+import javafx.scene.layout.HBox;
+import javafx.scene.text.Text;
+import org.junit.After;


this is a new test - should we use JUnit5?

I copied the other test class, but sure, is probably better to switch this to JUnit 5 then.

andy-goryachev-oracle · 2024-03-22T23:00:57Z

...fx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTreeTableTest.java

+
+    private TableColumnHeader firstColumnHeader;
+    private TreeTableView<Person> tableView;
+    private StageLoader sl;


minor: this field should probably be named stageLoader

andy-goryachev-oracle · 2024-03-22T23:02:57Z

...fx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTreeTableTest.java

+        TableColumnHeaderShim.resizeColumnToFitContent(firstColumnHeader, -1);
+
+        assertEquals("Width must be the same",
+                width, column.getWidth(), 0.001);


maybe a constant, here and elsewhere

private static final double EPSILON = 0.001;

?

andy-goryachev-oracle · 2024-03-22T23:07:05Z

modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubFontLoader.java

-        if (name.equals("system") || name.equals("system regular")) {
+        if (name.equals("system regular")) {
            FontHelper.setNativeFont(font, nativeFont, font.getName(), "System", "Regular");
+        } else if (name.equals("system bold")) {


does this mean this PR requires #1422 to be integrated first?

Yes, that would be easier for reviewing this. Once merged, I will update this branch. :)

kevinrushforth · 2024-03-25T19:01:07Z

/reviewers 2

openjdk · 2024-03-25T19:01:37Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Maran23 · 2024-03-27T22:29:02Z

@effad Since you benchmarked this method in #1358, could you do that again with this changes?

…188-tc-init-size # Conflicts: # modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubFontLoader.java

effad · 2024-04-02T07:37:15Z

@effad Since you benchmarked this method in #1358, could you do that again with this changes?

Sorry for the late reply, I just returned from easter holidays :-). I'll try to benchmark this change within the next days...

effad

As per request I tested the changes of this PR with the Benchmark found at https://gist.github.com/effad/9eebee0c1e86a8e605cb55ced9485dd4

Baseline: JFX 23-internal+0-2024-04-02-130613 average run time: 975
After gh pr checkout 1405: JFX 23-internal+0-2024-04-02-130613 average run time: 1026

So we have a measurable (~5%) but (IMO) neglectable slowdown. Compared to JFX 21.0.2+5 (2460) we are still more than twice as fast.

Maran23 · 2024-04-07T13:17:09Z

@effad Thank you, much appreciated! Good to see that this does not cause any major performance problems.

andy-goryachev-oracle

Works as expected with the code in the ticket, no issues found with the monkey tester.
Thanks!

Maran23 · 2024-04-12T07:58:13Z

/integrate

openjdk · 2024-04-12T07:59:26Z

Going to push as commit 44d53ba.
Since your change was applied there have been 16 commits pushed to the master branch:

0eb4d71: 8329705: Add missing Application thread checks to platform specific a11y methods
0d33606: 8324939: Editable TableView loses focus after commit
483c640: 8328752: Fix missing @OVERRIDES in javafx.web
c41b000: 8328742: Remove unused imports in manual tests
c08a6b7: 8328745: Remove unused imports in system tests
4e0a229: 8328718: Remove unused imports in javafx.controls
01a1d90: 8328746: Remove unused imports in demo apps
edbb88f: 8328820: Remove unused imports in javafx.swing
3761d37: 8328739: Remove unused imports in javafx.graphics
a85af13: 8328749: Remove unused imports in javafx.web
... and 6 more: https://git.openjdk.org/jfx/compare/6adbcffafd15f9f771c09afb03649e83e9e0b02a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-12T07:59:35Z

@Maran23 Pushed as commit 44d53ba.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8186188: TableColumHeader: initial auto-size broken if has graphic

63a74ad

openjdk bot added the rfr Ready for review label Mar 16, 2024

Maran23 added 6 commits March 20, 2024 12:30

JDK-8186188: Use label prefWidth(-1) instead of manually calculating …

f68fcca

…the width

JDK-8186188: fix tests by considering the system font (was always mix…

c2a3279

…ed with Amble)

JDK-8186188: prefWidth(-1) for TreeTableView as well

a8d26a2

JDK-8186188: add more tests, fix existing tests which were failing as…

9423af0

… the font is now always there

JDK-8186188: fix tests

ece98e0

JDK-8186188: copyright

554c8ed

Maran23 mentioned this pull request Mar 21, 2024

JDK-8328750: [TestBug] Improve Stub Font Support #1422

Closed

3 tasks

andy-goryachev-oracle reviewed Mar 22, 2024

View reviewed changes

Maran23 added 3 commits March 27, 2024 23:59

Merge branch 'master' of https://github.com/openjdk/jfx into JDK-8186…

53acbc5

…188-tc-init-size # Conflicts: # modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubFontLoader.java

improve tests + JUnit 5 for new test

b4d03d4

use snapped insets

9c51696

effad approved these changes Apr 2, 2024

View reviewed changes

openjdk bot changed the title ~~JDK-8186188: TableColumHeader: initial auto-size broken if has graphic~~ 8186188: TableColumHeader: initial auto-size broken if has graphic Apr 10, 2024

andy-goryachev-oracle approved these changes Apr 11, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Apr 11, 2024

openjdk bot added the integrated Pull request has been integrated label Apr 12, 2024

openjdk bot closed this Apr 12, 2024

openjdk bot removed the ready Ready to be integrated label Apr 12, 2024

openjdk bot removed the rfr Ready for review label Apr 12, 2024

Maran23 deleted the JDK-8186188-tc-init-size branch August 11, 2024 13:36

8186188: TableColumHeader: initial auto-size broken if has graphic #1405

8186188: TableColumHeader: initial auto-size broken if has graphic #1405

Uh oh!

Conversation

Maran23 commented Mar 16, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 16, 2024

Uh oh!

openjdk bot commented Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Mar 25, 2024

Uh oh!

openjdk bot commented Mar 25, 2024

Uh oh!

Maran23 commented Mar 27, 2024

Uh oh!

effad commented Apr 2, 2024

Uh oh!

effad left a comment

Choose a reason for hiding this comment

Uh oh!

Maran23 commented Apr 7, 2024

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Maran23 commented Apr 12, 2024

Uh oh!

openjdk bot commented Apr 12, 2024

Uh oh!

openjdk bot commented Apr 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Maran23 commented Mar 16, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 16, 2024 •

edited

Loading

mlbridge bot commented Mar 16, 2024 •

edited

Loading