RFR: 8139228: JFileChooser renders file names as HTML document [v3]

Wed May 21 16:00:13 UTC 2025

On Tue, 20 May 2025 18:40:29 GMT, Alexey Ivanov <aivanov at 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