6972078: Can not select single directory with GTKLookAndFeel #10866

kumarabhi006 · 2022-10-26T04:34:24Z

While using a JFileChooser with the file selection mode FILES_AND_DIRECTORIES and multiSelection enabled, it is impossible to select a single directory in GTK LAF.

The condition check has been modified when multiselection is enabled and user has selected single directory.
After the fix the behavior of JFileChooser is similar to other LAFs.

An automatic test case test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java is implemented and checked in CI pipeline. Link is attached in JBS.

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-6972078: Can not select single directory with GTKLookAndFeel

Reviewers

Prasanta Sadhukhan (@prsadhuk - Reviewer)
Tejesh R (@TejeshR13 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10866

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

Using diff file

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

bridgekeeper · 2022-10-26T04:34:37Z

👋 Welcome back kumarabhi006! 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-10-26T04:37:16Z

@kumarabhi006 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-10-26T05:18:57Z

Webrevs

TejeshR13 · 2022-10-26T10:30:48Z

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java

+                            pt.y + getSelectedFilesButton.getHeight() / 2);
+            robot.mousePress(InputEvent.BUTTON1_DOWN_MASK);
+            robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK);
+            robot.delay(100);


robot.mouseMove(pt.x + getSelectedFilesButton.getWidth() / 2, pt.y + getSelectedFilesButton.getHeight() / 2); robot.mousePress(InputEvent.BUTTON1_DOWN_MASK); robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); robot.delay(100);

ClickMouse method can be reused instead.

There is an offset added in x-position. That's why ClickMouse is not reused.

Yeah, compute the offset in main method and just pass the point. So that whole method can be reused.

prsadhuk · 2022-10-27T04:00:35Z

Can you also please test it works ok (or atleast not regress) for other selection mode FILES_ONLY, DIRECTORIES_ONLY in the test?

After the fix the behavior of JFileChooser is similar to other LAFs.

Can you then please enhance the test to iterate for all installed L&Fs?

prsadhuk · 2022-10-27T05:48:53Z

Also, please see if JDK-4912623 is related and fixed by this?

kumarabhi006 · 2022-10-27T08:37:48Z

Can you also please test it works ok (or atleast not regress) for other selection mode FILES_ONLY, DIRECTORIES_ONLY in the test?

Verified for file selection mode of DIRECTORIES_ONLY, test passed for all LAFs.

In case of selection mode of FILES_ONLY, test is failing for all LAFs. The reason behind this is we are trying to select Directory when selection mode is set to FILES_ONLY.

Can you then please enhance the test to iterate for all installed L&Fs?

Updated for all installed LAFs.

kumarabhi006 · 2022-10-27T08:48:42Z

Also, please see if JDK-4912623 is related and fixed by this?

Ok, I will check.

prsadhuk · 2022-10-27T09:27:00Z

Can you also please test it works ok (or atleast not regress) for other selection mode FILES_ONLY, DIRECTORIES_ONLY in the test?

Verified for file selection mode of DIRECTORIES_ONLY, test passed for all LAFs.

In case of selection mode of FILES_ONLY, test is failing for all LAFs. The reason behind this is we are trying to select Directory when selection mode is set to FILES_ONLY.

I guess you can see in folders where you have only files...Testwise, maybe use File.createTempFile for creating files and see..

kumarabhi006 · 2022-10-27T09:37:07Z

Can you also please test it works ok (or atleast not regress) for other selection mode FILES_ONLY, DIRECTORIES_ONLY in the test?

Verified for file selection mode of DIRECTORIES_ONLY, test passed for all LAFs.
In case of selection mode of FILES_ONLY, test is failing for all LAFs. The reason behind this is we are trying to select Directory when selection mode is set to FILES_ONLY.

I guess you can see in folders where you have only files...Testwise, maybe use File.createTempFile for creating files and see..

I have checked manually, it is possible to select single or multiple file when selection mode is FILES_ONLY for all LAFs.

I will try to update test file.

kumarabhi006 · 2022-11-02T10:55:29Z

