Access active source control provider

[rebornix]

In our Pull Request multi root workspace scenario, we'd like to know which source control provider is active and show corresponding pull requests. We expect the experience to be similar to the git status bar items, which will update when the active source control provider changes.

We used to have this API but removed in a8b2945#diff-158dcbfd31a2238ddd6a89b8a4a23316 . Here I listen to the repository.onFocus event (learnt from scmActivity), we can probably move this to SCM service.

@joaomoreno feel free to write your own implementation if you think it's a valid request and my code needs a lot of change.

[joaomoreno]

@rebornix Currently the active concept is very flimsy and only based on focus... I always avoided doing this because there really should be no active SCM provider. If 3 are visible, 3 are active. Also, having the active concept depend on focus is a real accessibility nightmare since focus is used a lot for navigation.

If I get this right, you are now using the active concept as a master picker for deciding what to render in a single detail view?

[rebornix]

@joaomoreno maybe I should call it focused source control provider. We only want to render one Pull Request tree view based on the source control provider being focused/picked, similar to how the file changes and scm activity status bar are being displayed. Any idea how we can archive that?

[joaomoreno]

Related issue: #25838

Another big problem with this API is that it allows any extension to get a hold of SourceControl instances. Once they have them, they can change them and influence how they behave. IMO, only the extensions which create SourceControl instances should be allowed to change them.

I always thought we would get rid of the focused provider state, currently only used in the status bar, by using the editor focused state. The status bar would should the status of the provider which contains the file in the editor. There's even an issue about this (can't find it...), because the current behaviour only works if you go into the SCM viewlet and select the right provider. Most users just expected for it to switch as you switch editors. Would this be a suitable solution for you (tracking the editor focus)?

PS: We used to have this API in the preview state, it just never reached stable.

[rebornix]

IMO we are running into a similar issue when dealing with multiple providers in multi-root workspace, and the Source Control Providers list/section controlled by the core is a good solution to it.

With a central Source Control Providers list/section, extensions don't need to manage their own list view and users can use it to choose the information of which repository they want to see (e.g., Changes, Conflicts). Without such a list, every extension has to contribute a list view to the viewlet and it will look weird and confusing. The pull request scenario is similar, a pull request provider is strongly bound to a repository, that's why we'd like to know which repository they want to interactive with.

Most users just expected for it to switch as you switch editors

Do we have user feedback or data on this? I think it can be confusing when there are multiple visible editors from different workspaces. Now we even have grid layout, which supports more editors. If users open SCM viewlet and find that the Source Control Provider is not the one they want, they need to find a relevant one in the Editors pane. Wouldn't this be more complex?

cc @kieferrm @RMacfarlane

[joaomoreno]

If users open SCM viewlet and find that the Source Control Provider is not the one they want, they need to find a relevant one in the Editors pane.

Good point.

I still think that there should only be one PR view. I just don't think the current active notion makes sense. Note that we can also have multiple SCM providers visible. And the active notion simply matches DOM focus on them:

And I get complaints because it is not clear as to when the statusbar shows info from repo A or B. My thoughts on improving the SCM status bar contribution are to create one which aggregates information from all visible providers, since visibility is indeed what users have total and explicit control on.

I'd be much more willing to expose provider visibility on the API, than a flimsy active concept. I wonder if we can build up on that and also have your view show two main branches in its tree, whenever two repositories are visible?

[RMacfarlane]

The idea of a visibleSourceControls API makes sense to me for the statusbar, but I think the current notion of "active" is also valuable.

I would argue that the notion of "active" is already well understood within the SCM viewlet because the file changes panel there updates on provider change. Having this single panel reduces clutter, and since it's so close to the list of providers, the relationship between the two is clear. Exposing this to extensions would allow for a more consistent UI experience in the SCM viewlet.

A related request: gitkraken/vscode-gitlens#224

[joaomoreno]

I would argue that the notion of "active" is already well understood within the SCM viewlet because the file changes panel there updates on provider change.

Note that you can have multiple file changes panels. Hold Cmd while clicking on another repo and you'll get two file change panels. Once you're there, the active provider concept becomes very loose, since it is centered around which file change panel was last focused. Which seems to already confuse plenty of users which expect the active repository to also switch when switching files in the editor itself.

[RMacfarlane]

Aha! I had never multiselected in that list before. Ok, I see why the concept is flimsy.

If we introduce a concept of visibleSourceControls, would visibility equate to selection within the list?

[rebornix]

I'd be much more willing to expose provider visibility on the API

