Re: RFR: JDK-8186188: TableColumHeader: initial auto-size broken if has graphic [v4]
On Wed, 20 Mar 2024 19:53:40 GMT, Marius Hanl 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 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 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
Re: RFR: JDK-8328750: [TestBug] Improve Stub Font Support
On Thu, 21 Mar 2024 22:06:42 GMT, Marius Hanl wrote:
> In https://github.com/openjdk/jfx/pull/1405, I identified some shortcomings
> of the stub font implementation. As I don't want to clutter the PR with that,
> I decided to cherrypick the improvements I did to a new ticket and PR.
>
> The current implementation has the following shortcomings:
> - It does not reliable detect the System Font, as a consequence, tests in
> TableColumnHeaderTest.java are failing on my local machine
> - Another consequence of this is, that the font size is always estimated with
> 0, as it is not detected
> - One shortcoming currently is, that the stub font siie estimate is not
> considering bold fonts. That would improve writing tests for some scenarios,
> e.g. for TableColumnHeader, where we would expect that the size of the header
> is bigger since it is bold
>
> Some tests were failing for the following reasons:
> - `AreaChartTest.java` - `expected -30.0, was -30.004` - I added
> ceiling to the data.
> - `StackedBarChartTest.java` - since we now calculate correctly, the path
> changed
> - A test tried to load `Helvetica`, which is not supported in the stub font
> loader. I changed it
> - The default System font is considered a `Regular` one (style)
>
> I wrote tests and documented the stub behaviour.
> I did some minor changes here:
> - System font is now detected, also in bold and italic
> - A bold font will be calculated with a little bit more width (1px). Checkout
> the test as well for that
>
> Note: This only changes test setup, no 'production' code.
modules/javafx.controls/src/test/java/test/javafx/scene/chart/AreaChartTest.java
line 538:
> 536: .map(lineTo -> new Point2D(
> 537:
> Math.ceil(xAxis.getValueForDisplay(lineTo.getX()).doubleValue()),
> 538:
> Math.ceil(yAxis.getValueForDisplay(lineTo.getY()).doubleValue()))
would it make more sense to either do a Math.round(), or better yet - write a
utility that computes array equality of Point2D's with some non-zero tolerance?
Do we have more tests like this that might warrant a new utility?
modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/StubFontContractTest.java
line 21:
> 19: * @see test.com.sun.javafx.pgstub.StubFontLoader
> 20: * @see test.com.sun.javafx.pgstub.StubTextLayout
> 21: */
thank you for explaining what the test is trying to do (and not trying to do)!
modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubFontLoader.java
line 57:
> 55: case "system italic" -> FontHelper.setNativeFont(font,
> nativeFont, font.getName(), "System", "Italic");
> 56: case "system bold italic" ->
> 57: FontHelper.setNativeFont(font, nativeFont,
> font.getName(), "System", "Bold Italic");
minor suggestions:
font -> f
nativeFont -> nf
this will nicely fit each case on a single line (and reduce clutter)
modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubFontLoader.java
line 76:
> 74: FontHelper.setNativeFont(font, nativeFont,
> font.getName(), "Amble LtCn", "Regular");
> 75: case "amble light condensed italic" ->
> 76: FontHelper.setNativeFont(font, nativeFont,
> font.getName(), "Amble LtCn", "Italic");
does it make sense to handle/log the default case, or this is indeed the
desired code (i.e. a no-op)?
modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubTextLayout.java
line 102:
> 100: boolean bold =
> font.getStyle().toLowerCase().contains("bold");
> 101: if (bold) {
> 102: fontSize++;
should this only apply to width computation (line 128)?
in other words, the height of for bold text of size 12 should have the same
height as plain text size 12, shouldn't it?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1422#discussion_r1536280082
PR Review Comment: https://git.openjdk.org/jfx/pull/1422#discussion_r1536280361
PR Review Comment: https://git.openjdk.org/jfx/pull/1422#discussion_r1536281156
PR Review Comment: https://git.openjdk.org/jfx/pull/1422#discussion_r1536281935
PR Review Comment: https://git.openjdk.org/jfx/pull/1422#discussion_r1536284428
Re: RFC: Screen capturing utility (for failed headful tests)
I like this idea in general. It would be a good thing to consider for a future test sprint. I have a few high-level questions on the specifics: 1. I've not used JUnit5 TestWatcher at all, but it seems like it might do what we want. Does anyone else have practical experience with it? For cases where a test passes or is skipped, is there any impact on the report? 2. Even if we do annotate all our JUnit5 headful robot tests with this, I would want to qualify that on a system property -- probably off by default. I presume this should be as easy as having the "onFailed" (or whatever it's called) method of the watcher check this property and do nothing if it isn't enabled? 3. Depending on how the test exits, it's quite possible that the JavaFX runtime has shutdown. It might be a bit tricky to do this in a robust manner without doing an exec of a new Java process. Any thoughts on this? 4. In answer to the problem of what happens if all too many tests fail (flooding the log), maybe you could have a system property that sets a max number of images to log? That might be hard to implement, though, unless you store some information in the build directory of the system tests, since each system test necessarily runs in a separate Java process. 5. Is writing to the log (e.g., System.err) the best way to capture the data? Whatever we do, we will want some easy way to extract and display the screenshots. If we do move forward with this, and use the JUnit5 TestWatcher facility, this will be one more motivation to drive the conversion of tests from JUnit4 to JUnit5 -- at least in the system tests project. See JDK-8328629 [2]. -- Kevin [2] https://bugs.openjdk.org/browse/JDK-8328629 On 3/22/2024 1:49 PM, Andy Goryachev wrote: Dear colleagues: What do you think of the following idea: The Problem === Automated headful tests might fail intermittently or on specific platform(s). After the fact, it's nearly impossible to understand why this happened. Solution Implement a JUnit5 TestWatcher which captures the primary screen pixels and dumps them as base64-encoded PNG images in stderr (or stdout). The logs are typically preserved, so the screenshots can be analyzed after the fact. As far as I know, there is no way to configure JUnit to apply this to each test using command line option, so each test needs to have the following annotation added: @ExtendWith(ScreenCaptureTestWatcher.class) For more details please see this ticket [0] and the draft PR [1]. Possible Issues === * the size of the image can be quite large, for example retina mac increases the size of the log by approximately 4MB (for each image). * a misconfiguration might cause every test to fail, resulting in very large logs References == [0] https://bugs.openjdk.org/browse/JDK-8328716 [1] https://github.com/openjdk/jfx/pull/1415 What do you think? Cheerio, -andy
RFC: Screen capturing utility (for failed headful tests)
Dear colleagues: What do you think of the following idea: The Problem === Automated headful tests might fail intermittently or on specific platform(s). After the fact, it's nearly impossible to understand why this happened. Solution Implement a JUnit5 TestWatcher which captures the primary screen pixels and dumps them as base64-encoded PNG images in stderr (or stdout). The logs are typically preserved, so the screenshots can be analyzed after the fact. As far as I know, there is no way to configure JUnit to apply this to each test using command line option, so each test needs to have the following annotation added: @ExtendWith(ScreenCaptureTestWatcher.class) For more details please see this ticket [0] and the draft PR [1]. Possible Issues === * the size of the image can be quite large, for example retina mac increases the size of the log by approximately 4MB (for each image). * a misconfiguration might cause every test to fail, resulting in very large logs References == [0] https://bugs.openjdk.org/browse/JDK-8328716 [1] https://github.com/openjdk/jfx/pull/1415 What do you think? Cheerio, -andy
Re: RFR: 8328718: Remove unused imports in javafx.controls
On Thu, 21 Mar 2024 18:47:24 GMT, Andy Goryachev wrote: > Using Eclipse IDE to remove unused imports in javafx.controls and update the > copyright year to 2024. Using wildcard for more than 10 static imports. > > This is a trivial change, 1 reviewer is probably enough. I can, but it will take me some time to get to it. - PR Comment: https://git.openjdk.org/jfx/pull/1416#issuecomment-2015727802
Re: RFR: 8328718: Remove unused imports in javafx.controls
On Thu, 21 Mar 2024 18:47:24 GMT, Andy Goryachev wrote: > Using Eclipse IDE to remove unused imports in javafx.controls and update the > copyright year to 2024. Using wildcard for more than 10 static imports. > > This is a trivial change, 1 reviewer is probably enough. @Maran23 @nlisker would you like to review this PR and its relatives? - PR Comment: https://git.openjdk.org/jfx/pull/1416#issuecomment-2015710063
Re: RFR: 8316372: Monkey Tester Application Part 3 [v3]
> Further changes to the MonkeyTester application: > > - remember split pane divider ✔ > - use 'private' instead of 'protected' in many cases ✔ > - added more scripts to the 'writing systems' text sample ✔ > - added RTL window control menu ✔ > - added embedded swing/fx in tools ✔ > - added copy popup menu in clipboard viewer ✔ > - added the custom css field to the css playground tool ✔ > - added many new pages ✔ > - split XYChartPage into separate pages ✔ > - switched to use property sheets (some choices might be incomplete) ✔ > > https://github.com/andy-goryachev-oracle/jfx/blob/8316372.monkey/tests/manual/monkey/README.md Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - review comments - Merge branch 'master' into 8316372.monkey - review comments - whitespace - tool bar - Merge branch 'master' into 8316372.monkey - whitespace - whitespace - whitespace - 8316372: Monkey Tester Application Part 3 - Changes: - all: https://git.openjdk.org/jfx/pull/1406/files - new: https://git.openjdk.org/jfx/pull/1406/files/2c23ca57..c472fd0c Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1406=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1406=01-02 Stats: 224 lines in 49 files changed: 32 ins; 77 del; 115 mod Patch: https://git.openjdk.org/jfx/pull/1406.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1406/head:pull/1406 PR: https://git.openjdk.org/jfx/pull/1406
Re: RFR: 8316372: Monkey Tester Application Part 3 [v2]
On Fri, 22 Mar 2024 13:00:02 GMT, Karthik P K wrote:
> * In all the pages, under Region option, if we select MAX_VALUE for Min
> Height or Min Width, the application hangs or whole window becomes white. I
> observed this issue if we select MIN_VALUE or POSITIVE_INFINITY as well.
works as designed, doesn't it?
it's unlikely that an app developer would set this value, but a double property
does accept this value, so MT allows us to test the behavior.
> * In Bubble Chart, when multiple series are present, clear option under
> XYChart clears all but one series.
this button clears the points in the first series. I've added tooltips that
explain the function.
> * In colour picker, if editable option is selected, should we be able to
> change the colour value manually? If yes, I'm not able to edit the colour
> value.
the `ditable` property affects text fields in the "Custom Color..." dialog, and
it works correctly. It probably makes little sense to have the editable
property for this control.
Thanks for pointing this out - even though I did notice that when implementing
the page, but did not investigate.
> tests/manual/monkey/src/com/oracle/tools/fx/monkey/options/FontOption.java
> line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights
>> reserved.
>
> Minor: 2023 can be removed
The code also lives in a separate repository where it's copyrighted correctly
(since I constantly add things to it), so no change.
https://github.com/andy-goryachev-oracle/MonkeyTest
> tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/DemoPage.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
>
> Minor: Add 2024 here?
thanks for noticing (here and elsewhere)!
> tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/TreeTableViewPage.java
> line 400:
>
>> 398: s.addChoice("AUTO_RESIZE_NEXT_COLUMN",
>> TreeTableView.CONSTRAINED_RESIZE_POLICY_NEXT_COLUMN);
>> 399: s.addChoice("AUTO_RESIZE_SUBSEQUENT_COLUMNS",
>> TreeTableView.CONSTRAINED_RESIZE_POLICY_SUBSEQUENT_COLUMNS);
>> 400: //s.addChoice("CONSTRAINED_RESIZE_POLICY",
>> TreeTableView.CONSTRAINED_RESIZE_POLICY);
>
> Is there any particular reason why this line is commented?
old code, CONSTRAINED_RESIZE_POLICY is an alias for
CONSTRAINED_RESIZE_POLICY_FLEX_LAST_COLUMN. will clean up.
-
PR Comment: https://git.openjdk.org/jfx/pull/1406#issuecomment-2015670932
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1536018201
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1536018745
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1536020410
RFR: 8328820: Remove unused imports in javafx.swing
Removed unused imports in javafx.swing (1 change) module. See the discussion in [JDK-8289379](https://bugs.openjdk.org/browse/JDK-8289379) - Commit messages: - 8328820: Remove unused imports in javafx.swing Changes: https://git.openjdk.org/jfx/pull/1428/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1428=00 Issue: https://bugs.openjdk.org/browse/JDK-8328820 Stats: 17 lines in 1 file changed: 7 ins; 9 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1428.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1428/head:pull/1428 PR: https://git.openjdk.org/jfx/pull/1428
RFR: 8267565: Support "@1x" and "@3x" high-density image naming convention
This PR extends the range of hi-res images that are loaded via naming convention, now including scale factors higher than "@2x". Supporting these is already being [recommended](https://developer.apple.com/design/human-interface-guidelines/images#Best-practices) for some platforms. I tested this manually on Windows with "300%" UI scale factor, and verified "@x2" still works on macOS. - Commit messages: - Add extended high-dpi image support Changes: https://git.openjdk.org/jfx/pull/1429/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1429=00 Issue: https://bugs.openjdk.org/browse/JDK-8267565 Stats: 49 lines in 5 files changed: 19 ins; 0 del; 30 mod Patch: https://git.openjdk.org/jfx/pull/1429.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1429/head:pull/1429 PR: https://git.openjdk.org/jfx/pull/1429
RFR: 8328811: Fix missing @Overrides in demos
Fixing missing @Overrides in demo apps. This is still a trivial change since all the spots are identified by the IDE. - Commit messages: - part 3 - part 2 - part 1 Changes: https://git.openjdk.org/jfx/pull/1426/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1426=00 Issue: https://bugs.openjdk.org/browse/JDK-8328811 Stats: 530 lines in 82 files changed: 442 ins; 0 del; 88 mod Patch: https://git.openjdk.org/jfx/pull/1426.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1426/head:pull/1426 PR: https://git.openjdk.org/jfx/pull/1426
RFR: 8328754: Fix missing @Overrides in test
Fixing missing @ OVERRIDES in tests. This is still a trivial change since all the spots are identified by the IDE. - Commit messages: - 8328754: Fix missing @Overrides in test Changes: https://git.openjdk.org/jfx/pull/1427/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1427=00 Issue: https://bugs.openjdk.org/browse/JDK-8328754 Stats: 87 lines in 23 files changed: 64 ins; 0 del; 23 mod Patch: https://git.openjdk.org/jfx/pull/1427.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1427/head:pull/1427 PR: https://git.openjdk.org/jfx/pull/1427
RFR: 8328818: Fix Decora JSL to remove missing @Override warnings
Updating the JSL files to remove the "missing @ override" warning. - Commit messages: - 8328818: Fix Decora JSL to remove missing @Override warnings Changes: https://git.openjdk.org/jfx/pull/1425/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1425=00 Issue: https://bugs.openjdk.org/browse/JDK-8328818 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jfx/pull/1425.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1425/head:pull/1425 PR: https://git.openjdk.org/jfx/pull/1425
Re: RFR: 8328749: Remove unused imports in javafx.web
On Thu, 21 Mar 2024 21:57:46 GMT, Andy Goryachev wrote: > Using Eclipse IDE to remove unused imports **javafx.web** module, and update > the copyright year to 2024. Using wildcard for more than 10 static imports. > > > -- > > This is a trivial change, 1 reviewer is probably enough. removed swing file from this PR. - PR Comment: https://git.openjdk.org/jfx/pull/1421#issuecomment-2015423993
Re: RFR: 8328749: Remove unused imports in javafx.web [v2]
> Using Eclipse IDE to remove unused imports **javafx.web** module, and update > the copyright year to 2024. Using wildcard for more than 10 static imports. > > > -- > > This is a trivial change, 1 reviewer is probably enough. Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision: removed swing file - Changes: - all: https://git.openjdk.org/jfx/pull/1421/files - new: https://git.openjdk.org/jfx/pull/1421/files/b75bbf83..8fe2b264 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1421=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1421=00-01 Stats: 17 lines in 1 file changed: 9 ins; 7 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1421.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1421/head:pull/1421 PR: https://git.openjdk.org/jfx/pull/1421
RFR: 8328749: Remove unused imports in javafx.swing, javafx.web
Using Eclipse IDE to remove unused imports in **javafx.swing** and **javafx.web** modules, and update the copyright year to 2024. Using wildcard for more than 10 static imports. -- This is a trivial change, 1 reviewer is probably enough. - Commit messages: - 8328749: Remove unused imports in javafx.swing, javafx.web Changes: https://git.openjdk.org/jfx/pull/1421/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1421=00 Issue: https://bugs.openjdk.org/browse/JDK-8328749 Stats: 38 lines in 4 files changed: 11 ins; 22 del; 5 mod Patch: https://git.openjdk.org/jfx/pull/1421.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1421/head:pull/1421 PR: https://git.openjdk.org/jfx/pull/1421
Re: RFR: 8328749: Remove unused imports in javafx.swing, javafx.web
On Fri, 22 Mar 2024 15:52:02 GMT, Andy Goryachev wrote: > I suppose I'd need to extract swing into a separate PR, right @kevinrushforth > ? Yes. I was just getting ready to write this: It looks like I spoke too soon when I said it was OK to combine the change to `javafx.swing` into this PR. You will need to separate it out. - PR Comment: https://git.openjdk.org/jfx/pull/1421#issuecomment-2015392567
Re: RFR: 8328749: Remove unused imports in javafx.swing, javafx.web
On Thu, 21 Mar 2024 21:57:46 GMT, Andy Goryachev wrote: > Using Eclipse IDE to remove unused imports in **javafx.swing** and > **javafx.web** modules, and update the copyright year to 2024. Using wildcard > for more than 10 static imports. > > > -- > > This is a trivial change, 1 reviewer is probably enough. I suppose I'd need to extract swing into a separate PR, right @kevinrushforth ? - PR Comment: https://git.openjdk.org/jfx/pull/1421#issuecomment-2015387718
Re: RFR: 8328749: Remove unused imports in javafx.swing, javafx.web
On Thu, 21 Mar 2024 21:57:46 GMT, Andy Goryachev wrote: > Using Eclipse IDE to remove unused imports in **javafx.swing** and > **javafx.web** modules, and update the copyright year to 2024. Using wildcard > for more than 10 static imports. > > > -- > > This is a trivial change, 1 reviewer is probably enough. The change to WebWorkerTest can remain, though. As noted in PR #1424 : There are special considerations when touching files under `modules/javafx.web/src/main/native/Source/WebCore/bindings/java/dom3/java`, so it will need an extra pair of eyes. Also, this will need to be backported to earlier release trains to keep `modules/javafx.web/src/main/native/` identical for all releases. Reviewers: @kevinrushforth @HimaBinduMeda - PR Comment: https://git.openjdk.org/jfx/pull/1421#issuecomment-2015393595 PR Comment: https://git.openjdk.org/jfx/pull/1421#issuecomment-2015396312
Re: RFR: 8328752: Fix missing @Overrides in javafx.web
On Thu, 21 Mar 2024 23:48:40 GMT, Andy Goryachev wrote: > Fix missing @Overrides in **javafx.web**. > > This is still a trivial change since all the spots are identified by the IDE. There are special considerations when touching files under `modules/javafx.web/src/main/native/Source/WebCore/bindings/java/dom3/java`, so it will need an extra pair of eyes. Also, this will need to be backported to earlier release trains to keep `modules/javafx.web/src/main/native/` identical for all releases. Reviewers: @kevinrushforth @HimaBinduMeda - PR Comment: https://git.openjdk.org/jfx/pull/1424#issuecomment-2015368894
RFR: 8328752: Fix missing @Overrides in javafx.web
Fix missing @Overrides in **javafx.web**. This is still a trivial change since all the spots are identified by the IDE. - Commit messages: - part 2 - 8328752: Fix missing @Overrides in javafx.web Changes: https://git.openjdk.org/jfx/pull/1424/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1424=00 Issue: https://bugs.openjdk.org/browse/JDK-8328752 Stats: 1060 lines in 103 files changed: 956 ins; 0 del; 104 mod Patch: https://git.openjdk.org/jfx/pull/1424.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1424/head:pull/1424 PR: https://git.openjdk.org/jfx/pull/1424
Re: RFR: 8316372: Monkey Tester Application Part 3 [v2]
On Wed, 20 Mar 2024 23:56:50 GMT, Andy Goryachev wrote:
>> Further changes to the MonkeyTester application:
>>
>> - remember split pane divider ✔
>> - use 'private' instead of 'protected' in many cases ✔
>> - added more scripts to the 'writing systems' text sample ✔
>> - added RTL window control menu ✔
>> - added embedded swing/fx in tools ✔
>> - added copy popup menu in clipboard viewer ✔
>> - added the custom css field to the css playground tool ✔
>> - added many new pages ✔
>> - split XYChartPage into separate pages ✔
>> - switched to use property sheets (some choices might be incomplete) ✔
>>
>> https://github.com/andy-goryachev-oracle/jfx/blob/8316372.monkey/tests/manual/monkey/README.md
>
> Andy Goryachev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comments
Thanks @andy-goryachev-oracle for adding all these options to MonkeyTester.
This will definitely help in testing many aspects of controls easily.
I sanity tested few pages. My observations are listed below.
- In all the pages, under Region option, if we select MAX_VALUE for Min Height
or Min Width, the application hangs or whole window becomes white. I observed
this issue if we select MIN_VALUE or POSITIVE_INFINITY as well.
- In Bubble Chart, when multiple series are present, clear option under XYChart
clears all but one series.
- In colour picker, if editable option is selected, should we be able to change
the colour value manually? If yes, I'm not able to edit the colour value.
Added few inline comments as well.
I will complete the review soon and update here if I have anymore comments.
tests/manual/monkey/src/com/oracle/tools/fx/monkey/options/FontOption.java line
2:
> 1: /*
> 2: * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights
> reserved.
Minor: 2023 can be removed
tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/DemoPage.java line 2:
> 1: /*
> 2: * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
Minor: Add 2024 here?
tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/HTMLEditor_Page.java
line 2:
> 1: /*
> 2: * Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights
> reserved.
Minor: 2022 is not required here right?
tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/TableViewPage.java
line 385:
> 383: s.addChoiceSupplier("20 Equal", () -> {
> 384: var cs = columnBuilder();
> 385: for(int i=1; i<20; i++) {
Minor: add space after `=` and `<`.
Same comment for ln.no.392
tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/TreeTableViewPage.java
line 142:
> 140: TreeTableColumn c = newColumn();
> 141: c.setText("C" + System.currentTimeMillis());
> 142: //c.setCellValueFactory((f) -> new
> SimpleStringProperty(describe(c)));
Minor: Commented line can be removed?
tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/TreeTableViewPage.java
line 400:
> 398: s.addChoice("AUTO_RESIZE_NEXT_COLUMN",
> TreeTableView.CONSTRAINED_RESIZE_POLICY_NEXT_COLUMN);
> 399: s.addChoice("AUTO_RESIZE_SUBSEQUENT_COLUMNS",
> TreeTableView.CONSTRAINED_RESIZE_POLICY_SUBSEQUENT_COLUMNS);
> 400: //s.addChoice("CONSTRAINED_RESIZE_POLICY",
> TreeTableView.CONSTRAINED_RESIZE_POLICY);
Is there any particular reason why this line is commented?
tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/XYChartPageBase.java
line 2:
> 1: /*
> 2: * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights
> reserved.
Minor: Remove 2023?
tests/manual/monkey/src/com/oracle/tools/fx/monkey/settings/ISettingsProvider.java
line 2:
> 1: /*
> 2: * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
Minor: Any particular reason why only 2022 is retained here?
tests/manual/monkey/src/com/oracle/tools/fx/monkey/tools/EmbeddedJTextAreaWindow.java
line 2:
> 1: /*
> 2: * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
Minor: Change 2023 -> 2024
tests/manual/monkey/src/com/oracle/tools/fx/monkey/util/ShowCharacterRuns.java
line 73:
> 71: HitInfo hit = owner.hitTest(new Point2D(x, y));
> 72: Path p = new Path(owner.rangeShape(hit.getCharIndex(),
> hit.getCharIndex() + 1));
> 73: //System.err.println(i + " " + cs); // FIX
Do we still need this commented line?
tests/manual/monkey/src/com/oracle/tools/fx/monkey/util/Utils.java line 44:
> 42:
> 43: public static void fromPairs(Object[] pairs,
> BiConsumer client) {
> 44: for(int i=0; ihttps://git.openjdk.org/jfx/pull/1406#pullrequestreview-1951849501
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1533681182
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1535164015
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1535166734
PR Review Comment:
