On Fri, 18 Feb 2022 09:15:43 GMT, Manukumar V S <m...@openjdk.org> wrote:
>> Create a regression test for >> [JDK-4618767](https://bugs.openjdk.java.net/browse/JDK-4618767) >> >> Issue identified in >> [JDK-4618767](https://bugs.openjdk.java.net/browse/JDK-4618767): >> Typing a letter while a JList has focus now makes the selection jump to the >> first/next node/item whose text starts with that letter even though that >> letter is accompanied by modifier keys such as ALT or CTRL. >> >> Fix: >> Only enable JList letter navigation when the user >> doesn't press any modifier keys such as ALT or CTRL. >> >> Testing: >> I have verified this test with JDK 1.4.0 and JDK 1.4.1 . >> The issue is reproducible using the test with JDK 1.4.0 , where the bug was >> originally reported and the test passed in JDK 1.4.1 where the issue was >> fixed. >> I have tested it in Mac and Windows platforms multiple times and it passed >> everywhere. > > Manukumar V S has updated the pull request incrementally with one additional > commit since the last revision: > > Wait until focus is on list, changed the mnemonics for opening menu, added > menulistener, caught UnsupportedLookAndFeelException and ignore LnF Changes requested by aivanov (Reviewer). test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 53: > 51: public class JListSelectedElementTest { > 52: > 53: private static final int MENU = KeyEvent.VK_F; Suggestion: private static final int FILE_MENU = KeyEvent.VK_F; or `FILE_MENU_MNEMONIC`, or use `KeyEvent.VK_F` directly as you do for other keys. Otherwise, it looks as if you pressing a Menu key, `KeyEvent.VK_CONTEXT_MENU`. test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 68: > 66: robot.setAutoDelay(200); > 67: > 68: final boolean isAMac = > System.getProperty("os.name").toLowerCase().contains("os x"); Suggestion: final boolean isMac = System.getProperty("os.name") .toLowerCase() .contains("os x"); test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 96: > 94: throw new RuntimeException("Waited for long, but > can't get focus for list"); > 95: } > 96: } Maybe you should wait for `FocusEvent.FOCUS_GAINED` event? In some way, requesting focus to list is redundant: it's the only component in the frame, there's no other component to get the focus. test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 103: > 101: > 102: // Assertion check to verify that selected node is 'bill' > 103: String elementSel = list.getSelectedValue(); `list.getSelectedValue()` must be called on EDT. test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 120: > 118: robot.keyRelease(KeyEvent.VK_ALT); > 119: robot.keyRelease(KeyEvent.VK_CONTROL); > 120: robot.keyRelease(MENU); I suggest adding a helper method or using an existing one from reghelpers/Utils class: public static void hitKeys(Robot robot, int... keys) { for (int i = 0; i < keys.length; i++) { robot.keyPress(keys[i]); } for (int i = keys.length - 1; i >= 0; i--) { robot.keyRelease(keys[i]); } } If you copy the implementation, you can use your robot field instead of passing it as the parameter. Also, it's a good practice to release in the reverse order to pressing them, which this method will help to achieve. test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 121: > 119: robot.keyRelease(KeyEvent.VK_CONTROL); > 120: robot.keyRelease(MENU); > 121: }else{ Suggestion: } else { Spaces missing. test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 129: > 127: > 128: waitCount = 0; > 129: while (!menuSelected) { A `CountDownLatch` instead of polling loop? test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 165: > 163: private static void createUI() { > 164: frame = new JFrame(); > 165: list = new JList(new String[]{"anaheim", "bill", "chicago", > "dingo", "ernie", "freak"}); Suggestion: list = new JList<>(new String[]{"anaheim", "bill", "chicago", "dingo", "ernie", "freak"}); test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 174: > 172: public void menuSelected(MenuEvent e) { > 173: menuSelected = true; > 174: } For anonymous classes, you use the regular indent of 4 spaces: menu.addMenuListener(new MenuListener() { @Override public void menuSelected(MenuEvent e) { menuSelected = true; } // Other methods }); test/jdk/javax/swing/JList/4618767/JListSelectedElementTest.java line 189: > 187: frame.setJMenuBar(menuBar); > 188: frame.setContentPane(list); > 189: frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE); Suggestion: frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE); You should never call `System.exit` from jtreg tests. ------------- PR: https://git.openjdk.java.net/jdk/pull/7496
