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

Fix cloning using Open From GitHub on Visual Studio 2015 #1982

Merged
merged 13 commits into from Oct 11, 2018

Conversation

Projects
None yet
3 participants
@jcansdale
Contributor

jcansdale commented Oct 9, 2018

This PR targets release/2.5.8

The aim of this PR is to make VSGitServices.Clone work as consistently as possible on Visual Studio 2015 and 2017. The clone operation should do the following on both versions:

  1. Start the clone operation
  2. Show clone progress bar on Team Explorer - Home
  3. Change context to the cloned repository

On Visual Studio 2017 the cloned repository will be opened as a Folder in Solution Explorer. This happens automatically when a repository is cloned and is equivalent to using the new File > Open > Folder...) command.

On Visual Studio 2015 the Team Explorer context is changed (using VSService.TryOpenRepository), but the Folder view doesn't exist on Visual Studio 2015.

What this PR does

  • Make sure the IGitRepositoriesExt service can always be found
  • Always show progress bar on Team Explorer - Home while cloning
  • Consistently detect clone completion on Visual Studio 2015 when progress bar is hidden
  • Don't attempt to open repository if clone fails (no .git folder)
  • Use Throttle to avoid false positives if progress bar glitches hidden
  • Use StartWith(false) so clone doesn't wait forever if progress section never appears

How to test

The following should work on Visual Studio 2015 and 2017

  1. Open Clone dialog using File > Open > Open from GitHub...
  2. Select a repository to clone (e.g. type dotnet/BenchmarkDotNet on URL tab)
  3. Click Clone button
  4. Expect progress bar to appear on Team Explorer - Home
    image
  5. Expect repository context to chance when clone completes (not before)
    image

When a clone operation fails

  1. Open any solution in Visual Studio 2015
  2. Open Clone dialog using File > Open > Open from GitHub...
  3. Select a repository that doesn't exist (e.g. type foo/bar on URL tab)
  4. Click Clone button
  5. Expect Failed to clone... notification to appear
    image
  6. Click the Cancel button
  7. Expect original solution/repository to remain and notification to disappear

On Visual Studio 2015, clone completion will only be detected when Team Explorer - Home is the topmost page in Team Explorer. You can confirm this by doing the following:

  1. Open Clone dialog using File > Open > Open from GitHub...
  2. Select a repository to clone (e.g. type dotnet/BenchmarkDotNet on URL tab)
  3. Click Clone button
  4. Click Manage Connections, which will navigate to Team Explorer - Connect.
  5. Wait for the clone to complete (you should find a clone progress further down on this page)
  6. Click Home, which will navigate to Team Explorer - Home
  7. Expect the repository context to chance as soon as Team Explorer - Home is visible

I tested some of the Rx logic using the following test:

        [TestCase("", "", true)]
        [TestCase("x", "", true)]
        [TestCase("", "x", true)]
        [TestCase("", "xox", true)]
        [TestCase("", "xo", false)]
        public async Task SwitchTest(string a, string b, bool expect)
        {
            var observable = new[] { a, b }.ToObservable();

            var result = await observable
                .Select(w => w.ToCharArray().ToObservable())
                .Switch()
                .StartWith('x')
                .Throttle(TimeSpan.FromSeconds(1))
                .Any(c => c == 'x');

            Assert.That(result, Is.EqualTo(expect));
        }

Fixes #1981, Fixes #1982

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 9, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (release/2.5.8@b2385e6). Click here to learn what that means.
The diff coverage is 0%.

@@               Coverage Diff                @@
##             release/2.5.8    #1982   +/-   ##
================================================
  Coverage                 ?   41.04%           
================================================
  Files                    ?      403           
  Lines                    ?    17167           
  Branches                 ?     2381           
================================================
  Hits                     ?     7047           
  Misses                   ?     9568           
  Partials                 ?      552
Impacted Files Coverage Δ
src/GitHub.VisualStudio.UI/Views/ViewLocator.cs 0% <ø> (ø)
src/GitHub.App/ViewModels/CommentViewModel.cs 65.59% <ø> (ø)
...rc/GitHub.App/ViewModels/CommentThreadViewModel.cs 90.9% <ø> (ø)
src/GitHub.App/Services/CommentService.cs 0% <ø> (ø)
src/GitHub.App/Services/RepositoryCloneService.cs 48.95% <ø> (ø)
src/GitHub.VisualStudio.UI/Views/ContentView.cs 0% <ø> (ø)
...rc/GitHub.VisualStudio.UI/Views/ActorAvatarView.cs 0% <ø> (ø)
...iews/GitHubPane/FileNameToImageMonikerConverter.cs 0% <ø> (ø)
...neReviews/ViewModels/InlineCommentPeekViewModel.cs 76.85% <ø> (ø)
...Pane/DirectoryIsExpandedToImageMonikerConverter.cs 0% <ø> (ø)
... and 1 more