I guess you can see in folders where you have only files...Testwise, maybe use File.createTempFile for creating files and see..

I tried modifying the test case for different selection mode for all LAFs.
In case of FILES_AND_DIRECTORIES and DIRECTORIES_ONLY, the test passed in all LAFs.
But when the selection mode is set to FILES_ONLY, the UI in GTK doesn't show the file lists and test is failing.

However If I test manually for each selection mode, the behavior is same for all LAFs.

In case of FILES_AND_DIRECTORIES, able to select single as well as multiple directories or files.
In case of DIRECTORIES_ONLY, able to select single as well as multiple directories but not files.
In case of FILES_ONLY, able to select single as well as multiple files but not directories.

kumarabhi006 · 2022-11-03T12:01:42Z

@prsadhuk As discussed, created the sub-test for all file selection mode. Checked with all LAFs and in CI pipeline, link is added in JBS.

prsadhuk · 2022-11-04T06:29:31Z

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java

+            Point frameLocation = fileChooser.getLocationOnScreen();
+            int frameWidth = frame.getWidth();
+            int frameHeight = frame.getHeight();
+
+            Point btnLocation = getSelectedFilesButton.getLocationOnScreen();
+            int btnWidth = getSelectedFilesButton.getWidth();
+            int btnHeight = getSelectedFilesButton.getHeight();
+            clickMouse(frameLocation, 0, frameHeight, 230);
+            clickMouse(btnLocation, btnWidth, btnHeight, 0);
+            checkResult(laf);


I guess you can optimise by a helper method for this repeated code passing the offset value as paramter.

Also, did you check JDK-4912623 as mentioned?

Added a helper method for the repeated code.

I tried to reproduce the JDK-4912623 bug. SwingSet2 JFileChooser demo behaves as it is mentioned in the bug description but in Gedit I didn't find separate folder and files list.

The bug says
2) Go to FileChooser Demo.
3) Tab to Folder List
4) Press CTRL+A. It will select all the items in the list.
5) Now Tab to file list and press ctrl+A, it will not select all the items.
6) Now Open Gedit.
7) Click Open.
8) Repeat the steps 3 to 5. Behaviour for the native will be opposite to its Java counterpart.

so you can create manually folder with only files and a folder with only folders and then try with Gedit to open those folders and trey the steps...

The bug says 2) Go to FileChooser Demo. 3) Tab to Folder List 4) Press CTRL+A. It will select all the items in the list. 5) Now Tab to file list and press ctrl+A, it will not select all the items. 6) Now Open Gedit. 7) Click Open. 8) Repeat the steps 3 to 5. Behaviour for the native will be opposite to its Java counterpart.

so you can create manually folder with only files and a folder with only folders and then try with Gedit to open those folders and trey the steps...

I tried these steps, I am able to select all directories and all files in respective folder on press of Ctrl+A in Gedit.

Is GTK Filechooser not behaving the same as gedit?
ie is GTK FileChooser not allowing to select all folders in folder list and all files in files list by CTRL+A with your fix too?

So, it is same without the fix too as is mentioned in that bug.

Yeah, it behaves same without the fix also.

Can you please see why that is so as it should be exercising the same code path as this fix?

Ok, I will check.

Can you please see why that is so as it should be exercising the same code path as this fix?

GTK FileChooser allows to select all folders in folder list but in files list it allows only single file selection. The reason behind this is while creating the GTKFileChooserUI, filelist selection model is set to SINGLE_SELECTION if multiselection is disabled. Code snippet is attached from protected JScrollPane createFilesList() method.

if (getFileChooser().isMultiSelectionEnabled()) { fileList.setSelectionMode(ListSelectionModel.MULTIPLE_INTERVAL_SELECTION); } else { fileList.setSelectionMode(ListSelectionModel.SINGLE_SELECTION); }

But for directory list there is no such condition is present and due to that it allows to select multiple folders in directory list with ctrl+A.

@prsadhuk As discussed I have checked with the condition modified to chooser.getFileSelectionMode() != JFileChooser.FILES_ONLY, but this leads to failure for current test in FILES_AND_DIRECTORIES and DIRECTORIES_ONLY selection mode.

