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

Backend part of working sets #87666

Closed
wants to merge 1 commit into from
Closed

Backend part of working sets #87666

wants to merge 1 commit into from

Conversation

solomatov
Copy link
Contributor

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:

  • It creates workingSets.json file in .vscode folders in roots of mounted folder
  • It allows defining included folders in these files
  • It allows to switch active working set using a setting in workspace

Design decisions:

  • The working sets have overlapping scopes, i.e. you can activate the working set with the same name, and it will work in several mounted folders. This is done by design, since often, working sets span several repositories.
  • I didn't use anything complicated as include patterns, since I want to have a simple UI, ideally, checkbox tree to choose what's available.

Next steps:

  • If it gets accepted, I will create a UI for it. It can be implemented as an extension, or as a part of the main process (which is preferable, since there're more UI features there).

Please, let me know what you think about this.

@solomatov
Copy link
Contributor Author

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

@isidorn
Copy link
Contributor

isidorn commented Dec 27, 2019

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.
As far as I see this is the only issue that we have open which resembles working sets #9498
Please let me know if you are aware of other feature requests which would be solved with this.

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.
Hope that makes sense.

fyi @bpasero

@isidorn isidorn added this to the January 2020 milestone Dec 27, 2019
@solomatov
Copy link
Contributor Author

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

  • In vscode core. The good thing is that it will be much easier to have a checkbox tree.
  • In extension. There're no checkbox trees, but it's possible to more or less emulate one with change in the checkbox icon.

@solomatov
Copy link
Contributor Author

@isidorn Concerning the roadmap:

https://github.com/microsoft/vscode/wiki/Roadmap

Improve VS Code performance in large workspaces
- Investigate 'working sets' of files and folders in the file explorer
- Provide guidance to users how to configure VS Code
- Improve Open File / Quick Pick performance

@isidorn
Copy link
Contributor

isidorn commented Jan 9, 2020

@solomatov Thanks again for this PR and thank you for the Nuclide working sets pointer.
I have presented this in our weekly UX meeting and we concluded this is best done by an extension by:

  • Contributing a new Working Sets view to the explorer. This Working Sets view would have all the functionality for managing Working Sets - renaming, seting which one is active, delating etc..
  • Contributing a "Add to Working Set" action in the Explorer - which can be multi selection aware so it is easier for the users to create Working Sets from the Explorer

Changes needed on the vscode Explorer side:

  • Explorer would need to have a new configuration from which files in the explorer would get filtered. Something like a duplication of files.exclude. This is what you have already done in this PR. However I would prefer if Explorer has no notion of Working Sets, so I would call this setting differenetly
  • Explorer needs to show opened editor even if it is hidden via some setting. This makes sense overall and we can tackle it as a seperate PR

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.

@isidorn
Copy link
Contributor

isidorn commented Jan 9, 2020

Some additional insights after discussing with @bpasero

  1. This new setting should not be exclusive but inclusive (specifying what is shown). Similar request Exclude all files except for... #869 So it could be called files.includesas this comment points out
  2. The downside of the extension always writing the setting based on the active set - the setting would always get dirty. We can think about this problem a bit later - idealy we would need to have workspace settings outside of a workspace [Feature] Local Workspace settings #40233 This we plan to tackle in the future, however it is not on the current plan as @sandy081 points out

As @bpasero suggest we could introduce a new service that would be aware of what files are visible and what not - respecting the files.include and files.exclude and the open editor.

So that seems like a great first step for this. Introducing the setting files.include and introducing the service for this. After we have this we can think more about how the extension would change this setting dynamically based on the active working set.

@bpasero
Copy link
Member

bpasero commented Jan 9, 2020

To add on top: currently files.exclude is being used and respected in these locations:

  • Explorer
  • Search results
  • Breadcrumbs picker
  • Quick open
  • Problems

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 files.exclude and files.include will result in potential conflicts if both patterns match. We probably need to define some rules which pattern wins (e.g. by looking at the pattern length).

@OmarTawfik
Copy link

OmarTawfik commented Jan 9, 2020

@bpasero @isidorn thanks for all the pointers!
I'm working with @solomatov on this area. Please let me know if the below covers your comments:

  1. Adding the new service to see what is included or not.
  2. Make it respect/read files.include and files.exclude.
  3. Replace all files.exclude usage with this new service.
  4. Decide what happens in case of conflicts (both patterns match), who would win?
    • My thoughts here, people use include to define what they want to see in the file tree, and exclude to exclude patterns that don't matter here (within that tree), so our logic should follow the same (includes.match(path) && !excludes.match(path)). Please let me know if you think otherwise.
  5. Have the setting be updated by an external "working sets" extension. The extension would keep its knowledge of sets private, along with functionality to update/delete/create them.
    • for now, the plan is to use vscode.WorkspaceConfiguration.update(_, _, true) to update that setting (globally).

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?

@solomatov
Copy link
Contributor Author

@isidorn
So, to confirm some parts of the plan:

  • Do not create a working-set.json file, and use files.include, and files.exclude instead via a dedicated service
  • The extension will read its settings, generate derived settings with includes and excludes, and use these settings to guide the service, controlling what's visible and what's not. Do we need a new API for these or can we store it just in some of the configuration files? I have seen some proposal to expose internal memory only settings, will we need this to do this work? Will you be open to a PR to expose them in the API?

@bpasero

  • Concerning search results, it's a bit tricky. User might want to find references everywhere, but show the ones from the working set, and the rest in a different way. I agree that in the rest of the items, files should be excluded.

cc @OmarTawfik

@bpasero
Copy link
Member

bpasero commented Jan 10, 2020

Decide what happens in case of conflicts (both patterns match), who would win?

I think going by (includes.match(path) && !excludes.match(path)) is probably a good start and we can see if there are any issues.

Concerning search results, it's a bit tricky.

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.

@isidorn
Copy link
Contributor

isidorn commented Jan 10, 2020

@OmarTawfik @solomatov great recap, that was exactly our thinking.

  1. Search already has a checkbox if it should respect the files.exclude setting (picture attached). That way a user can easily switch if he wants to search the whole workspace or not. @bpasero showing results collapsed would still make some searches very slow on large workspaces. By default this checkbox is turned on, which I think makes sense to avoid long running searches.
  2. Changing the logic of how include and exclude align should be easy once we have the service. (includes.match(path) && !excludes.match(path)) is the only thing that makes sense to me atm
  3. @solomatov as @OmarTawfik pointed out you can use vscode.WorkspaceConfiguration.update to update the setting. The downside is that you will always make the settings file dirty. As for the PR to tackle [Feature] Local Workspace settings #40233 @sandy081 would be best to answer if he would be willing to accept a PR

Thanks a lot and let me know if you have more questions.

Screenshot 2020-01-10 at 12 31 05

@OmarTawfik
Copy link

@bpasero @isidorn thanks for all the suggestions! it certainly makes things more straightforward, but would present the following issues with using files.include:

  • different windows to the same workspace: in that case, they would all read/edit the same setting, thus updating in one window would override the other.
  • user is allowed to edit that setting: in that case, this will invalidate their active working sets, without having the mental connection between both, which will be a negative UX.

Any thoughts on that?

@bpasero
Copy link
Member

bpasero commented Jan 13, 2020

different windows to the same workspace

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.

user is allowed to edit that setting

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 files.include value? If the user makes changes to files.include it would not impact those working set definitions?

@sandy081
Copy link
Member

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.

@isidorn
Copy link
Contributor

isidorn commented Jan 13, 2020

@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 files.include so when there is no longer a working set active the intial files.include value is restored (which the user manualy set). If the user manually changed the files.include while a working set is active yes that would lead to incosistent UI, but I think we can live with that as that sounds like a corner case imho. A working set extension could even notify the user to not change the files.include while a working set is active if it detects changes on that setting.

@solomatov
Copy link
Contributor Author

@isidorn I have concerns about storing files.include in the settings.json:
a) It's often committed and we don't want it to be put into settings.json
b) A user can change it, and close settings.json. In this case, it will be hard to bring it back, and that's what we want to avoid

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

