On Mon, 27 Feb 2023 19:11:43 GMT, Alexey Ivanov <[email protected]> wrote:
>> Damon Nguyen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Propagate exceptions. Move methods to EDT.
>
> test/jdk/javax/swing/JComboBox/EditableComboBoxPopupPos.java line 62:
>
>> 60:
>> 61: for (UIManager.LookAndFeelInfo laf :
>> UIManager.getInstalledLookAndFeels()) {
>> 62: lafName = laf.getName().equals("CDE/Motif") ? "Motif" :
>> laf.getName();
>
> Is it really needed? Is `CDE/Motif` such a bad name that it needs adjusting?
This was added because the `/` caused issues while debugging with file paths,
but it's no longer needed. Updated
> test/jdk/javax/swing/JComboBox/EditableComboBoxPopupPos.java line 89:
>
>> 87: // first selection item is in the correct position on
>> screen.
>> 88: cb1.setSelectedIndex(1);
>> 89: cb2.setSelectedIndex(1);
>
> These methods should be called on EDT.
Updated
> test/jdk/javax/swing/JComboBox/EditableComboBoxPopupPos.java line 97:
>
>> 95: }
>> 96: } catch (Exception e) {
>> 97: e.printStackTrace();
>
> You should not catch the exception but propagate it for jtreg to fail the
> test if it throws the exception. Otherwise, the test is considered passed
> even though it may print `Test failed`.
Good point. I propagated the exception and re-tested
> test/jdk/javax/swing/JComboBox/EditableComboBoxPopupPos.java line 117:
>
>> 115: robot.mouseMove(cb.getLocationOnScreen().x + cb.getWidth()
>> 116: - BUTTON_OFFSET, cb.getLocationOnScreen().y
>> 117: + POPUP_OFFSET);
>
> `getLocationOnScreen()`, `getWidth()` should be called on EDT.
Updated. I moved these accessors out of the method and now call them on the EDT
and store the value in vars.
> test/jdk/javax/swing/JComboBox/EditableComboBoxPopupPos.java line 131:
>
>> 129: throws InterruptedException, InvocationTargetException {
>> 130: if (c1.getSelectedItem().toString().equals("One")
>> 131: && c2.getSelectedItem().toString().equals("One")) {
>
> `getSelectedItem` should be called on EDT.
Same as above
> test/jdk/javax/swing/JComboBox/EditableComboBoxPopupPos.java line 135:
>
>> 133: SwingUtilities.invokeAndWait(() -> frame.dispose());
>> 134: } else {
>> 135: SwingUtilities.invokeAndWait(() -> frame.dispose());
>
> Your `main` method disposes of the frame. You should just move try-finally
> block into the for-loop over available Look-and-Feels.
Added to the bottom of the loop
> test/jdk/javax/swing/JComboBox/EditableComboBoxPopupPos.java line 139:
>
>> 137: }
>> 138: }
>> 139: }
>
> Please add a new line character to the end of the file.
Updated
-------------
PR: https://git.openjdk.org/jdk/pull/12750
