RFR: JDK-8186188: TableColumHeader: initial auto-size broken if has graphic [v4]

[Andy Goryachev]

On Wed, 20 Mar 2024 19:53:40 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>> 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
>> 
>> 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).
>
> Marius Hanl has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - JDK-8186188: copyright
>  - JDK-8186188: fix tests

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

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

> 684:         TableColumnHeader header = tableSkin.getTableHeaderRow().getColumnHeaderFor(tc);
> 685:         header.applyCss();
> 686:         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.

modules/javafx.controls/src/test/java/test/javafx/scene/chart/AreaChartTest.java line 537:

> 535:                 .map(pathElement -> (LineTo) pathElement)
> 536:                 .map(lineTo -> new Point2D(
> 537:                         Math.round(xAxis.getValueForDisplay(lineTo.getX()).doubleValue()),

please revert the unrelated changes (moved to #1422)

modules/javafx.controls/src/test/java/test/javafx/scene/chart/StackedBarChartTest.java line 111:

> 109:         String bounds = computeBoundsString((Region)childrenList.get(0), (Region)childrenList.get(1),
> 110:                 (Region)childrenList.get(2));
> 111:         assertEquals("10 453 218 35 238 409 218 79 465 355 218 133 ", bounds);

unrelated

modules/javafx.controls/src/test/java/test/javafx/scene/control/TextInputControlTest.java line 158:

> 156:         Scene s = new Scene(textInput);
> 157:         textInput.applyCss();
> 158:         assertEquals(Font.font("Amble", 24), textInput.getFont());

unrelated

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTreeTableTest.java line 48:

> 46: import javafx.scene.layout.HBox;
> 47: import javafx.scene.text.Text;
> 48: import org.junit.After;

this is a new test - should we use JUnit5?

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTreeTableTest.java line 64:

> 62:     private TableColumnHeader firstColumnHeader;
> 63:     private TreeTableView<Person> tableView;
> 64:     private StageLoader sl;

minor: this field should probably be named `stageLoader`

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/TableColumnHeaderTreeTableTest.java line 153:

> 151: 
> 152:         assertEquals("Width must be the same",
> 153:                 width, column.getWidth(), 0.001);

maybe a constant, here and elsewhere

private static final double EPSILON = 0.001;

?

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

> 51:         if (name.equals("system regular")) {
> 52:             FontHelper.setNativeFont(font, nativeFont, font.getName(), "System", "Regular");
> 53:         } else if (name.equals("system bold")) {

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

-------------

PR Review: https://git.openjdk.org/jfx/pull/1405#pullrequestreview-1955910101
PR Review Comment: https://git.openjdk.org/jfx/pull/1405#discussion_r1536295365
PR Review Comment: https://git.openjdk.org/jfx/pull/1405#discussion_r1536296972
PR Review Comment: https://git.openjdk.org/jfx/pull/1405#discussion_r1536297127
PR Review Comment: https://git.openjdk.org/jfx/pull/1405#discussion_r1536297778
PR Review Comment: https://git.openjdk.org/jfx/pull/1405#discussion_r1536298798
PR Review Comment: https://git.openjdk.org/jfx/pull/1405#discussion_r1536299480
PR Review Comment: https://git.openjdk.org/jfx/pull/1405#discussion_r1536300462
PR Review Comment: https://git.openjdk.org/jfx/pull/1405#discussion_r1536315771