RFR: JDK-8348302 : [Test bug] Update FileDialogIconTest.java [v3]

[Alexey Ivanov]

On Fri, 7 Feb 2025 22:20:26 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

>> FileDialogIconTest.java has been updated.
>> 
>> Following changes were made.
>> 
>> -  Test instructions updated
>> -  BugID associated with the test is updated to the correct one
>> -  setIconBufferedImagesToFrame and setIconBufferedImagesToDialog btns added to the frame.
>> -  other minor cleanups
>
> Harshitha Onkar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   minor

test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 59:

> 57:     public static void main(String[] args) throws Exception {
> 58:         String INSTRUCTIONS = """
> 59:                 The 1st row of buttons in testUI are to open Load,

It may not be apparent what `testUI` is… on the other hand, it should still be obvious that you referring to a window on the right.

It could better to refer to it as a ‘window’… or a least ‘test UI’ (with a space).

test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 76:

> 74: 
> 75:         PassFailJFrame.builder()
> 76:                 .title("Test Instructions Frame")

I believe the ‘frame‘ is redundant.

test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 152:

> 150:                         image = Toolkit.getDefaultToolkit().getImage(fileName);
> 151:                         PassFailJFrame.log("Loaded image " + "T" + i + ".gif."
> 152:                                            + " Setting to the list for frame");

Why can't `fileName` be used in the logging?

Suggestion:

                        PassFailJFrame.log("Loaded image " + fileName + "."
                                           + " Setting to the list for frame");

test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 172:

> 170:                         image = Toolkit.getDefaultToolkit().getImage(fileName);
> 171:                         PassFailJFrame.log("Loaded image " + "T" + i + ".gif."
> 172:                                            + " Setting to the list for dialog");

This piece of code looks the save as the one above.

Does it make to isolate the loading of images from setting them (to avoid code duplication)?

test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 185:

> 183:         setImageButton5.addActionListener(new ActionListener() {
> 184:             public void actionPerformed(ActionEvent event) {
> 185:                 List<Image> list = new ArrayList<>();

Suggestion:

                List<Image> list = new ArrayList<>(4);

Micro-optimization: we know the number of images, the array in `ArrayList` could be sized accordingly.

test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 217:

> 215:                         list.add(image);
> 216:                     }
> 217:                     setImagesToFD(list);

Here, again the code is the same as in `setImageButton5.addActionListener` except for the final call `setImagesToFD` vs `setImagesToFrame`. I suggest moving capture code out of either methods and re-use it.

test/jdk/java/awt/Dialog/FileDialogIconTest/FileDialogIconTest.java line 256:

> 254:     static class MyActionListener implements ActionListener {
> 255:         int id;
> 256:         String name;

Suggestion:

        final int id;
        final String name;

If the value of the fields is unchanged, they should be `final`. The may even be `private`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949668666
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949652576
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949653808
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949657086
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949658652
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949661300
PR Review Comment: https://git.openjdk.org/jdk/pull/23523#discussion_r1949663909