@solomatov
Copy link
Contributor Author

@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

@isidorn
Copy link
Contributor

isidorn commented Jan 13, 2020

@solomatov I agree that storing it in settings.json is not a good long term solution. However we can still start with other things and temporarliy put it in storage.json before we look into this issue.
As for #40233 we do want this in VSCode and should be doable.
As for next steps, @sandy081 would be best to answer this, however I do know that this milestone he is focued on other tasks and he planned to add that item to his plan next milestone. And yes having another ConfigurationTarget is something we also thought makes sense.

@isidorn
Copy link
Contributor

isidorn commented Jan 13, 2020

Just for reference as an example: an Explorer Exclude extension found by @egamma
There is a bit of common ground between this extension and what the Working Sets extension would do
https://marketplace.visualstudio.com/items?itemName=RedVanWorkshop.explorer-exclude-vscode-extension

@OmarTawfik
Copy link

I think the in-memory solution mentioned here would work well for our case. Without it:

  • User changes to that setting would corrupt our view of the world.
  • This would corrupt the case where the user has multiple windows, where changing the active working set would update the other window.

There is an internal support for storing in memory configuration. But this is not supported in the API. But your scenario is a good use case to expose this.

cc @sandy081 I want to take a stab at this, maybe exposing ConfigurationTarget.InMemory? would you be open at accepting a PR? any pointers would be appreciated.

