On Wed, 20 Mar 2024 23:56:50 GMT, Andy Goryachev <ango...@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
