On Tue, 13 May 2025 10:34:38 GMT, Tejesh R <t...@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
