-
Notifications
You must be signed in to change notification settings - Fork 67
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
Fix add libraries dialog not working on Linux #434
Changes from 3 commits
30ab944
b2d3702
4665d8c
a735dc5
7e57119
2197367
4cde9f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT license. | ||
|
||
import * as fse from "fs-extra"; | ||
import * as _ from "lodash"; | ||
import * as minimatch from "minimatch"; | ||
import * as path from "path"; | ||
|
@@ -20,6 +19,7 @@ export class LibraryController implements Disposable { | |
public constructor(public readonly context: ExtensionContext) { | ||
this.disposable = Disposable.from( | ||
instrumentOperationAsVsCodeCommand(Commands.JAVA_PROJECT_ADD_LIBRARIES, () => this.addLibraries()), | ||
instrumentOperationAsVsCodeCommand(Commands.JAVA_PROJECT_ADD_LIBRARY_FOLDERS, () => this.addLibraries(true)), | ||
instrumentOperationAsVsCodeCommand(Commands.JAVA_PROJECT_REMOVE_LIBRARY, (node: DataNode) => | ||
node.uri && this.removeLibrary(Uri.parse(node.uri).fsPath)), | ||
instrumentOperationAsVsCodeCommand(Commands.JAVA_PROJECT_REFRESH_LIBRARIES, () => | ||
|
@@ -31,36 +31,34 @@ export class LibraryController implements Disposable { | |
this.disposable.dispose(); | ||
} | ||
|
||
public async addLibraries(libraryGlobs?: string[]) { | ||
if (!libraryGlobs) { | ||
libraryGlobs = []; | ||
const workspaceFolder: WorkspaceFolder | undefined = Utility.getDefaultWorkspaceFolder(); | ||
const isWindows = process.platform.indexOf("win") === 0; | ||
const results: Uri[] | undefined = await window.showOpenDialog({ | ||
defaultUri: workspaceFolder && workspaceFolder.uri, | ||
canSelectFiles: true, | ||
canSelectFolders: isWindows ? false : true, | ||
canSelectMany: true, | ||
openLabel: isWindows ? "Select jar files" : "Select jar files or directories", | ||
filters: { Library: ["jar"] }, | ||
}); | ||
if (!results) { | ||
return; | ||
} | ||
libraryGlobs = await Promise.all(results.map(async (uri: Uri) => { | ||
// 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 (await fse.stat(uri.fsPath)).isDirectory() ? `${uriPath}/**/*.jar` : uriPath; | ||
})); | ||
} | ||
|
||
public async addLibraryGlobs(libraryGlobs: string[]) { | ||
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) { | ||
const workspaceFolder: WorkspaceFolder | undefined = Utility.getDefaultWorkspaceFolder(); | ||
const results: Uri[] | undefined = await window.showOpenDialog({ | ||
defaultUri: workspaceFolder && workspaceFolder.uri, | ||
canSelectFiles: !folders, | ||
canSelectFolders: folders, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
canSelectMany: true, | ||
openLabel: folders ? "Select Library Folders" : "Select Jar Libraries", | ||
filters: folders ? { Folders: ["*"] } : { "Jar Files": ["jar"] }, | ||
}); | ||
if (!results) { | ||
return; | ||
} | ||
this.addLibraryGlobs(await Promise.all(results.map(async (uri: Uri) => { | ||
// 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 folders ? uriPath + "/**/*.jar" : uriPath; | ||
}))); | ||
} | ||
|
||
public async removeLibrary(removalFsPath: string) { | ||
const setting = Settings.referencedLibraries(); | ||
const removedPaths = _.remove(setting.include, (include) => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the param name to
canSelectFolders
to just align with the API?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, fixed in a735dc5.