@sandy081
Copy link
Member

Next steps are to discuss the design and implementation for #40233 or #43226. I would suggest to discuss them in the corresponding issue and open a PR. I cannot assure that I can look at this in this milestone as we have other plans already. But we can discuss the design and implementation.

@OmarTawfik
Copy link

@sandy081 I'll follow up on #43226, since it aligns to our goals! thanks for the pointer.
The current milestone is not an issue, since I can work on my fork, and we can review/make sure it aligns with your project plans, and merge it whenever needed.
What I want to avoid is to design a solution that is not going to get merged into master at some point, and would require us to revert/conflict with any future updates.

@isidorn
Copy link
Contributor

isidorn commented Jan 14, 2020

@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
We always try to find the solution that would work for multiple scenarios and to me it seems like #40233 is the way to go here since it is more general.

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:

  • User changing would corrupt your view: your extension can react on configuration changes
  • VS Code does not support opening multiple windows on the same workspace, or did I misunderstand something here? Can you explain this multi window scenario better? Thanks

Also I see @bpasero has already answered these concernes here

@OmarTawfik
Copy link

@isidorn thanks for taking a look.

Problem with #40233

User changing would corrupt your view: your extension can react on configuration changes

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?
The best it can do currently is to just warn the user along the lines of (settings manually edited, overwrite needed) and then error out. With #43226, this problem goes away

VS Code does not support opening multiple windows on the same workspace, or did I misunderstand something here? Can you explain this multi window scenario better? Thanks

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 issue

