-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
Backend part of working sets #87666
Backend part of working sets #87666
Conversation
@kieferrm What do you think in general about this approach? I see that you have this item on the roadmap. Did you have any discussions about it? Can you delegate this parts of it to external contributors? What about other items? |
Thank you very much for this PR. However I would first like to understand how this would look in the UI, before we actually look at any implementations. So I would first bring this up in our weekly UX meeting, and after that we can continue the discussion. I will also see how Eclipse did this, please let me know if there are other solutions which you think are good. fyi @bpasero |
@isidorn The approach I have taken is inspired by Nuclide working sets. You can take a look at how they look like here: https://nuclide.io/docs/features/working-sets/ There're two ways to implement them:
|
@isidorn Concerning the roadmap: https://github.com/microsoft/vscode/wiki/Roadmap
|
@solomatov Thanks again for this PR and thank you for the Nuclide working sets pointer.
Changes needed on the vscode Explorer side:
Also the extension would depending on the active working set make sure that the value of this new setting is correct. Some other related feature requests #41274 #10962 #41860 Even though #9498 is similar to this, it is not exactly the same and that one could also be done by extensions if we tackled this #15178 @solomatov let me know what you think and we can discuss in more details if needed. |
Some additional insights after discussing with @bpasero
As @bpasero suggest we could introduce a new service that would be aware of what files are visible and what not - respecting the So that seems like a great first step for this. Introducing the setting |
To add on top: currently
I think I would expect working sets to have an influence in the same areas as well. Thus I was suggesting a new service that anyone can ask if a resource should be included or not. Having both |
@bpasero @isidorn thanks for all the pointers!
One open question: ideally, we want want file search to cover the whole workspace, not just the included files. Would that service be used for that? what do you think about this being the default behavior for VSCode? |
@isidorn
cc @OmarTawfik |
I think going by
Yeah we may have go decide this on a case by case basis. One option would be to still show the search results from outside the working set in a different bucket, collapsed by default. |
@OmarTawfik @solomatov great recap, that was exactly our thinking.
Thanks a lot and let me know if you have more questions. |
@bpasero @isidorn thanks for all the suggestions! it certainly makes things more straightforward, but would present the following issues with using
Any thoughts on that? |
I think we would need to support a way to configure this setting for the workspace from outside the workspace. Since we do not allow the same workspace to be opened in multiple windows, a user could make changes to both windows without impacting each other.
That is probably OK as long as the extension is storing the working set definition somewhere. E.g. couldn't the extension store an array of all working sets and switching between them is simply replacing the |
Re PR to tackle #40233, I am happy to accept it. Since it touches API and UX this needs to be discussed in respective calls which can be worked out together. |
@OmarTawfik regarding point 2) just to add on top of what @bpasero said. The working set extension should also remember the initial value of the |
@isidorn I have concerns about storing files.include in the settings.json: One solution to a) is to have #40233. Another one which I like the most is exposing in-memory settings. If we will workspace folder specific in memory settings, it will solve the problem, i.e. we will just load the settings from there. There's a usability problem of what to do while the extension is being activated, but we can cache the old value in the local storage or in other temporary storage available to the VS Code. What do you think? cc @OmarTawfik |
@sandy081 So what are our next steps for this? I think, the simplest way to add these settings is to set an attribute on a setting scheme whether it's user specific or project specific. In this way, we can keep the same UX for settings as we have now. cc @OmarTawfik |
@solomatov I agree that storing it in |
Just for reference as an example: an Explorer Exclude extension found by @egamma |
I think the in-memory solution mentioned here would work well for our case. Without it:
cc @sandy081 I want to take a stab at this, maybe exposing |
@sandy081 I'll follow up on #43226, since it aligns to our goals! thanks for the pointer. |
@OmarTawfik after discussion with @sandy081 please note that #43226 is not requested by that many users and that it has more open questions than #40233 Thanks for offering to do a PR however to avoid designing a solution that will not get merged I would advise that you wait some time until we internally decide on the best way to tackle this. As for your concerns for #40233:
Also I see @bpasero has already answered these concernes here |
@isidorn thanks for taking a look. Problem with #40233
Yes, but that would still corrupt the view of the world. If the user has chosen to display (for example) two working sets, each covering two folders (total of 4), and then they go and modify that setting locally to remove one (now only three remain). What would the extension do in that case?
Not the same workspace, but two folders in the same repo. The main use case is a large repo with hundreds of projects. Since settings would be updated for both when one is updated, #40233 would not work for us here. Current progress with this issueI almost implemented the For example, consider structure:
And with the setting: {
"files.include": {
"src/**": true,
}
} In the following filter, if we get vscode/src/vs/workbench/contrib/files/browser/views/explorerViewer.ts Lines 507 to 521 in 980d8ef
So, returning
Thinking about it, it would be expensive to enumerate every child recursively to see if anything matches or not. One possible solution to this is to not use globs, but use explicit folder names only, like
Any thoughts on this? |
@OmarTawfik Are you assuming that in memory settings are not editable by user? I do not see the reason to restrict this to user from settings perspective. Also how are you expecting user see these in memory settings values? |
One idea that just crossed my mind: could we leverage VSCode workspace files as the thing to store different working sets into? Consider this scenario:
|
@bpasero It might work, but I see the following problems:
|
How would you solve that with your approach? Specifically how could you have working set A and B at the same time?
We have #35109 to switch workspaces without window reload. However, editors would change as a result of that operation. This should maybe be a user setting because I can see that I want the editors from my one working set to restore once I change to it. That would give me a nice task-oriented model where I can switch between my sets while restoring all associated editors automatically.
This model would still allow you to put settings into the folder of that workspace that would then be shared across all workspaces that have this folder in it. This is limited to resource settings that can be defined on the level of a folder, but for example all editor settings are resource settings. |
We will do that by "combining" the paths provided in
Working sets are private in nature. They are not meant to be checked-in/shared between teams. We want to avoid UX confusion by not having it persisted to files on disk that users see/interact with.
Any thoughts on this? |
'working-sets'?: { | ||
'working-sets': { | ||
[key: string]: string[] | ||
} | ||
}; |
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.
Hey! I'm kinda new to TypeScript, just poking around, trying to learn some stuff 😄 . I've never seen this type of Interface before. Why don't you define IWorkingSetsConfiguration
like this instead:
'working-sets'?:{ [key:string]: string[] }
Never seen this pattern before, just curious 😄
I am not sure I would agree. Why do we artificially limit this feature to not be shareable? I think if a repo owner decides that certain |
@OmarTawfik can you create a new PR for the |
@bpasero IMO, that is what
@isidorn that's the plan. I just want to agree on the implementation before I start sending PRs. Based on comments on this thread and #43226, it looks like the recommended direction by the VSCode team is:
There are a couple of issues with that plan:
If we can resolve these blockers, I think it is a reasonable way forward, and I can write a full proposal/start sending PRs in order. |
@OmarTawfik regarding the issues with
We would introduce As for more details regarding #40233 I suggest @sandy081 and you discuss it, and I know he will have more time next milestone (in 2 weeks). And yes it makes perfect sense to first discuss through these issues and only then start on the PRs. Thanks a lot! |
Alright. Let's discuss more when you get some time in the next milestone :) Thanks for following up. |
Sorry for not commenting here sooner, I was busy with other feature areas - mostly accessibility. I will definetely have time for reviewing and testing So let us know what you think would be the best next steps for you. Thank you |
I see that you have a support for working sets in the roadmap on the next year.
This proposal creates a "backend" part for the working sets:
Design decisions:
Next steps:
Please, let me know what you think about this.