codecov bot commented Oct 9, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (release/2.5.8@b2385e6). Click here to learn what that means.
The diff coverage is 0%.

@@               Coverage Diff                @@
##             release/2.5.8    #1982   +/-   ##
================================================
  Coverage                 ?   41.04%           
================================================
  Files                    ?      403           
  Lines                    ?    17167           
  Branches                 ?     2381           
================================================
  Hits                     ?     7047           
  Misses                   ?     9568           
  Partials                 ?      552
Impacted Files Coverage Δ
src/GitHub.VisualStudio.UI/Views/ViewLocator.cs 0% <ø> (ø)
src/GitHub.App/ViewModels/CommentViewModel.cs 65.59% <ø> (ø)
...rc/GitHub.App/ViewModels/CommentThreadViewModel.cs 90.9% <ø> (ø)
src/GitHub.App/Services/CommentService.cs 0% <ø> (ø)
src/GitHub.App/Services/RepositoryCloneService.cs 48.95% <ø> (ø)
src/GitHub.VisualStudio.UI/Views/ContentView.cs 0% <ø> (ø)
...rc/GitHub.VisualStudio.UI/Views/ActorAvatarView.cs 0% <ø> (ø)
...iews/GitHubPane/FileNameToImageMonikerConverter.cs 0% <ø> (ø)
...neReviews/ViewModels/InlineCommentPeekViewModel.cs 76.85% <ø> (ø)
...Pane/DirectoryIsExpandedToImageMonikerConverter.cs 0% <ø> (ø)
... and 1 more

@jcansdale jcansdale changed the title from [wip] Fix cloning using Open From GitHub on Visual Studio 2015 to Fix cloning using Open From GitHub on Visual Studio 2015 Oct 10, 2018

@jcansdale jcansdale requested a review from grokys Oct 10, 2018

jcansdale added some commits Oct 9, 2018

Add method to find IGitRepositoriesExt
Stop relying on the state of IGitHubServiceProvider to locate
IGitRepositoriesExt.
Don't show Connect when Start Page button clicked
There's no more need to explicitly show the Team Explorer - Connect
page when the Start Page GitHub button is clicked. This will happen
automatically before cloning.
Show clone progress on Team Explorer - Home
A clone progress bar and option to cancel appears on Team Explorer -
Home while cloning. Navigate to Team Explorer - Home, when clone
operation is about to start.
TryOpenRepository when clone completes on VS 2015
We want the Clone behavior to be as similar as possible on Visual
Studio 2015 and 2017.
Show Team Explorer - Home after clone
This gives user an opportunity to choose a solution.
TryOpenRepository on empty target folder
If the user navigates away from the Connect page, the CanClone property
will stop updating and the Clone method won't return. Point Team
Explorer at the target folder when clone starts incase this happens.
Detect completion using visibility of progress bar
Detect when a clone completes by waiting for the clone progress bar on
the Team Explorer Home page to be hidden.
Look for progress bar on any Home page
Look for clone progress bar on any Team Explorer Home page that the
user navigates to. The Home page will need to be the topmost page on
Team Explorer for the clone to complete.
Navigate to Home after clone starts on 2017
- Be consistent between 2015 and 2017 by navigating to Home page after
clone starts.
- There's no need to use JoinableTaskFactory.RunAsync when a task is
being awaited.
Move StartClonenOnConnectPageAsync into Clone
Move NavigateToHomePage out of WaitForCloneOnHomePageAsync to show the
stages more clearly.

@jcansdale jcansdale changed the base branch from master to release/2.5.8 Oct 10, 2018

@sguthals

This comment has been minimized.

Show comment
Hide comment
@sguthals

sguthals Oct 11, 2018

Contributor

I just confirmed that this worked on VS2015 Community 👍

Looks like there is still a build failing though.

Contributor

sguthals commented Oct 11, 2018

I just confirmed that this worked on VS2015 Community 👍

Looks like there is still a build failing though.

@sguthals

LGTM other than the build that's failing

