RFR: 8281966: Absolute path of symlink is null in JFileChooser [v4]
Alexey Ivanov
aivanov at openjdk.org
Thu Aug 4 15:39:19 UTC 2022
On Thu, 4 Aug 2022 12:08:15 GMT, Tejesh R <tr at openjdk.org> wrote:
>> Absolute path of Symbolic Link created in Windows is set to `null` in `BasicFileChooserUI` class. This happens when propertyChangeListener is implemented to get the Symbolic link's Absolute path on Mouse click through JFileChooser. The reason being that on click of Symbolic link, the _ValueChanged()_ in `BasicFileChooserUI` class has a logic which actually sets the `chooser.SelectedFile()` to `null` even though the path is not null. Hence the issue is addressed by checking if its a Symbolic link and then setting the `chooser.SelectedFile()` to the value of clicked link without modifying the other logics.
>
> Tejesh R has updated the pull request incrementally with one additional commit since the last revision:
>
> Fix for multi selection and updated the test to handle both single and multi selection
It works fine now. Thank you!
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicFileChooserUI.java line 713:
> 711: && chooser.isTraversable(((File)objects[0]))
> 712: && (useSetDirectory || (!fsv.isFileSystem(((File)objects[0]))
> 713: && !Files.isSymbolicLink(Paths.get(((File)objects[0]).getPath()))))) {
~~This does not align with other parts of the code where `isSymbolicLink` is combined together with `isFileSystem`.~~
You should format the code to avoid confusion: `isSymbolicLink` is not part of the top-level condition but rather of the part inside the parentheses after `||`
This should look this way:
&& (useSetDirectory
|| (!fsv.isFileSystem(((File)objects[0]))
&& !Files.isSymbolicLink(((File)objects[0]).toPath())))) {
src/java.desktop/share/classes/javax/swing/plaf/basic/BasicFileChooserUI.java line 721:
> 719: File f = (File) object;
> 720: boolean isDir = f.isDirectory();
> 721: Path path = Paths.get(f.getPath());
Suggestion:
Path path = f.toPath();
You can directly convert a `File` to `Path`. Please update this in all the places.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 41:
> 39:
> 40: /*
> 41: *@test
Suggestion:
* @test
A space has disappeared.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 58:
> 56: static JTextArea textArea = null;
> 57: static JCheckBox checkMSelection = null;
> 58: static PassFailJFrame passFailJFrame = null;
There's no need to assign `null` or `false` explicitly, these are the default values for reference types.
Why do you use `Boolean` type? Can't it be primitive `boolean` type instead?
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 79:
> 77: 1. Open an elevated Command Prompt.
> 78: 2. Paste the following commands:
> 79: cd /d C:\
Suggestion:
cd /d C:\\
Another backslash is missing to display the backslash.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 83:
> 81: cd FileChooserTest
> 82: mkdir target
> 83: mklink /d link FileChooserTest
The command should create a symbolic to the target:
mklink /d link target
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 95:
> 93: with valid Absolute path then Click PASS, else either
> 94: of them failed click FAIL.
> 95: 6. When done with testing, paste the following commands to remove the 'filechooser' directory
I propose the following text:
5. Single-selection:
Click "link" directory, the absolute path of the symbolic
link should be displayed. If it's null, click FAIL.
Click "target" directory, its absolute path should be
displayed.
Enable multiple selection by clicking the checkbox.
Multi-selection:
Click "link", press Ctrl and then click "target".
Both should be selected and their absolute paths should be
displayed.
If "link" can't be selected or if its absolute path is null,
click FAIL.
If "link" can be selected in both single- and multi-selection modes,
click PASS.
6. When done with testing, paste the following commands to remove
the 'FileChooserTest' directory:
I hope the instructions for testing in 5 are clearer now.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 97:
> 95: 6. When done with testing, paste the following commands to remove the 'filechooser' directory
> 96: cd \
> 97: rmdir /s /q filechooser
Suggestion:
cd \\
rmdir /s /q C:\\FileChooserTest
Another backslash is missing in the first line to correctly display `` and you didn't update the directory name for `rmdir`.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 103:
> 101: frame = new JFrame("JFileChooser Symbolic Link test");
> 102: panel = new JPanel(new BorderLayout());
> 103: checkMSelection = new JCheckBox("Enable Multi-Selection");
`checkMSelection` → `multiSelection`?
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 104:
> 102: panel = new JPanel(new BorderLayout());
> 103: checkMSelection = new JCheckBox("Enable Multi-Selection");
> 104: textArea = new JTextArea();
`textArea` → `pathList`?
I also suggest passing rows, columns to define the preferred size: `new JTextArea(10, 50);`.
Then remove the line: `textArea.setPreferredSize(new Dimension(600,300));`. The UI fits better on the screen this way.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 105:
> 103: checkMSelection = new JCheckBox("Enable Multi-Selection");
> 104: textArea = new JTextArea();
> 105: jfc = new JFileChooser();
Suggestion:
jfc = new JFileChooser(new File("C:\"));
To start file chooser at the root of drive `C:` where the sample commands create the `FileChooserTest` folder.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 106:
> 104: textArea = new JTextArea();
> 105: jfc = new JFileChooser();
> 106: passFailJFrame = new PassFailJFrame(INSTRUCTIONS);
Suggestion:
passFailJFrame = new PassFailJFrame("Test Instructions",
INSTRUCTIONS,
5L,
35, 40);
Since the instructions are long for this test, I suggest passing additional parameters and modifying the default number of rows displayed so that the complete instructions are displayed.
If you change the instruction text, you may need to amend the number of rows (35).
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 112:
> 110: frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
> 111: panel.add(checkMSelection,BorderLayout.EAST);
> 112: panel.add(textArea,BorderLayout.WEST);
Spaces after comma.
Wrap the `textArea` into `JScrollPane`:
panel.add(new JScrollPane(textArea), BorderLayout.WEST);
Thus text could be scrolled.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 116:
> 114: jfc.setControlButtonsAreShown(false);
> 115: jfc.setFileSelectionMode(JFileChooser.DIRECTORIES_ONLY);
> 116: jfc.setMultiSelectionEnabled(bMultiSel_Enabled);
Suggestion:
jfc.setMultiSelectionEnabled(checkMSelection.isSelected());
To drop `bMultiSel_Enabled` field.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 118:
> 116: jfc.setMultiSelectionEnabled(bMultiSel_Enabled);
> 117: textArea.setPreferredSize(new Dimension(600,300));
> 118: textArea.append("Path List");
Suggestion:
textArea.append("Path List\n");
Add a line break right away. To simplify appending the lines in property changed listener.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 128:
> 126: } else {
> 127: bMultiSel_Enabled = false;
> 128: }
Suggestion:
bMultiSel_Enabled = ((JCheckBox)source).isSelected();
In fact, you don't need `bMultiSel_Enabled` field. Just use `isSelected()` directly:
jfc.setMultiSelectionEnabled(((JCheckBox)source).isSelected());
If you like, you can access the `JCheckBox` instance directly, you saved it in a field:
jfc.setMultiSelectionEnabled(checkMSelection.isSelected());
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 136:
> 134: if (JFileChooser.SELECTED_FILE_CHANGED_PROPERTY.equals(evt.getPropertyName())) {
> 135: System.out.println("Absolute Path : "+evt.getNewValue());
> 136: textArea.append("\nAbsolute Path : "+evt.getNewValue());
Spaces around `+`.
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 137:
> 135: System.out.println("Absolute Path : "+evt.getNewValue());
> 136: textArea.append("\nAbsolute Path : "+evt.getNewValue());
> 137: }
I suggest handling `SELECTED_FILES_CHANGED_PROPERTY` to ensure multi-selection works correctly:
@Override
public void propertyChange(PropertyChangeEvent evt) {
String msg = null;
if (JFileChooser.SELECTED_FILE_CHANGED_PROPERTY.equals(evt.getPropertyName())) {
msg = "Absolute Path : " + evt.getNewValue();
} else if (JFileChooser.SELECTED_FILES_CHANGED_PROPERTY.equals(evt.getPropertyName())) {
msg = "Selected files: " + Arrays.toString((File[]) evt.getNewValue());
}
if (msg != null) {
System.out.println(msg);
textArea.append(msg + "\n");
}
}
test/jdk/javax/swing/JFileChooser/FileChooserSymLinkTest.java line 141:
> 139: });
> 140: frame.add(panel,BorderLayout.NORTH);
> 141: frame.add(jfc,BorderLayout.CENTER);
Spaces after comma.
-------------
Changes requested by aivanov (Reviewer).
PR: https://git.openjdk.org/jdk/pull/9597
More information about the client-libs-dev
mailing list