On Tue, 20 May 2025 18:40:29 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Tejesh R has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review fix
>
> test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 29:
>
>> 27: import javax.swing.filechooser.FileSystemView;
>> 28: import java.io.File;
>> 29: import java.util.List;
>
> Suggestion:
>
> import java.io.File;
> import java.util.List;
> import javax.swing.Icon;
> import javax.swing.JFileChooser;
> import javax.swing.JFrame;
> import javax.swing.filechooser.FileSystemView;
>
> We haven't reached an agreement, yet `java.*` packages usually go before
> `javax.*` packages in the list of imports.
Updated.
> test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 43:
>
>> 41:
>> 42: public static void main(String[] args) throws Exception {
>> 43: String INSTRUCTIONS = """
>
> Please make `INSTRUCTIONS` a `private static final` field at the top of the
> class. This is the most common way in tests, and it'll make the `main` method
> cleaner especially after adding additional logic for switching Look-and-Feels
> as I [suggested in a discussion
> thread](https://github.com/openjdk/jdk/pull/24439#discussion_r2100110185).
Updated.
> test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 45:
>
>> 43: String INSTRUCTIONS = """
>> 44: 1. FileChooser shows up a virtual directory and file
>> with name
>> 45: "<html><h1 color=#ff00ff><font face="Comic Sans
>> MS">Testing Name".
>
> The file name is now `Swing Rocks!`
Updated.
> test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 60:
>
>> 58: If it appears to be in plain text, then test
>> FAILS.
>> 59: (Verify for all Look and Feel).
>> 60: """;
>
> Suggestion:
>
> """;
>
> Avoid trailing space, it's not needed.
Updated.
> test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 85:
>
>> 83: frame.pack();
>> 84: return frame;
>> 85: }
>
> Suggestion:
>
> private static JFrame createFileChooser(boolean htmlEnabled) {
> JFileChooser jfc = new JFileChooser(new VirtualFileSystemView());
> jfc.putClientProperty("html.disable", htmlEnabled);
> jfc.setControlButtonsAreShown(false);
>
> JFrame frame = new JFrame((htmlEnabled) ? "HTML enabled" : "HTML
> disabled");
> frame.add(jfc);
> frame.pack();
> return frame;
> }
>
> I think this way is cleaner: create the file chooser, configure it¹; then
> create the frame, put the file chooser into the frame.
>
> `html_enabled` → `htmlEnabled`, which aligns to the Java style.
>
> ¹ I added `jfc.setControlButtonsAreShown(false)` to hide the *Open* and
> *Cancel* buttons.
Updated.
> test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 96:
>
>> 94: public File[] getRoots() {
>> 95: return new File[]{
>> 96: new File("/", "<html><h1 color=#ff00ff><font
>> face=\"Comic Sans MS\">SWING ROCKS!!!111"),
>
> Suggestion:
>
> new File("/", "<html><h1 color=#ff00ff><font face="Comic
> Sans MS">Swing Rocks!"),
>
> Use title case?
Updated.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2100650539
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2100649554
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2100653203
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2100644786
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2100652050
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2100646865
