Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

6777156: GTK L&F: JFileChooser can jump beyond root directory in combobox and selection textarea. #10390

Closed
wants to merge 5 commits into from

Conversation

kumarabhi006
Copy link
Contributor

@kumarabhi006 kumarabhi006 commented Sep 22, 2022

When a user selects ../ from directory list in GTK FileChooser continuosly, it leads to '../../../.....' in combobox and selection textarea even though it the current directory is at root level.

Fix for the issue is to remove "../" entry from JFileChooser directory list in GTK LAF if current directory is root.

Test case has been added 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-6777156: GTK L&F: JFileChooser can jump beyond root directory in combobox and selection textarea.

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10390

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 22, 2022

👋 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 openjdk bot added the rfr Pull request is ready for review label Sep 22, 2022
@openjdk
Copy link

openjdk bot commented Sep 22, 2022

@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.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Sep 22, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 22, 2022

Webrevs

@kumarabhi006 kumarabhi006 changed the title 6777156: GTK JFileChooser can show /../../../.. in combobox and selection textarea 6777156: GTK L&F: JFileChooser shouldn't display /../../../.. in combobox and selection textarea. Sep 22, 2022
@@ -432,6 +432,10 @@ public DoubleClickListener(JList<?> list) {
public void mouseClicked(MouseEvent e) {
if (SwingUtilities.isLeftMouseButton(e) && e.getClickCount() == 2) {
int index = list.locationToIndex(e.getPoint());
File currentDirectory = getFileChooser().getCurrentDirectory();
if (currentDirectory.getParentFile() == null && index == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a Comment explaining the statement index == 1 would be better to understand that it signifies root directory index.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment.
Basically, getParentFile method is used to check root directory and index == 1 is the index of ../ directory.

if (prevDir == crntDir) {
passed = true;
break;
} else if (++i > 5) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retries will be 4 right......? Since you are starting with 2 till 5......?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the retries to be 5 and then break to avoid infinite loop.

@TejeshR13
Copy link
Contributor

Is there any possibility to check for current Directory to be root before confirming that the traversal is stopping at root?

@kumarabhi006
Copy link
Contributor Author

Is there any possibility to check for current Directory to be root before confirming that the traversal is stopping at root?

Added the condition to check if the current directory is root.

Copy link
Member

@jayathirthrao jayathirthrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the behaviour in other LAF's?
Also lets change title of this bug to something like "GTK L&F: JFileChooser can jump beyond root directory in combobox and selection textarea"

@@ -432,6 +432,18 @@ public DoubleClickListener(JList<?> list) {
public void mouseClicked(MouseEvent e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this issue reproducible only through mouse click event? Can we use key navigation and return button to see same issue? Is button press using keyboard navigation treated as mouseClick event?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same issue has been observed in case of key navigation also. we need to handle those scenario where a user is navigating through keys. I will check and update accordingly as we discussed over call.

});

robot.waitForIdle();
robot.delay(1000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After robot.setAutoDelay() why we need such large delays in robot at many places?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check this and update.

}

private static void doubleClickMouse(Point p) {
robot.mouseMove(p.x+75, p.y+frame.getHeight()/2 - 90);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not use magic numbers to determine location on screen. Will these coordinates hold true at different uiScales?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test case has been changed to manual now. So, there is no need of getting location on screen now.

@kumarabhi006
Copy link
Contributor Author

What is the behaviour in other LAF's?

I have checked in Nimbus and Metal LAF, there is a separate button in both LAF to move to previous directory and it is disabled when current directory is root.

@kumarabhi006 kumarabhi006 changed the title 6777156: GTK L&F: JFileChooser shouldn't display /../../../.. in combobox and selection textarea. 6777156: GTK L&F: JFileChooser can jump beyond root directory in combobox and selection textarea. Sep 27, 2022
@kumarabhi006
Copy link
Contributor Author

@jayathirthrao @TejeshR13 As compared to previous fix where the condition was handled at listener level (mouse double click event), has been changed in current fix.

Now as per the current fix the proposed solution is to remove the "../" from directory list if the current directory is root in GTK LAF.

Test case has been updated to manual test as compared to previous fix.
CI link is added in JBS for latest commit.

Copy link
Member

@jayathirthrao jayathirthrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its good that now we are handling entries in directory list at appropriate place instead of modifying it at some event listeners. LGTM.

@openjdk
Copy link

openjdk bot commented Oct 11, 2022

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

6777156: GTK L&F: JFileChooser can jump beyond root directory in combobox and selection textarea.

Reviewed-by: jdv, tr, psadhukhan

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

  • 33d0618: 6616245: NullPointerException when using JFileChooser with a custom FileView
  • fba763f: 8291519: jdk/jfr/api/event/TestShouldCommit.java failed with Unexpected value of shouldCommit()
  • 6053bf0: 8293782: Shenandoah: some tests failed on lock rank check
  • 4435d56: 8282395: URL.openConnection can throw IOOBE
  • fe70487: 8294958: java/net/httpclient/ConnectTimeout tests are slow
  • 97f1321: 8294356: IGV: scheduled graphs contain duplicated elements
  • 5e05e42: 8294901: remove pre-VS2017 checks in Windows related coding
  • e775acf: 8293986: Incorrect double-checked locking in com.sun.beans.introspect.ClassInfo
  • 9d116ec: 8294262: AArch64: compiler/vectorapi/TestReverseByteTransforms.java test failed on SVE machine
  • 4b17d28: 8294261: AArch64: Use pReg instead of pRegGov when possible
  • ... and 254 more: https://git.openjdk.org/jdk/compare/1b496064bf1c5078b6f753282771996e0d1b94d6...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, @prsadhuk) 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).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 11, 2022
*/
File crntDir = getFileChooser().getCurrentDirectory();
if (crntDir != null && crntDir.getParentFile() == null &&
files.contains(new File("/.."))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not possible to use "FileSystemView.isFileSystemRoot(File)" for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not checked this.
Do you mean to check FileSystemView.isFileSystemRoot(crntDir ) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's what you commented "if current directory is root"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok... I will check this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it not possible to use "FileSystemView.isFileSystemRoot(File)" for this?

Updated.

* root file system.
* @run main/manual TestFileChooserDirectorySelection
*/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to put the test in plaf/gtk folder as it fixed gtk issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test file moved to plaf/gtk folder.

@prsadhuk
Copy link
Contributor

Please confirm that along with ubuntu, you have tested on Oracle Linux too.

@TejeshR13
Copy link
Contributor

Fix works fine and tested.

@kumarabhi006
Copy link
Contributor Author

Please confirm that along with ubuntu, you have tested on Oracle Linux too.

I have checked in Oracle Linux, Fix is working fine.

@kumarabhi006
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 11, 2022
@openjdk
Copy link

openjdk bot commented Oct 11, 2022

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

@jayathirthrao
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented Oct 11, 2022

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

  • 3ebe5ad: 8294751: Zero: Allow larger default heaps
  • 33d0618: 6616245: NullPointerException when using JFileChooser with a custom FileView
  • fba763f: 8291519: jdk/jfr/api/event/TestShouldCommit.java failed with Unexpected value of shouldCommit()
  • 6053bf0: 8293782: Shenandoah: some tests failed on lock rank check
  • 4435d56: 8282395: URL.openConnection can throw IOOBE
  • fe70487: 8294958: java/net/httpclient/ConnectTimeout tests are slow
  • 97f1321: 8294356: IGV: scheduled graphs contain duplicated elements
  • 5e05e42: 8294901: remove pre-VS2017 checks in Windows related coding
  • e775acf: 8293986: Incorrect double-checked locking in com.sun.beans.introspect.ClassInfo
  • 9d116ec: 8294262: AArch64: compiler/vectorapi/TestReverseByteTransforms.java test failed on SVE machine
  • ... and 255 more: https://git.openjdk.org/jdk/compare/1b496064bf1c5078b6f753282771996e0d1b94d6...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 11, 2022
@openjdk openjdk bot closed this Oct 11, 2022
@openjdk 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 Oct 11, 2022
@openjdk
Copy link

openjdk bot commented Oct 11, 2022

@jayathirthrao @kumarabhi006 Pushed as commit 9d0009e.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants