Fix add libraries dialog not working on Linux #434

0dinD · 2021-01-05T14:08:20Z

Fixes #238

When trying to add a library to Referenced Libraries for invisible projects on Linux, all files are grayed out (unselectable), and you can't even navigate into folders. This makes it impossible to add libraries for invisible projects on Linux, unless you modify the settings.json manually.

This is caused by the unclear API docs for OpenDialogOptions, which say:

A dialog can select files, folders, or both. This is not true for Windows which enforces to open either files or folder, but not both.

This extension then assumes that canSelectFiles and canSelectFolders can be activated at the same time on both macOS and Linux - but it's actually unsupported on both Windows and Linux (which is the root cause of the issue).

Behind the scenes, VS Code is using the Electron API to show the file dialog, and the whole situation actually becomes much clearer if you read the Electron docs for dialog:

Note: On Windows and Linux an open dialog can not be both a file selector and a directory selector, so if you set properties to ['openFile', 'openDirectory'] on these platforms, a directory selector will be shown.

So what currently happens is that this extension requests a file dialog that can select both files and folders on Linux, which Electron then converts to a file dialog that can only select folders. That means you can't select any Jar files, and you also can't select (or navigate into) any folders, because they don't match the jar file filter.

30ab944 is a very simple fix that just makes sure to only offer directory selection on macOS.

But after implementing this, I felt like it was a bit of a shame that Windows and Linux users were missing this QOL feature, just because of some Electron or OS limitation. So in b2d3702 I added a separate button for adding folders to the Referenced Libraries, so that users can also add library folders on Windows and Linux.

Let me know if these changes sound good, and if the naming makes sense for all the buttons/tooltips. I would also like some help with the Chinese translation for the new/updated button tooltips, since I don't know any (and I don't really trust Google Translate to do a good job).

ghost · 2021-01-05T14:08:35Z

All CLA requirements met.

0dinD · 2021-01-05T14:22:02Z

Not sure why the test failed on Linux, the errors in the log don't seem to be related to the changes in this PR. And I've ran the tests successfully on my development machine (running Debian 10).

jdneo · 2021-01-06T00:15:56Z

Thank you @0dinD, I'll take a look!

jdneo · 2021-01-06T05:53:14Z

@0dinD Thank you for the detailed explanation. And the fix looks promising!

To me, personally I prefer the way 30ab944 does for this issue. b2d3702 is also great, but it introduces a new command and buttons . I'm not sure how users can easily get the point between add file and add folder.

So I tend to keep it simple:

mac: files and folders are both fine
linux & win: files only

But, we can definitely revisit this if we see more and more users are asking adding folder support on Linux and Windows. What do you think?

0dinD · 2021-01-06T23:00:01Z

@jdneo I do agree that keeping it simple is preferable over adding a new button.

An alternative solution that I just came to think of is to make the java.project.addLibraryFolders command the "alt" action for the same button (see the docs). This way, most users will only see the normal addLibraries button, while more advanced users who know about the alt command can still easily add "library folders". What do you think about that solution?

We could also add both commands to the context menu of Referenced Libraries.

jdneo · 2021-01-07T00:46:04Z

@0dinD I think that's a good idea. Will you update the PR for that?

0dinD · 2021-01-07T06:12:16Z

@jdneo I'll update the PR later today. But since it still requires two separate commands, I do still need translations for the new command titles. Are you able to help with that?

jdneo · 2021-01-07T06:15:47Z

No worry, I'll append my commit to this PR :)

0dinD · 2021-01-07T16:02:42Z

OK, I've addressed the changes we discussed in 4665d8c.

Note that on Linux, Alt + click is typically reserved by the desktop environment for moving windows around, so nothing will happen when you Alt + click the inline button, even though the icon for the alt command is visible. This can be fixed by either rebinding the window moving key in the system settings, or by using Ctrl + Alt + click, or Shift + click, which will still trigger the alt action in VS Code.

jdneo · 2021-01-08T02:03:23Z

src/controllers/libraryController.ts

        const setting = Settings.referencedLibraries();
        setting.exclude = this.dedupAlreadyCoveredPattern(libraryGlobs, ...setting.exclude);
        setting.include = this.updatePatternArray(setting.include, ...libraryGlobs);
        Settings.updateReferencedLibraries(setting);
    }

