RFR: 8139228: JFileChooser renders file names as HTML document [v2]
Alexey Ivanov
aivanov at openjdk.org
Fri May 16 20:09:57 UTC 2025
On Tue, 13 May 2025 10:34:38 GMT, Tejesh R <tr at openjdk.org> wrote:
>> The rendering of the directory names are handled as JLabel w.r.t Look and feel and also either Details/List view. Though FilePane creates basic rendering for these two few Look and Feel define their own renderers and also ComboBox Directory directory name view. Since HTML filtering is not taken care in any of these renderers, JLabel renders them as HTML document if nothing is set or specified.
>> The fix is to get "html.disable" property from JFileChooser and set the same to JLabel component which renders and set Directory name. Hence applications can either enable/disable this property and control HTML rendering of directory name.
>
> Tejesh R has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
>
> - Merge branch 'master' of https://git.openjdk.java.net/jdk into branch_8139228
> - Fix for windows L&F
> - Fix + Manual test
Changes requested by aivanov (Reviewer).
src/java.desktop/macosx/classes/com/apple/laf/AquaFileChooserUI.java line 1195:
> 1193: setIcon(fc.getIcon(file));
> 1194: setEnabled(isSelectableInList(file));
> 1195: this.putClientProperty("html.disable", getFileChooser().getClientProperty("html.disable"));
Suggestion:
putClientProperty("html.disable", getFileChooser().getClientProperty("html.disable"));
No method call above uses explicit `this`, I see no reason to use it here either.
This comment applies to all the modified files.
src/java.desktop/share/classes/sun/swing/FilePane.java line 1238:
> 1236: setText(text);
> 1237: this.putClientProperty("html.disable", getFileChooser().getClientProperty("html.disable"));
> 1238: return this;
Be consistent at least inside one file. Here you removed the blank line, but below at 1632–1635 you preserved the blank line.
src/java.desktop/share/classes/sun/swing/FilePane.java line 1635:
> 1633: this.putClientProperty("html.disable", getFileChooser().getClientProperty("html.disable"));
> 1634:
> 1635: return this;
I prefer this style everywhere: all the code that sets up the component, a blank line followed by the `return` statement.
I'd add a blank line before `putClientProperty` as well — setting the `"html.disable"` property isn't the part of previous statements which configure the renderer component one way or another, and, with blank lines around this call, it will stand out in the code.
test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 68:
> 66: .instructions(INSTRUCTIONS)
> 67: .columns(45)
> 68: .testUI(initialize())
In majority of cases, you should pass a method reference rather than the result of a method call to `testUI`.
As it's written, the `initialize` is executed on the main thread, and its result—the list frames—is passed to `PassFailJFrame`.
What you actually want is `HTMLFileName::initialize`. In this case, the `PassFailJFrame` framework will call the `initialize` method on EDT when the time to create UI comes.
test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 69:
> 67: .columns(45)
> 68: .testUI(initialize())
> 69: .position(PassFailJFrame.Position.TOP_LEFT_CORNER)
Suggestion:
.positionTestUIRightColumn()
I suggest using [**`positionTestUIRightColumnCentered`**](https://cr.openjdk.org/~aivanov/PassFailJFrame/api/PassFailJFrame.Builder.html#positionTestUIRightColumnCentered()) to lay out the entire test UI in the centre of the screen.
Since two file choosers take quite a lot of space, the file chooser at the bottom doesn't fit the screen if you use [`positionTestUIRightColumn`](https://cr.openjdk.org/~aivanov/PassFailJFrame/api/PassFailJFrame.Builder.html#positionTestUIRightColumn()).
Alternatively, you can choose a horizontal layout with [`positionTestUIBottomRowCentered`](https://cr.openjdk.org/~aivanov/PassFailJFrame/api/PassFailJFrame.Builder.html#positionTestUIBottomRowCentered()).
test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 79:
> 77: }
> 78:
> 79: public static List<JFrame> initialize() {
Suggestion:
private static List<JFrame> initialize() {
test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 84:
> 82: String fileName = homeDir + File.separator +
> 83: "<html><h1 color=#ff00ff><font face=\"Comic Sans MS\">Testing Name";
> 84: directory = new File(fileName);
You shouldn't create the files in the home directory of the user any way. Create them in the current directory which is a scratch directory when the test is run with jtreg; the contents of the scratch directory is automatically removed by jtreg when the test exits, even if the test itself fails to remove its own temporary files.
Yet if you create the files in the user's home, all these files and directories will be still be around if anything goes awry in the test.
test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 85:
> 83: "<html><h1 color=#ff00ff><font face=\"Comic Sans MS\">Testing Name";
> 84: directory = new File(fileName);
> 85: directory.mkdir();
Do you need the directory at all?
Use `VirtualFileSystemView` for all platforms. The fact that it's possible to create files with HTML tags in the name is irrelevant to the test.
test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 96:
> 94: jfc = new JFileChooser(homeDir);
> 95: jfc_HTML_Enabled = new JFileChooser(homeDir);
> 96: }
I suggest creating a method that creates one file chooser placed in a frame; pass a boolean parameter to specify whether to set the `"html.disable"` property to `false` or keep its default value of `true`.
Then your `initialize` method would be as simple as a single return statement:
private static List<JFrame> initialize() {
return List.of(createFileChooser(true), createFileChooser(false));
}
test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 107:
> 105: }
> 106:
> 107: static class VirtualFileSystemView extends FileSystemView {
Suggestion:
private static class VirtualFileSystemView extends FileSystemView {
-------------
PR Review: https://git.openjdk.org/jdk/pull/24439#pullrequestreview-2847459647
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2093571535
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2093578359
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2093586784
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2093592420
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2093595668
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2093604829
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2093609644
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2093604201
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2093617717
PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2093604589
More information about the client-libs-dev
mailing list