Create container views to support source control other than Git #3438
Conversation
westbury
force-pushed
the
GH-3423-part-1
branch
from
9fe7515 to
4606c4c
Compare
westbury
force-pushed
the
GH-3423-part-1
branch
2 times, most recently
from
4596efc to
ee06881
Compare
westbury
force-pushed
the
GH-3423-part-1
branch
from
ee06881 to
0324577
Compare
There was a problem hiding this comment.
I like the idea, nice changes overall :)
Note that we usually never use multiInject, not a bad thing, just a bit anti-pattern when it comes to Theia (we mostly use named ContributionProviders). @akosyakov thoughts?
Also I don't know how other project members like this feature.
| * | ||
| * @param fileStat | ||
| */ | ||
| isUnderSourceControl(fileStat: FileStat): boolean; |
There was a problem hiding this comment.
I think this needs to be Promise<boolean> or at least MaybePromise.
There was a problem hiding this comment.
Currently isUnderSourceControl is synchronous. Maybe it would be better to make it asynchronous as that would give more flexibility to other SCM implementations. It would also remove the non-intuitive requirement that the children are populated in the FileStat.
| /** | ||
| * Resolves to an array of repositories discovered in the workspace given with the workspace root URI. | ||
| */ | ||
| repositories(workspaceRootUri: string, options: Git.Options.Repositories): Promise<Repository[]>; |
There was a problem hiding this comment.
I understood this is intended to abstract over different scms, but it uses git specific API.
There was a problem hiding this comment.
I am really not sure if abstracting over SCMs is a good idea. I can see that mercurial is similar to Git but others are often not.
SVN and other non-distributed SCMs will have more differences than Mercurial, but I still see most of the components in the Git views being reusable for other SCMs. I really don't see why it would not be a good idea to allow SCM implementations to re-use the components that apply and override the components that differ. VS Code provides five extension points to be used by SCM implementations and I assume we would do the same.
It's good to have the top level detection of what widget to show as the SCM widget, but that doesn't need to share concepts like repository or so.
You are right that Repository is in the Git namespace and is used for all SCMs. This certainly could be cleaned up. However Repository only has a single property 'localUri'. All SCMs must result in local copy of the code and this is the URI of that local copy, so 'localUri' will apply to all SCMs. Perhaps the Repository class was expected to expand so that it includes Git-specific properties. (I had considered adding the SCM type to this class.) I could share at the top level just the local URI, or I could create a base Repository class which can be used as is or extended by SCM providers.
Also what is the use case of having multiple scm containers in one view?
You are right that we could provide a separate view for each SCM. However that would result in multiple tabs, multiple views to be shown or hidden in the 'view' menu. There would also be multiple components in the status bar, one to select from all the Git repositories, another to select from all the Mercurial repositories. This would rapidly fill up the status bar. Surely it is a better user experience if there is a single SCM view, and a single list from which the user can select a repository. The single view wil, of course, react to the contents. So, for example, if the repository is Mercurial then there will be no 'staged' section. If the repository is SVN then there will be further differences. Maybe I am missing something but that is the expectation of our users. VS Code has a single view for all SCMs.
|
|
||
| export const GIT_WIDGET_FACTORY_ID = 'git'; | ||
|
|
||
| export interface ScmWidgetFactory { |
There was a problem hiding this comment.
The name suggest it creates ScmWidget instances. But I don't see a method that would do that.
There was a problem hiding this comment.
Also what is the use case of having multiple scm containers in one view?
IIUC, in this case it would reuse the same view, but the actual widget inside it would be VCS dependent. And I also seem to understand that one assumption made in this change is that their will be only one VCS type/kind per folder? As in, a folder versioned using git won't use mercurial at the same time?
Yes, correct. Only one ScmWidget visible at a time, they are stacked. It would be possible, I suppose, for two SCM implementions to claim they support a repository in the same directory. If this happens the first one will be used.
| } | ||
| } | ||
| } | ||
| return 'no-scm'; |
There was a problem hiding this comment.
should be NO_SCM_WIDGET_FACTORY_ID
|
I am really not sure if abstracting over SCMs is a good idea. I can see that mercurial is similar to Git but others are often not. Finally, please make sure that this is inline with the VS Code abstractions in https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.d.ts. |
IIUC, in this case it would reuse the same view, but the actual widget inside it would be VCS dependent. And I also seem to understand that one assumption made in this change is that their will be only one VCS type/kind per folder? As in, a folder versioned using git won't use mercurial at the same time? @westbury any more insight on this? |
An excellent idea. Those SCM abstractions are close to how I expected this work to become. |
|
This PR has now been obsoleted by #4279. None of the code in this PR is any longer of any value so closing. |
Signed-off-by: Nigel Westbury [email protected]
This PR is the first part of the changes suggested in GH-3423. I have separated them out to make it easier to review and manage the changes.
With this change, the Git view is now a container into which multiple SCM implementation widgets may be injected. Each SCM widget indicates if it can handle a particular repository, and the container will show the first widget that can, or a suitable message if none of the widgets can handle the repository.
If there is only a single SCM supported then the title, tooltip etc for the container view will be taken from the widget for that SCM, otherwise generic text is used. This ensures that there is no change in the UI in core Theia but not using the name 'Git' when a repository may not be a Git repository.
With these changes, other SCMs can be contributed but they will have to copy git-widget.ts and modify it. We should have a base class from which one can override those functions where the SCM differs. This work will be done later but I think it will be easier for everyone if this is done in parts. Also GitHistory has not been changed.
The singletonFill class is there to ensure that the GitWidget expands to fill the container (otherwise the latest commit section is moved up against the changes). Let me know if you prefer a different way of doing this.
There is a message that appears when the repository is not recognised by any of the supported SCMs. It is easy to get that message in our product (Mbed Studio) but no so easy in the Theia example. I can get it by selecting a repository, then deleting the .git directory, then closing and re-opening the Git view.