RFR: 8316372: Monkey Tester Application Part 3 [v2]

Fri Mar 22 13:02:34 UTC 2024

On Wed, 20 Mar 2024 23:56:50 GMT, Andy Goryachev <angorya at openjdk.org> 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<DataRow, Object> 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<String,String> client) {
> 44:         for(int i=0; i<pairs.length; ) {

Minor: Add space after `=` and `<`

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

PR Review: https://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: https://git.openjdk.org/jfx/pull/1406#discussion_r1535248692
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1535268019
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1535285220
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1535302896
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1535306689
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1535349178
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1535380025
PR Review Comment: https://git.openjdk.org/jfx/pull/1406#discussion_r1535389064