And for JDK-4912623, still not able to select all files in JFileChooser on press of ctrl+A.

turbanoff · 2022-11-06T16:15:34Z

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java

+    }
+
+    private static void checkDirectoriesOnlyTest(UIManager.LookAndFeelInfo laf)
+            throws Exception  {


Suggested change

throws Exception {

throws Exception {

turbanoff · 2022-11-06T16:15:40Z

test/jdk/com/sun/java/swing/plaf/gtk/TestFileChooserSingleDirectorySelection.java

+    }
+
+    private static void checkFilesAndDirectoriesTest(UIManager.LookAndFeelInfo laf)
+            throws Exception  {


Suggested change

throws Exception {

throws Exception {

openjdk · 2022-11-10T13:36:17Z

@kumarabhi006 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:

6972078: Can not select single directory with GTKLookAndFeel

Reviewed-by: psadhukhan, tr

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 163 new commits pushed to the master branch:

4465361: 8295948: Support for Zicbop/prefetch instructions on RISC-V
f2acdfd: 8296638: RISC-V: NegVI node emits wrong code when vector element basic type is T_BYTE/T_SHORT
bfc5816: 8295475: Move non-resource allocation strategies out of ResourceObj
e802b12: 8296196: Class.getEnumConstants() throws undocumented ClassCastException and NullPointerException
78a08a0: 8295430: Use cmsDoTransformLineStride instead of cmsDoTransform in the loop
f0a6e71: 8295812: Skip the "half float" support in LittleCMS during the build
79c0092: 8285635: javax/swing/JRootPane/DefaultButtonTest.java failed with Default Button not pressed for L&F: com.sun.java.swing.plaf.motif.MotifLookAndFeel
0981bfb: 8296156: [macos] Resize DMG windows and background to fit additional DMG contents
93fed9b: 8296448: RISC-V: Fix temp usages of heapbase register killed by MacroAssembler::en/decode_klass_not_null
d6e2d0d: 8296611: Problemlist several sun/security tests until JDK-8295343 is resolved
... and 153 more: https://git.openjdk.org/jdk/compare/324bec19aa9b9d4944a7e1129d494d57a077ba51...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 (@prsadhuk, @TejeshR13) 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).

kumarabhi006 · 2022-11-10T13:51:03Z

/integrate

openjdk · 2022-11-10T13:53:17Z

@kumarabhi006
Your change (at version 5c5c564) is now ready to be sponsored by a Committer.

TejeshR13 · 2022-11-10T14:08:47Z

/sponsor

openjdk · 2022-11-10T14:12:09Z

Going to push as commit 4a68210.
Since your change was applied there have been 163 commits pushed to the master branch:

4465361: 8295948: Support for Zicbop/prefetch instructions on RISC-V
f2acdfd: 8296638: RISC-V: NegVI node emits wrong code when vector element basic type is T_BYTE/T_SHORT
bfc5816: 8295475: Move non-resource allocation strategies out of ResourceObj
e802b12: 8296196: Class.getEnumConstants() throws undocumented ClassCastException and NullPointerException
78a08a0: 8295430: Use cmsDoTransformLineStride instead of cmsDoTransform in the loop
f0a6e71: 8295812: Skip the "half float" support in LittleCMS during the build
79c0092: 8285635: javax/swing/JRootPane/DefaultButtonTest.java failed with Default Button not pressed for L&F: com.sun.java.swing.plaf.motif.MotifLookAndFeel
0981bfb: 8296156: [macos] Resize DMG windows and background to fit additional DMG contents
93fed9b: 8296448: RISC-V: Fix temp usages of heapbase register killed by MacroAssembler::en/decode_klass_not_null
d6e2d0d: 8296611: Problemlist several sun/security tests until JDK-8295343 is resolved
... and 153 more: https://git.openjdk.org/jdk/compare/324bec19aa9b9d4944a7e1129d494d57a077ba51...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-11-10T14:12:38Z

@TejeshR13 @kumarabhi006 Pushed as commit 4a68210.

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

kiraka42

lgtm

Single directory selection fix

99f317d

openjdk bot added the client client-libs-dev@openjdk.org label Oct 26, 2022

Whitespace error fix

551a790

openjdk bot added the rfr Pull request is ready for review label Oct 26, 2022

TejeshR13 reviewed Oct 26, 2022

View reviewed changes

kumarabhi006 added 2 commits October 26, 2022 19:24

Merge

b87bd67

Review comment fix

c129d18

Test for all installed LAF

970eca4

Subtest added for file selection mode

8e7adb1

prsadhuk reviewed Nov 4, 2022

View reviewed changes

Review comment fix

0b7d1a6

turbanoff reviewed Nov 6, 2022

View reviewed changes

Review comment fix

5c5c564

prsadhuk approved these changes Nov 10, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 10, 2022

TejeshR13 approved these changes Nov 10, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 10, 2022

openjdk bot added the integrated Pull request has been integrated label Nov 10, 2022

openjdk bot closed this Nov 10, 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 Nov 10, 2022

kiraka42 reviewed Nov 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6972078: Can not select single directory with GTKLookAndFeel #10866

6972078: Can not select single directory with GTKLookAndFeel #10866

kumarabhi006 commented Oct 26, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 26, 2022

openjdk bot commented Oct 26, 2022

mlbridge bot commented Oct 26, 2022 •

edited

Loading

TejeshR13 Oct 26, 2022

kumarabhi006 Oct 26, 2022

TejeshR13 Oct 26, 2022 •

edited

Loading

kumarabhi006 Oct 26, 2022

prsadhuk commented Oct 27, 2022

prsadhuk commented Oct 27, 2022

kumarabhi006 commented Oct 27, 2022

kumarabhi006 commented Oct 27, 2022

prsadhuk commented Oct 27, 2022

kumarabhi006 commented Oct 27, 2022

kumarabhi006 commented Nov 2, 2022 •

edited

Loading

kumarabhi006 commented Nov 3, 2022

prsadhuk Nov 4, 2022

kumarabhi006 Nov 4, 2022

prsadhuk Nov 4, 2022

kumarabhi006 Nov 4, 2022

prsadhuk Nov 4, 2022

kumarabhi006 Nov 4, 2022

kumarabhi006 Nov 4, 2022

kumarabhi006 Nov 8, 2022

kumarabhi006 Nov 10, 2022

prsadhuk Nov 10, 2022

turbanoff Nov 6, 2022

turbanoff Nov 6, 2022

openjdk bot commented Nov 10, 2022 •

edited

Loading

kumarabhi006 commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

TejeshR13 commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

kiraka42 left a comment •

edited by bridgekeeper bot

Loading

6972078: Can not select single directory with GTKLookAndFeel #10866

6972078: Can not select single directory with GTKLookAndFeel #10866

Conversation

kumarabhi006 commented Oct 26, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 26, 2022

openjdk bot commented Oct 26, 2022

mlbridge bot commented Oct 26, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TejeshR13 Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsadhuk commented Oct 27, 2022

prsadhuk commented Oct 27, 2022

kumarabhi006 commented Oct 27, 2022

kumarabhi006 commented Oct 27, 2022

prsadhuk commented Oct 27, 2022

kumarabhi006 commented Oct 27, 2022

kumarabhi006 commented Nov 2, 2022 • edited Loading

kumarabhi006 commented Nov 3, 2022

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

openjdk bot commented Nov 10, 2022 • edited Loading

kumarabhi006 commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

TejeshR13 commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

openjdk bot commented Nov 10, 2022

kiraka42 left a comment • edited by bridgekeeper bot Loading

Choose a reason for hiding this comment

kumarabhi006 commented Oct 26, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 26, 2022 •

edited

Loading

TejeshR13 Oct 26, 2022 •

edited

Loading

kumarabhi006 commented Nov 2, 2022 •

edited

Loading

openjdk bot commented Nov 10, 2022 •

edited

Loading

kiraka42 left a comment •

edited by bridgekeeper bot

Loading