@@ -59,7 +57,6 @@ async Task<CodeContainer> RunAcquisition(IProgress<ServiceProgressData> download
try
{
var uiProvider = await Task.Run(() => Package.GetGlobalService(typeof(IGitHubServiceProvider)) as IGitHubServiceProvider);
await ShowTeamExplorerPage(uiProvider);

This comment has been minimized.

@sguthals

sguthals Oct 11, 2018

Contributor

So this was the piece that was waiting for the Team Explorer to let us know that the clone had completed?

@sguthals

sguthals Oct 11, 2018

Contributor

So this was the piece that was waiting for the Team Explorer to let us know that the clone had completed?

This comment has been minimized.

@jcansdale

jcansdale Oct 11, 2018

Contributor

This was a hack to make the IGitRepositoriesExt service available later on when the Clone method attempts to retrieve it from IGitHubServiceProvider.

The problem was that it required two pieces to be in place before it wold work.

  1. The Team Explorer - Connext page must be visible
  2. Something must have set the GitServiceProvider on IGitHubServiceProvider

This was way too fragile and the reason the bug crept in.

@jcansdale

jcansdale Oct 11, 2018

Contributor

This was a hack to make the IGitRepositoriesExt service available later on when the Clone method attempts to retrieve it from IGitHubServiceProvider.

The problem was that it required two pieces to be in place before it wold work.

  1. The Team Explorer - Connext page must be visible
  2. Something must have set the GitServiceProvider on IGitHubServiceProvider

This was way too fragile and the reason the bug crept in.

@@ -39,6 +36,8 @@ public class VSGitServices : IVSGitServices
[SuppressMessage("Microsoft.Performance", "CA1823:AvoidUnusedPrivateFields", Justification = "Used in VS2017")]
readonly Lazy<IStatusBarNotificationService> statusBar;
[SuppressMessage("Microsoft.Performance", "CA1823:AvoidUnusedPrivateFields", Justification = "Used in VS2015")]
readonly Lazy<IVSServices> vsServices;

This comment has been minimized.

@sguthals

sguthals Oct 11, 2018

Contributor

Can you quickly explain this bit for my edification?

@sguthals

sguthals Oct 11, 2018

Contributor

Can you quickly explain this bit for my edification?

This comment has been minimized.

@jcansdale

jcansdale Oct 11, 2018

Contributor

There is a TryOpenRepository method on IVSServices that we only need to call when running on Visual Studio 2015. On Visual Studio 2017, the cloned repository automatically opens in folder view (which is new in Visual Studio 2017).

If we didn't call TryOpenRepository, you wouldn't see the URL change on Team Explorer - Home and there'd be no option to open a solution. You'd need to manually find and open a solution in the cloned folder.

@jcansdale

jcansdale Oct 11, 2018

Contributor

There is a TryOpenRepository method on IVSServices that we only need to call when running on Visual Studio 2015. On Visual Studio 2017, the cloned repository automatically opens in folder view (which is new in Visual Studio 2017).

If we didn't call TryOpenRepository, you wouldn't see the URL change on Team Explorer - Home and there'd be no option to open a solution. You'd need to manually find and open a solution in the cloned folder.

// The operation will have completed when CanClone goes false and then true again.
await gitExt.WhenAnyValue(x => x.CanClone).Where(x => !x).Take(1);
await gitExt.WhenAnyValue(x => x.CanClone).Where(x => x).Take(1);
#if TEAMEXPLORER14

This comment has been minimized.

@sguthals

sguthals Oct 11, 2018

Contributor

So this is the new bit right? So instead of waiting for the response from TE, we're starting the clone and immediately navigating to the TE Home page and then the user will see the TE change as a result of the clone completing but TE is handling that. And we can identify that change (instead of actually waiting for an update from TE)

@sguthals

sguthals Oct 11, 2018

Contributor

So this is the new bit right? So instead of waiting for the response from TE, we're starting the clone and immediately navigating to the TE Home page and then the user will see the TE change as a result of the clone completing but TE is handling that. And we can identify that change (instead of actually waiting for an update from TE)

This comment has been minimized.

@jcansdale

jcansdale Oct 11, 2018

Contributor

We can infer that the clone operation has completed when the progress bar is hidden and automatically change context to the newly cloned repository. If we didn't change repository context at this point, the user would know the clone had completed, but they'd be left to find and open a solution from the newly cloned folder (not a nice experience).

@jcansdale

jcansdale Oct 11, 2018

Contributor

We can infer that the clone operation has completed when the progress bar is hidden and automatically change context to the newly cloned repository. If we didn't change repository context at this point, the user would know the clone had completed, but they'd be left to find and open a solution from the newly cloned folder (not a nice experience).

static async Task WaitForCloneOnHomePageAsync(ITeamExplorer teamExplorer)
{
// The clone progress bar appears on the GettingStartedSection of the Home page,
// so we wait for this to be hidden before continuing.

This comment has been minimized.

@sguthals

sguthals Oct 11, 2018

Contributor

@sguthals

sguthals Oct 11, 2018

Contributor

jcansdale added some commits Oct 11, 2018

Ignore glitch where section starts invisible
If no events arrive default to invisible.
Watch the topmost section.
Only open repository when .git directory exists
We don't want to change context when a clone operation has failed.
@grokys

grokys approved these changes Oct 11, 2018

@jcansdale jcansdale merged commit c38cd21 into release/2.5.8 Oct 11, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jcansdale jcansdale deleted the fixes/1981-open-from-github-clone-2015 branch Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment