-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Move Git history view to scm-extra package #6381
Conversation
8030c77
to
0727806
Compare
b8383cf
to
b6b29ce
Compare
Does it make sense to move Git History to SCM? There are already VS Code extensions providing git history. They should be used. cc @svenefftinge |
Are you saying the current Git History should be removed completely from the Theia code base in favor of using VS Code extensions? Unless I am missing something, the VS Code extensions are specific to a single SCM type. Extensions written for Git won't work with Mercurial etc. Our requirement is that the history view shows history for different SCM types, switching between a Git and Mercurial repository in the same way that the current SCM view switches seamlessly between Git and Mercurial repositories. |
Not remove, I don't think it makes sense to have it in the SCM extension, since each vscode extension will bring own history implementation.
Yes, but each VS Code extension can provide a tree view or a webview to implement history specific to a SCM type. I would prefer that we discuss it on dev meeting when Sven is back. Unfortunately in 2 weeks after EclipseCon. It is not clear to me how we should move forward. |
@westbury I've chatted with @svenefftinge about it. Let's keep Also wonder whether you are interested into using |
Looking at the PR: We should test that the layout with git history view created by old Theia is still can be opened. Theia has the layout migration contribution:
|
Yes, moving into scm-extra would certainly work for us. It makes sense that the scm package is the minimal needed to support the vscode SCM extensions. The 'amend' support could also be moved, perhaps by allowing Theia extensions to contribute sections to be bottom of the SCM view. We will obviously need some discussions here on what we do if the rest of the community abandon the Theia git extension. Note the following concerns:
|
Yes, let's discuss it tomorrow. I'll be on the dev meeting. |
ok, added to the agenda: https://github.com/eclipse-theia/theia/wiki/Dev-Meetings#agenda-2019-10-29 |
on Red Hat side, we're using only VS Code scm (we've removed all extensions from 'our' Theia that have VS Code extensions counterpart) |
c008384
to
7d0a058
Compare
7d0a058
to
fb9028e
Compare
54d3e90
to
4a732e9
Compare
I have now
Unfortunately there is now a failure in the integration test. This fails for me locally even on master. Many of the views seems to randomly fail to open and close. |
3b7c44a
to
3d729ef
Compare
The integration tests now pass on CI again, so this is now ready for review. @akosyakov We appreciate your considering this PR because being limited to what we get from VS Code extensions really does not work for us. I understand this may move to a separate repository if you remove native Git extension. |
I'm not sure why I'm seeing this difference in behavior, maybe just a cache issue. There also seems an issue with the git history. In some cases when in the Git history panel a commit is expanded and a file is clicked to see its changes, it the file is opened without showing its name in the monaco navigator. I need to add that this behavior is also present in the master branch. |
Thanks @federicobozzini for testing this. I don't think either of the two issues are problems on the branch.
I can't reproduce this. If I stop Theia while b is the selected repository, then git init a, then restart Theia, I see b selected but a now added to the list. Same if I don't stop Theia. It could be a cache issue as you say if somehow the selection was cleared so the outer repository would be selected as the first in the list.
This one is caused by the label provider. It can be fixed by changing the following line to return 'undefined' (now that that is allowed courtesy of 11be0c0) theia/examples/api-samples/src/browser/sample-dynamic-label-provider-contribution.ts Line 78 in 46a2d51
However the fix to the label provider is bigger than that so needs to happen under a separate PR. The bug is on the master branch. |
@federicobozzini but generally It looks good to you? |
I've done another round of testing and I've found only one minor issue. When focusing on an element in the history panel, a blue border appears around some elements in the history. It looks like it's caused by some css rules that are still applied using the Except for that, I can't see any meaningful difference. |
thanks, blue border should be fixed before merging I test now too. |
Agreed, this will be addressed on Monday |
Please have a look at #6381 (comment) too. This API is not used in the scm extension. I did some testing and could not find anything completely broken. Only thing which bothered me how we name @jbicker @kittaakos maybe you can test a bit too, git view, diff and history in particularly? |
Discussed with @svenefftinge to name it |
f4ff9b0
to
d8fa3e2
Compare
I've made the following changes:
|
Thanks @westbury @federicobozzini could you re-test? |
Re-tested. The issue I pointed out is not present anymore. |
@thegecko @westbury Is it very urgent? Could we await release and land it on Friday? @marcdumais-work release is this Thursday correct? if is is urgent, i'm fine with merging. |
Correct |
No rush, don't want to risk de-stabilising before a release :) |
27594ff
to
7d93537
Compare
I have rebased onto the 0.16.0 release. There were no conflicts other than the usual changelog and version numbers. However there is a test failure which appeared. I have seen this test failure before but it went away after a rebase. It was intermittent in that it appeared when rebasing then went away with the next rebase. I have fixed the tests by making a couple of changes in editor-decoration-style.ts and git-repository-provider.spec.ts. The offending code in editor-decoration-style was never executed when the branch is in a passing state. |
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.
looks good to me
Signed-off-by: Nigel Westbury <nigelipse@miegel.org>
7d93537
to
b24cc0d
Compare
In the future (once it is stabilized), we might benefit from using the new timeline api with the contributed git commit history view: |
@vince-fugnitto could you file an issue we have to implement a widget in the scm extension which supports such API |
I believe it is a proposed API still, and looking at the view it seems to be for the |
@vince-fugnitto agreed maybe we have a look at it in half a year |
What it does
The SCM view currently supports different SCM types through VSCode APIs. However the History view supports Git only. This PR refactors the history view, splitting apart the Git specific code from the history view. This allows a single history view to be shown that tracks the selected repository regardless of whether the repository is a Git repository or a repository from another source manager type such as Mercurial.
This PR moves GitHistoryWidget to ScmHistoryWidget in the SCM package. GitNavigableListWidget and other dependencies have also been moved.
The VSCode does not have API to support history. Therefore we define our own API. I could have implemented this in a similar way to the AmendSupport interface, where the provider provides an implementation. However I decided instead to create a child container for each SCM type, so one child container for Git, one for Mercurial etc. The history support implementations are bound in these containers. The containers containing the repository options are now child containers to these. This approach gives flexibility when adding extra SCM types, or extra an extra interface, or an implementation of an interface for a particular SCM type. So, for example, someone could add support for SVN but not implement the History interface. Someone else could add an extension that implements the History Support interface for SVN and bind it in the SVN child container.
How to test
This PR should be tested to ensure that it works with Git repositories exactly as before. The only differences one should notice is that the view label has been changed from 'SCM' and 'Git History' to 'Source Control' and 'Source History'. The label change was preferred by our UX team but if there are reasons for using the same label as VSCode then I can change it back and we will override it in our application.
Review checklist
Reminder for reviewers