+    public async addLibraries(folders?: boolean) {


Change the param name to canSelectFolders to just align with the API?

Makes sense, fixed in a735dc5.

jdneo · 2021-01-08T02:20:28Z

src/controllers/libraryController.ts

+        const results: Uri[] | undefined = await window.showOpenDialog({
+            defaultUri: workspaceFolder && workspaceFolder.uri,
+            canSelectFiles: !folders,
+            canSelectFolders: folders,


One possible regression here is: for the Mac users, they actually lose the capability to select both files and folders in one dialog.

So maybe still also check if it is in MacOS?

I actually think it's better for the sake of simplicity to keep folder selection specific to the more advanced alt command. Since I don't think we can change the command title based on OS (without creating a third command that only displays on macOS), there is no consistent way to communicate with the user that both Jar files and folders can be selected when running macOS. However, I do agree that removing the capability is a regression (which might bother some users), so I added back folder selection to the addLibraries dialog on macOS in a735dc5.

jdneo · 2021-01-11T04:50:02Z

src/controllers/libraryController.ts

+            // keep the param: `includeWorkspaceFolder` to false here
+            // since the multi-root is not supported well for invisible projects
+            const uriPath = workspace.asRelativePath(uri, false);
+            return canSelectFolders ? uriPath + "/**/*.jar" : uriPath;


When it's on MacOS, and triggered from addLibraries (canSelectFolders is undefined), if the user selects a folder, then it will not append /**/*.jar.

So maybe we still need the previous logic -- check if it is a folder or not and return different value accordingly.

Good catch! I missed that since I don't have a mac to test on, but I'll add the old logic back.

Fixed in 2197367 (although I can't test if it works, since I don't have a mac).

jdneo · 2021-01-11T07:46:03Z

Chinese translation added.

BTW, I update Remove Library from Project Classpath to Remove from Project Classpath since I think there is no need to say remove 'what' in the command title.

jdneo

Verified on Mac and Windows, all works fine!

Thank you @0dinD !

0dinD · 2021-01-13T15:17:06Z

No problem, glad I could help!

FYI @jdneo the issue fixed in this PR seems to be related to a recent negative review on the Java Extension Pack and the Language Support for Java extension. I can see you've already referenced this PR on the review left on this extension, but it would probably be a good idea to respond to all reviews (even though they're from the same user), as it might help others. I don't know about the extension pack, but I'm guessing @fbricon or @rgrunber is responsible for the Java extension.

0dinD added 2 commits January 4, 2021 22:38

Fix 'addLibraries' dialog not working on Linux

30ab944

Add separate button for adding folders to 'Referenced Libraries'

b2d3702

0dinD mentioned this pull request Jan 5, 2021

Clarify/correct OpenDialogOptions note microsoft/vscode#113831

Closed

jdneo added this to the 0.17.0 milestone Jan 7, 2021

jdneo self-requested a review January 7, 2021 07:54

jdneo added bug Something isn't working enhancement New feature or request labels Jan 7, 2021

Make addLibraryFolders an alt-command instead of separate button

4665d8c

jdneo reviewed Jan 8, 2021

View reviewed changes

Address review comments

a735dc5

jdneo reviewed Jan 11, 2021

View reviewed changes

Add chinese traslation

7e57119

0dinD and others added 2 commits January 11, 2021 16:26

Address review comment

2197367

Merge branch 'master' into fix-add-libraries-dialog

4cde9f8

jdneo approved these changes Jan 12, 2021

View reviewed changes

jdneo merged commit 21fcce9 into microsoft:master Jan 12, 2021

0dinD mentioned this pull request Jan 12, 2021

chore: prepare for 0.17.0 #441

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix add libraries dialog not working on Linux #434

Fix add libraries dialog not working on Linux #434

0dinD commented Jan 5, 2021

ghost commented Jan 5, 2021 •

edited by ghost

0dinD commented Jan 5, 2021

jdneo commented Jan 6, 2021

jdneo commented Jan 6, 2021

0dinD commented Jan 6, 2021 •

edited

jdneo commented Jan 7, 2021

0dinD commented Jan 7, 2021

jdneo commented Jan 7, 2021

0dinD commented Jan 7, 2021 •

edited

jdneo Jan 8, 2021

0dinD Jan 10, 2021

jdneo Jan 8, 2021

0dinD Jan 10, 2021

jdneo Jan 11, 2021 •

edited

0dinD Jan 11, 2021

0dinD Jan 11, 2021

jdneo commented Jan 11, 2021

jdneo left a comment

0dinD commented Jan 13, 2021

Fix add libraries dialog not working on Linux #434

Fix add libraries dialog not working on Linux #434

Conversation

0dinD commented Jan 5, 2021

ghost commented Jan 5, 2021 • edited by ghost

0dinD commented Jan 5, 2021

jdneo commented Jan 6, 2021

jdneo commented Jan 6, 2021

0dinD commented Jan 6, 2021 • edited

jdneo commented Jan 7, 2021

0dinD commented Jan 7, 2021

jdneo commented Jan 7, 2021

0dinD commented Jan 7, 2021 • edited

jdneo Jan 8, 2021

Choose a reason for hiding this comment

0dinD Jan 10, 2021

Choose a reason for hiding this comment

jdneo Jan 8, 2021

Choose a reason for hiding this comment

0dinD Jan 10, 2021

Choose a reason for hiding this comment

jdneo Jan 11, 2021 • edited

Choose a reason for hiding this comment

0dinD Jan 11, 2021

Choose a reason for hiding this comment

0dinD Jan 11, 2021

Choose a reason for hiding this comment

jdneo commented Jan 11, 2021

jdneo left a comment

Choose a reason for hiding this comment

0dinD commented Jan 13, 2021

ghost commented Jan 5, 2021 •

edited by ghost

0dinD commented Jan 6, 2021 •

edited

0dinD commented Jan 7, 2021 •

edited

jdneo Jan 11, 2021 •

edited