On Wed, 20 Mar 2024 19:53:40 GMT, Marius Hanl <mh...@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
