Fix #4167: use java.projects context for explorer actions#4450
Fix #4167: use java.projects context for explorer actions#4450MeherSru wants to merge 3 commits into
Conversation
|
This change introduces the java.projects array context and uses it to make the Add/Remove Source Path Explorer actions more precise. These actions are now only shown for Java project folders instead of every folder in the Explorer. |
There was a problem hiding this comment.
Thanks for tackling #4167! The array-context-key approach is the right tool here, but I think it's wired to the wrong set for these two actions. Left a few inline notes.
please also consider adding PR's description.
|
@wenytang-ms Thanks for the detailed feedback! I've updated the PR to address the comments. I reverted the Explorer menu changes, moved the java.projects update out of the javaLSReady path, and added a helper to refresh the context when projects are imported or deleted. |
datho7561
left a comment
There was a problem hiding this comment.
The context is set up properly, but it seems like it's not actually used to adjust access to context menu items. The when clauses for adding and removing source folders will need to be adjusted. See https://code.visualstudio.com/api/references/when-clause-contexts . Take a look at package.json line 1917, where the when clause is configured
|
Ahh just noticed that changes to the package.json were removed after @wenytang-ms 's review. I think there's a way to configure it properly with how you've set it up |
b0610b5 to
59ccfb4
Compare
datho7561
left a comment
There was a problem hiding this comment.
I think this is a good idea. The criteria for when it's shown isn't perfect (for example, this prevents you from adding nested folders to the source path), but I think it's still helpful. (In Eclipse projects that don't use Maven or Gradle, the source folders are usually located at the top level of the project, eg. ./src/)
I'm going to figure out why the tests are failing before merging this.
|
The test failure is an existing, consistent test failure that I've documented here: #4458. I think we're good to merge, unless you want to take another look @wenytang-ms ? |
No description provided.