I almost implemented the files.include setting, but ran into a blocker (possibly related to #85193). When checking the globs in that setting in the explorer to see if we should view it or not, we cannot use TreeVisibility.Recurse to check children of a folder.

For example, consider structure:

File1.txt
src/
   File2.txt

And with the setting:

{
  "files.include": {
    "src/**": true,
  }
}

In the following filter, if we get src folder, we cannot determine if we should show it or not based on the glob alone, since it might contain files that should get matched:

filter(stat: ExplorerItem, parentVisibility: TreeVisibility): TreeFilterResult<FuzzyScore> {
if (parentVisibility === TreeVisibility.Hidden) {
return false;
}
if (this.explorerService.getEditableData(stat) || stat.isRoot) {
return true; // always visible
}
// Hide those that match Hidden Patterns
const cached = this.hiddenExpressionPerRoot.get(stat.root.resource.toString());
if (cached && cached.parsed(path.relative(stat.root.resource.path, stat.resource.path), stat.name, name => !!(stat.parent && stat.parent.getChild(name)))) {
return false; // hidden through pattern
}
return true;

So, returning TreeVisibility.Recurse is not supported for async trees:

throw new Error('Recursive tree visibility not supported in async data compressed trees');

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 folderA, folderA/folderB, or folderA/folderB/file1.txt, but no *.

  • PROs: let us perform this efficiently, but just splitting paths and checking explicitly for names. No glob parsing/regex execution would be necessary.
  • CONs: this deviates from files.exclude and might cause minor user confusion.

Any thoughts on this?

@sandy081
Copy link
Member

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

@OmarTawfik
Copy link

@sandy081 Great question! Let's discuss that on #43226? I'll aggregate questions/answers there.

Do you have any thoughts on the issue presented above (using globs vs paths to expand the tree)?

@bpasero
Copy link
Member

bpasero commented Jan 15, 2020

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:

  • you have a repository that is quite large and that would benefit from working sets
  • instead of worrying about where to put the files.include settings for each working set, a *.code-workspace file is created for each working set
  • each workspace file configures files.include differently based on needs
  • switching working sets means switching workspaces
  • you can have multiple working sets opened in multiple windows easily (each window resembles one workspace)
  • changes to the working sets do not make the workspace dirty because the changes go into the workspace file directly
  • a user can still optionally decide to check these workspace files into the repository if the user thinks they are useful for others

@solomatov
Copy link
Contributor Author

@bpasero It might work, but I see the following problems:

  • People might have many working sets, and they are not exclusive from each other. So for example, you have two projects, A and B, and have working sets for each of them. In some cases you want A, in some cases B, and in some cases A and B.
  • Switching working sets is pretty heavyweight. It would be nice to be really fast, like just one click away, without loosing context of open editors, settings, etc.
  • You might want to share settings between working sets, and I am not aware of any solution for it, except global settings.

@bpasero
Copy link
Member

bpasero commented Jan 15, 2020

People might have many working sets, and they are not exclusive from each other. So for example, you have two projects, A and B, and have working sets for each of them. In some cases you want A, in some cases B, and in some cases A and B.

How would you solve that with your approach? Specifically how could you have working set A and B at the same time?

Switching working sets is pretty heavyweight. It would be nice to be really fast, like just one click away, without loosing context of open editors, settings, etc.

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.

You might want to share settings between working sets, and I am not aware of any solution for it, except global settings.

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.

@isidorn isidorn modified the milestones: January 2020, On Deck Jan 16, 2020
@OmarTawfik
Copy link

OmarTawfik commented Jan 16, 2020

How would you solve that with your approach? Specifically how could you have working set A and B at the same time?

We will do that by "combining" the paths provided in files.include from all active working sets.

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.

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.

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 folderA, folderA/folderB, or folderA/folderB/file1.txt, but no *.

Any thoughts on this?

Comment on lines +525 to +529
'working-sets'?: {
'working-sets': {
[key: string]: string[]
}
};

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 😄

@bpasero
Copy link
Member

bpasero commented Jan 17, 2020

Working sets are private in nature. They are not meant to be checked-in/shared between teams.

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 files.include patterns help navigate the repository, why would we prevent that from being shared?

@isidorn
Copy link
Contributor

isidorn commented Jan 17, 2020

@OmarTawfik can you create a new PR for the files.include and ping me on it.
The discussion here is mostly about the general design of the Working Sets feature, so I think it would be easier to follow if we split up the discussion. The other one would be more about the tree filter and the actual files.include implementation. Thanks!

@OmarTawfik
Copy link

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 files.include patterns help navigate the repository, why would we prevent that from being shared?

@bpasero IMO, that is what .code-workspace files are for. If it is shareable, then it should be in a workspace that defines its own folders. working sets (as described here, and as it is used in Nuclide) are not shareable. They are an abstraction on top of the active workspace. These working sets will be saved to disk (in a user-specific file) but not anywhere near the project/repo.

The discussion here is mostly about the general design of the Working Sets feature, so I think it would be easier to follow if we split up the discussion. The other one would be more about the tree filter and the actual files.include implementation. Thanks!

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

  1. Implement files.include as a global setting, and have the tree present files that match files.include.matches(path) && !files.exclude.matches(path).
  2. Implement the ability to save settings local to that workspace ([Feature] Local Workspace settings #40233?) but not persisted to disk
  3. Implement working sets as an extension that updates that setting inside the current active workspace, and presents UI for the user to edit their working sets from.

There are a couple of issues with that plan:

  • files.include must be full paths, and cannot support globs * like files.exclude, as we need to figure out if we are expanding parents without looking at their children (more details).
  • How would [Feature] Local Workspace settings #40233 behave technically? how can the extension update a setting, but make that change only available to the active window? cc @sandy081

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.

@isidorn
Copy link
Contributor

isidorn commented Jan 20, 2020

@OmarTawfik regarding the issues with files.include. Please note that we can not eagirly resolve folders just to double check if they should be visible based on if their children match a glob pattern. I would go with the simple approach: if a folder might have a child that matches an include glob pattern, show the folder.
Cornercase example: files.include is a/b/c.txt. The file c.txt in question does not exist, we would still show a and b. I would find it strange that folders disapear once you expend them, and we can not infintly eagirly resolve folders due to performance reasons. Does this make sense?

files.include as a global setting

We would introduce files.include as a regular setting, not sure if you meant something special by global.

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!

@sandy081
Copy link
Member

How would #40233 behave technically? how can the extension update a setting, but make that change only available to the active window? cc @sandy081

Local/user folder settings are scoped only to the folder. It means these are also applied when you open that folder in a different workspace.

@OmarTawfik
Copy link

Alright. Let's discuss more when you get some time in the next milestone :) Thanks for following up.

@alshdavid
Copy link

Great idea, will be following this closely. I have made an image of what I think working sets could look like when implemented:

image

Working sets could be accessible via an icon on the activity bar

@isidorn
Copy link
Contributor

isidorn commented Feb 19, 2020

Sorry for not commenting here sooner, I was busy with other feature areas - mostly accessibility.

I will definetely have time for reviewing and testing files.include PR in the March / April milestone.
And for Local Workspace Settings I just discussed with @sandy081 and he has this feature on the plan for the next couple of milestones.

So let us know what you think would be the best next steps for you.

Thank you
isidor

@solomatov solomatov closed this Jan 6, 2021
@solomatov solomatov deleted the working-sets branch January 6, 2021 21:10
@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants