4834298: JFileChooser.getSelectedFiles() failed with multi-selection and double-click #9996

TejeshR13 · 2022-08-24T07:44:52Z

JFileChooser.getSelectedFiles() failed to retrieve files with multi-selection and double-click. This occurs when a single file is selected then enable multi-selection then select the same file through mouse double-click(Two file chooser dialogs used before and after multi-selection enabled). SelectedFiles are updated when a change in files/directory selection is done or when selection is made through keyboard selection. In this case since a single file is selected before multi-selection is enabled and same file is selected again without changing the selection index/directory, leaving selected Files un-updated.
Proposed Fix : Whenever Multi-Selection is enabled check if any files are selected, if yes update the selected Files of JFileChoooser.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-4834298: JFileChooser.getSelectedFiles() failed with multi-selection and double-click

Reviewers

@kumarabhi006 (no known github.com user name / role) ⚠️ Review applies to 1d7f271b
Jayathirth D V (@jayathirthrao - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9996/head:pull/9996
$ git checkout pull/9996

Update a local copy of the PR:
$ git checkout pull/9996
$ git pull https://git.openjdk.org/jdk pull/9996/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9996

View PR using the GUI difftool:
$ git pr show -t 9996

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9996.diff

bridgekeeper · 2022-08-24T07:45:59Z

👋 Welcome back tr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-08-24T07:48:22Z

⚠️ @TejeshR13 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk · 2022-08-24T07:50:15Z

@TejeshR13 The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-08-24T07:53:16Z

Webrevs

turbanoff · 2022-09-01T06:24:17Z

src/java.desktop/share/classes/sun/swing/FilePane.java

+            //Check if any files are selected before setting multi-Selection.
+            //If selected, retain them and update the selected files in FileChooser class.
+            File[] selectedFiles = null;
+            if(getFileChooser().getSelectedFiles().length != 0) {


let's add space after ifs

kumarabhi006 · 2022-09-06T08:53:43Z

src/java.desktop/share/classes/sun/swing/FilePane.java

+            if (getFileChooser().getSelectedFiles().length != 0) {
+                selectedFiles = getFileChooser().getSelectedFiles();
+            } else if (getFileChooser().getSelectedFile() != null) {
+                File selectedFile = getFileChooser().getSelectedFile();


File selectedFile = getFileChooser().getSelectedFile(); selectedFiles = new File[1]; selectedFiles[0] = selectedFile;

You can directly assign getFileChooser().getSelectedFile() value to selectedFiles[0]. I think no need to create extra selectedFile variable.

kumarabhi006 · 2022-09-06T09:26:48Z

test/jdk/javax/swing/JFileChooser/MultiSelectionEnabledSelectedFilesTest.java

+        chooser.showOpenDialog(null);
+        File[] files = chooser.getSelectedFiles();
+
+        if(files.length <= 0) {


if(files.length <= 0) {

Please add space after if.

kumarabhi006 · 2022-09-06T11:07:09Z

I guess copyright year for FilePane.java needs to be updated.

TejeshR13 · 2022-09-06T11:10:09Z

I guess copyright year for FilePane.java needs to be updated.

Updated.

kumarabhi006

Applied the patch and tested, attached test case is passed with your fix.

jayathirthrao · 2022-09-08T09:50:04Z

test/jdk/javax/swing/JFileChooser/MultiSelectionEnabledSelectedFilesTest.java

+ *          returns selectedFiles when Multi-Selection is enabled.
+ * @run main/manual MultiSelectionEnabledSelectedFilesTest
+ */
+public class MultiSelectionEnabledSelectedFilesTest {


Is this bug specific to any platform?
Because the test is created to run on all platforms and it doesnt fail in my Mac.

Yeah, it doesn't fail in mac. It does fail in windows and linux, so didn't make any os specific. Still I have tested in all platforms and it works fine. The bug was raised as linux specific.

Why the test doesnt fail in Mac(In my case second dialog doesnt exit and it stays on the screen, which will result in timeout and jtreg error on Mac)?

It looks like fix is not some platform specific change.

The fix handles Windows and linux platforms, since FilePane.class is involved on mode selection. In Mac its doesn't come thought this path, I guess it handles some other way. The issue doesn't occur on key press also (All platforms), since the path of setting selected files are different for each scenario.

I guess the dialogue is not not been removed when exception is thrown. Will take a look into it.

If the product fix that is being done will touch only Linux and Windows(And in Mac it looks like it not exiting properly), we can make this regression test to run only on Linux and Windows using jtreg tag:
@requires (os.family == "windows" | os.family == "linux")

jayathirthrao · 2022-09-08T09:51:57Z

test/jdk/javax/swing/JFileChooser/MultiSelectionEnabledSelectedFilesTest.java

+
+    static void runTest() {
+        //Initialize the components
+        String INSTRUCTIONS


Instruction should be updated to specify that we need to use double-click.
Also when i select a file using double click in first and then select same file using double-click(without moving mouse) test doesnt exit. JFileChooser second dialog just stays on the screen.

jayathirthrao · 2022-09-08T09:54:05Z

test/jdk/javax/swing/JFileChooser/MultiSelectionEnabledSelectedFilesTest.java

+        chooser.showOpenDialog(null);
+        File[] files = chooser.getSelectedFiles();
+
+        if (files.length <= 0) {


Can File[] length be negative? I think just a "==" to "0" check should suffice.

jayathirthrao · 2022-09-08T10:18:04Z

src/java.desktop/share/classes/sun/swing/FilePane.java

+                selectedFiles = new File[1];
+                selectedFiles[0] = getFileChooser().getSelectedFile();
+            }
+


Is this problem seen only when single file is selected? What happens when more than one file is selected?
By default, is MultiSelection enabled on JfileChooser?

In FilePane.java, i see that get/setSelectedFiles() is exclusively used when multi-selection mode is enabled and get/setSelectedFile() is used when multi-selection is not enabled. Using getSelectedFile() to change the state when multi-selection is enabled might introduce state corruption issues in other scenarios.

This problem occurs when user switch the mode from single selection to multi selection, that too on specified scenario. By default multiselection will be disabled. When user selects a file in single selection and then switch to multi selection the selected single file is not retained in selectedFiles (Even though its a single file). So In order to handle this situation, whenever user switch from any state to multiselection enabled state, the previously selected files(if present) is set to selectedFiles().

Other state are not altered or modified during the process, just the selected files are set to setSelectedFiles.

jayathirthrao · 2022-09-08T10:32:32Z

src/java.desktop/share/classes/sun/swing/FilePane.java

+
+            if (selectedFiles != null) {
+                getFileChooser().setSelectedFiles(selectedFiles);
+            }
        } else {
            listSelectionModel.setSelectionMode(ListSelectionModel.SINGLE_SELECTION);
            clearSelection();


When we move from multi-selection enabled to disable. We are clearing the selection and calling setSelectedFiles(null). Do we need to replicate similar state update when we move from multi-selection disable to enable (clearing selection and calling setSelectedFile(null))?

It looks like somewhere we are not clearing SelectedFile() and it is causing this issue.

If we replicate the same then previously selected files will be cleared and user wont be able to retain the selected files.

Clearing the selection from multi to single is fine, but the reverse might not be right I guess..... Since selected file/files can be retained when user switch to multi selection.

In this scenario we are actually closing the first JFileChooser dialog and then opening another JFileChooser dialog with multi-selection. Do we need to maintain selected file state between these dialogs? If yes, why? Please clarify.

In mac, the selection is maintained during this state transition, only in this scenario its not handled. And since same filechooser is used in selecting the files ( Just pop-up dialogue is used to select the files) I think it would be better to retain the selection.

In keyboard selection the state is actually not maintained. Then do you think it better to clear the selection and set selected files to null....?

Clearing the selection and setting it to null also solves the problem though. When we call clear selection, reset will happen and selectedFiles will be set.

As discussed on call its better to clear selection when we move between multi-selection enabled/disabled as captured in #9996 (comment).

Because using getSelectedFile() to update setSelectedFiles() might cause state management issues in some other scenarios.

Yeah, I have updated as discussed on call and tested.

TejeshR13 · 2022-09-09T08:11:47Z

@kumarabhi006 U might have to re-review the PR as we have changed the logic. Previously the selected flies state was retained, now the state is been cleared on MultiSelection enabled/disabled switch.

jayathirthrao · 2022-09-09T11:03:01Z

@TejeshR13 I see some test failures in latest CI job like javax/swing/JTree/4618767/JTreeSelectedElementTest.java.
Have you verified these are not regressions introduced by this change?

TejeshR13 · 2022-09-09T11:05:09Z

@TejeshR13 I see some test failures in latest CI job like javax/swing/JTree/4618767/JTreeSelectedElementTest.java. Have you verified these are not regressions introduced by this change?

Yeah, those tests are failing without my changes too, so confirmed that it wasn't from my change. Some tests are failing without my fix also, so to ensure that my fix doesn't cause any regression I have tested without my fix and shared the link in JBS.

jayathirthrao · 2022-09-09T11:16:17Z

@TejeshR13 I see some test failures in latest CI job like javax/swing/JTree/4618767/JTreeSelectedElementTest.java. Have you verified these are not regressions introduced by this change?

Yeah, those tests are failing without my changes too, so confirmed that it wasn't from my change. Some tests are failing without my fix also, so to ensure that my fix doesn't cause any regression I have tested without my fix and shared the link in JBS.

Thanks for adding CI run without fix also in JBS. LGTM.

kumarabhi006 · 2022-09-09T11:32:25Z

@kumarabhi006 U might have to re-review the PR as we have changed the logic. Previously the selected flies state was retained, now the state is been cleared on MultiSelection enabled/disabled switch.

Applied the patch and tested, attached test case is passed with your recent fix.

openjdk · 2022-09-09T11:33:52Z

@TejeshR13 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

4834298: JFileChooser.getSelectedFiles() failed with multi-selection and double-click

Reviewed-by: jdv

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 378 new commits pushed to the master branch:

3dd94f3: 8292671: Hotspot Style Guide should allow covariant returns
9d6b028: 8234315: GTK LAF does not gray out disabled JMenu
812d805: 6447816: Provider filtering (getProviders) is not working with OR'd conditions
43e191d: 8293524: RISC-V: Use macro-assembler functions as appropriate
14eb5ad: 8291753: Add JFR event for GC CPU Time
30d4145: 8293230: x86_64: Move AES and GHASH stub definitions into separate source files
4c5501c: 8293548: ProblemList sun/management/jmxremote/bootstrap/RmiBootstrapTest.java#id1 on linux-x64
46e6e41: 8293051: Further refactor javac after removal of -source/-target/--release 7
c0ee30a: 8293348: A false cyclic inheritance error reported
85ec1f8: 8293492: ShenandoahControlThread missing from hs-err log and thread dump
... and 368 more: https://git.openjdk.org/jdk/compare/77398430b5e13768cddd5f63e8fe9e53735bbea8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jayathirthrao) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

TejeshR13 · 2022-09-09T11:46:44Z

/integrate

openjdk · 2022-09-09T11:48:19Z

@TejeshR13
Your change (at version c437cdf) is now ready to be sponsored by a Committer.

jayathirthrao · 2022-09-13T04:52:18Z

/sponsor

openjdk · 2022-09-13T04:54:25Z

Going to push as commit 9cd3e35.
Since your change was applied there have been 403 commits pushed to the master branch:

ec2629c: 8275275: AArch64: Fix performance regression after auto-vectorization on NEON
cbee0bc: 8292587: AArch64: Support SVE fabd instruction
68645eb: 8293566: RISC-V: Clean up push and pop registers
526eb54: 8293669: SA: Remove unnecssary "InstanceStackChunkKlass: InstanceStackChunkKlass" output when scanning heap
41ce658: 8292225: Rename ArchiveBuilder APIs related to source and buffered addresses
155b10a: 8293329: x86: Improve handling of constants in AES/GHASH stubs
d3f7e3b: 8293339: vm/jvmti/StopThread/stop001/stop00103 crashes with SIGSEGV in Continuation::is_continuation_mounted
524af94: 8283627: Outdated comment in MachineDescriptionTwosComplement.isLP64
cea409c: 8292738: JInternalFrame backgroundShadowBorder & foregroundShadowBorder line is longer in Mac Look and Feel
9ef6c09: 8287908: Use non-cloning reflection methods where acceptable
... and 393 more: https://git.openjdk.org/jdk/compare/77398430b5e13768cddd5f63e8fe9e53735bbea8...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-13T04:54:46Z

@jayathirthrao @TejeshR13 Pushed as commit 9cd3e35.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

TejeshR13 added 12 commits January 2, 2022 20:01

Initial Commit.

288c597

Merge branch 'master' of github.com:TejeshR13/jdk

2403494

Merge remote-tracking branch 'upstream/master'

92823e0

Merge branch 'openjdk:master' into master

347af6e

Merge remote-tracking branch 'upstream/master'

98cf2b7

Merge branch 'master' of github.com:TejeshR13/jdk

8788395

Merge remote-tracking branch 'upstream/master'

a6d536c

Merge remote-tracking branch 'upstream/master'

c36d137

Merge remote-tracking branch 'upstream/master'

5a80d75

Merge remote-tracking branch 'upstream/master'

a7788e4

Merge remote-tracking branch 'upstream/master'

b0c4ead

Fix - Updated filepane with fix and added test case

7129054

openjdk bot added the rfr Pull request is ready for review label Aug 24, 2022

openjdk bot added the client client-libs-dev@openjdk.org label Aug 24, 2022

turbanoff reviewed Sep 1, 2022

View reviewed changes

Updated based on review comments

71c936a

kumarabhi006 reviewed Sep 6, 2022

View reviewed changes

Updated based on review comments

1d7f271

kumarabhi006 approved these changes Sep 6, 2022

View reviewed changes

jayathirthrao reviewed Sep 8, 2022

View reviewed changes

Updated based on review comments

c437cdf

jayathirthrao approved these changes Sep 9, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 9, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 9, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 13, 2022

openjdk bot closed this Sep 13, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 13, 2022

4834298: JFileChooser.getSelectedFiles() failed with multi-selection and double-click #9996

4834298: JFileChooser.getSelectedFiles() failed with multi-selection and double-click #9996

Conversation

TejeshR13 commented Aug 24, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 24, 2022

openjdk bot commented Aug 24, 2022

openjdk bot commented Aug 24, 2022

mlbridge bot commented Aug 24, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumarabhi006 commented Sep 6, 2022

TejeshR13 commented Sep 6, 2022

kumarabhi006 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejeshR13 Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayathirthrao Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejeshR13 commented Sep 9, 2022

jayathirthrao commented Sep 9, 2022

TejeshR13 commented Sep 9, 2022 • edited Loading

jayathirthrao commented Sep 9, 2022

kumarabhi006 commented Sep 9, 2022

openjdk bot commented Sep 9, 2022 • edited Loading

TejeshR13 commented Sep 9, 2022

openjdk bot commented Sep 9, 2022

jayathirthrao commented Sep 13, 2022

openjdk bot commented Sep 13, 2022

openjdk bot commented Sep 13, 2022

TejeshR13 commented Aug 24, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 24, 2022 •

edited

Loading

TejeshR13 Sep 8, 2022 •

edited

Loading

jayathirthrao Sep 8, 2022 •

edited

Loading

TejeshR13 commented Sep 9, 2022 •

edited

Loading

openjdk bot commented Sep 9, 2022 •

edited

Loading