On Fri, 7 Feb 2025 22:20:26 GMT, Harshitha Onkar <hon...@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