@joaomoreno I like this idea. When I look at the two Changes sections in the viewlet, I don't have any confusion. Visibility is a better concept than Active*. My question is the same as @RMacfarlane about whether the visibility equals to the selections in provider list. If so, we can have the same experience when users only select one provider (then a single Changes section is visible), and show two root nodes when users cmd click multiple providers.

[joaomoreno]

Yeah, visibility === list selection. Cool, I agree and think it's our best option. Let's get that going in July?

[eamodio]

@joaomoreno WOAH -- I never tried that (re: #51734 (comment)). I want that all the time -- I personally would love to be able to get rid of the master/detail and just have the multi-select be the default. It would be even better if the branch/sync info could be added onto the view's "title".

As for active/visible this is a challenge we all are facing -- in GitLens I try to bridge the gap, by showing a repository "node" that tracks the active file, but also still show all the available repos (repos is basically the same as the source control providers). I've also thought about syncing with the selected SCM view (if that became possible), but now with multi-select that falls apart.

Also with mutli-select the status bar seems "broken" -- maybe that should track the active file as well?

Visible === selection feels strange to me -- why not just provide the list (all available), and selection change events (& selected property) as other custom views?

[joaomoreno]

Also with mutli-select the status bar seems "broken" -- maybe that should track the active file as well?

There's an open issue for that... I'm too lazy to search for it.

Visible === selection feels strange to me -- why not just provide the list (all available), and selection change events (& selected property) as other custom views?

What do you mean?

[eamodio]

re: Visible === selection

Mainly because they are all "visible", but one or more are selected. And I think it would be good to get access to all the SCM providers (selected or not), with the ability to tell if one is selected (and listen for selection change events)

[joaomoreno]

I think it would be good to get access to all the SCM providers (selected or not), with the ability to tell if one is selected (and listen for selection change events)

I like that. cc @RMacfarlane

[RMacfarlane]

So this would be something like:

	export namespace scm {

		export interface SourceControl {
			
			...

			/**
			 * Whether or not the source control is selected.
			 */
			readonly isSelected: boolean;
		}

		/**
		 * All registered [source controls](#SourceControl).
		 */
		export const sourceControls: Readonly<SourceControl>[];

		/**
		 * An [event](#Event) which fires when the selected [source controls](#ReadonlySourceControl) have changed.
		 */
		export const onDidChangeSelectedSourceControls: Event<Readonly<SourceControl>[]>;
	}

To me, the selection event seems a little ambiguous in this case - I could imagine the event data being either all source controls that have isSelected: true or all source controls that have had their value of of isSelected changed.

My preference would be for separate sourceControls and selectedSourceControls arrays without a flag on the source controls themselves. I think this makes the selection event data clearer and is more consistent with other APIs, for example the proposed QuickPick: https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.proposed.d.ts#L785

@eamodio @joaomoreno What do you think?

[eamodio]

How about something like this:

export namespace scm {
	export interface RepositoryView {
		readonly label: string;
		readonly rootUri: Uri | undefined;
		readonly inputBox: SourceControlInputBox; // Maybe?
	}

	/**
	 * An event describing a change to the set of known repositories.
	 */
	export interface RepositoriesChangeEvent {
		/**
		 * Added repositories.
		 */
		readonly added: RepositoryView[];

		/**
		 * Removed repositories.
		 */
		readonly removed: RepositoryView[];
	}

	export interface RepositorySelectionChangeEvent {

		/**
		 * Selected Repositories.
		 */
		readonly selection: RepositoryView[];

	}

	export interface SourceControl { // Would probably need to use another name here, since currently each repo is called a `SourceControl`
		/**
		 * All currently known repositories
		 */
		readonly repositories: RepositoryView[];

		/**
		 * Currently selected repositories.
		 */
		readonly selection: RepositoryView[];

		/**
		 * An [event](#Event) which fires when a repository is added or removed.
		 */
		readonly onDidChangeRepositories: Event<RepositoriesChangeEvent>;

		/**
		 * An [event](#Event) which fires when the known set of repositories have changed.
		 */
		readonly onDidChangeRepositorySelection: Event<RepositorySelectionChangeEvent>;
	}
}

There are certainly naming issues above, because of the current use of SourceControl (which feels like a strange name for what is effectively a repository afaik). But this is in line with the WorkspaceFolders structure for existence, and in line with the TreeView (and kind of TextDocument) for selection.

Also to be consistent (and with what you said @RMacfarlane) I dropped the isSelected flag

[joaomoreno]

@eamodio Unfortunately that would be breaking API, which we're aiming not to do. Repository is also not a generic enough name, since it means different things in SVN and TFS.

I actually like @RMacfarlane's suggestion, though I would make the event fire the collection of SourceControls which are selected.

A readonly isSelected property looks OK to me.