On Fri, 16 May 2025 19:24:36 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> 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 > > 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. Updated. > 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. Updated. > 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. Updated. > 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. Updated. > 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()). Yeah, [positionTestUIBottomRowCentered](https://cr.openjdk.org/~aivanov/PassFailJFrame/api/PassFailJFrame.Builder.html#positionTestUIBottomRowCentered()) works better. Updated. > test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 79: > >> 77: } >> 78: >> 79: public static List<JFrame> initialize() { > > Suggestion: > > private static List<JFrame> initialize() { Updated > 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. Since I have used virtual file system, this would be skipped now. > 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. I've used VirtualFileSystemView to all platforms. Having both directory and file with HTML tags in the virtual FSV is helpful in verifying both File Pane and Directory ComboBox view too. > 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)); > } Updated. > test/jdk/javax/swing/JFileChooser/HTMLFileName.java line 107: > >> 105: } >> 106: >> 107: static class VirtualFileSystemView extends FileSystemView { > > Suggestion: > > private static class VirtualFileSystemView extends FileSystemView { Updated. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2095746398 PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2095746660 PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2095746841 PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2095747011 PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2095747986 PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2095752012 PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2095753291 PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2095748627 PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2095753544 PR Review Comment: https://git.openjdk.org/jdk/pull/24439#discussion_r2095751839
