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

Unified Clone / Open from GitHub UI #1919

Merged
merged 46 commits into from Sep 27, 2018

Conversation

Projects
None yet
4 participants
@jcansdale
Contributor

jcansdale commented Sep 13, 2018

What this PR does

  • Allow user to Open a repository when the target path already exists
  • View/open cloned repositories on Team Explorer Home (same as built in clone)
  • In Visual Studio 2017 open cloned repositories in Folder view (same as built in clone)
  • Fix GitHub button on Start Page
  • Add Add File / Open / Open from GitHub... button
  • Pre-populate Url from clipboard/command args when available
  • Only show warning banner when a file is in the way of the target path
  • Use Clone/Open dialog for existing GitHub.OpenFromUrl command

TODO

  • Add metrics

What this PR doesn't do

The GitHub.OpenFromUrl command will no longer attempt to navigate the the file and line number from a target Url. This was done to trim the scope of this PR to simply cloning/opening a repository.

The next step will be to incorporate functionality from the Open from clipboard command. This will be more complex to test and will be done as a separate PR.

At the moment these two commands can be combined manually:

  1. Copy a GitHub URL to the clipboard
  2. Use Add File / Open / Open from GitHub...
  3. Use GitHub > Open from clipboard

How to test

Here is an example workflow

  1. Copy a GitHub URL
    image

  2. Select File > Open > Open from GitHub...
    image

  3. URL is copied into username/repository field and path is populated with a default location
    image

  4. Click Open if local repository already exists or update path and Clone to create a new repository
    image

  5. Repository folder is loaded into Solution Explorer and Team Explorer - Home appears
    image

  6. Select from list of solutions

jcansdale added some commits Sep 10, 2018

Use clone dialog as OpenFromUrl UI
Use clone from URL as the UI for GitHub.OpenFromUrl.
Add File / Open / Open from GitHub... button
Now we have a proper UI, re-enable the `File / Open / Open from
GitHub...` button.
Add RepositoryCloneService.CloneOrOpenRepository
Add method for cloning or opening a repository.
Add ITeamExplorerServices.OpenRepository
Add method to open a repository and view it on Team Explorer Home.
Don't attempt to open folder on Visual Studio 2015
Use VSServices.TryOpenRepository on Visual Studio 2015 where solution
folders aren't available. This creates a temporary solution to make
Team Explorer change its active repository.
Only show path error when file is in way
When a directory already exists we can open the repository, so only
show a path error when a file exists at the target path.
Fix CA errors
IVSServices is only used in Visual Studio 2015 so retrieve it on demand.
Return Url instead of IRepositoryModel
Be flexible about the type of Url that CloneDialogResult can return.
Make CloneOrOpenRepository accept URL
Convert ToRepositoryUrl inside this method.
Simplify OpenFromUrlCommand
This command is now only responsible for opening the clone and calling
CloneOrOpenRepository.

@jcansdale jcansdale changed the title from [spike] Unified Clone / Open from GitHub UI to Unified Clone / Open from GitHub UI Sep 18, 2018

@jcansdale jcansdale requested review from grokys and meaghanlewis Sep 18, 2018

jcansdale and others added some commits Sep 19, 2018

@meaghanlewis

This comment has been minimized.

Show comment
Hide comment
@meaghanlewis

meaghanlewis Sep 21, 2018

Contributor

This LGTM @jcansdale

Contributor

meaghanlewis commented Sep 21, 2018

This LGTM @jcansdale

jcansdale added some commits Sep 25, 2018

Only increment NumberOfClones during clone
It appears that CloneRepository is also called when a new repository is
created. Only increment the new counters during CloneOrOpenRepository.
Increment new counters in CloneOrOpenRepository
Increment new NumberOfGitHubClones and NumberOfEnterpriseClones
counters in CloneOrOpenRepository but not CloneRepository.
Add tests for NumberOf*Opens counters
Expect to fail on this commit.
Increment NumberOf*Opens counters
Increment NumberOfGitHubOpens and NumberOfEnterpriseOpens counters when
CloneOrOpenRepository is called.
@@ -121,20 +172,7 @@ public async Task<ViewerRepositoriesModel> ReadViewerRepositories(HostAddress ad
try
{
await vsGitServices.Clone(cloneUrl, repositoryPath, true, progress);
await usageTracker.IncrementCounter(x => x.NumberOfClones);

This comment has been minimized.

@jcansdale

jcansdale Sep 25, 2018

Contributor

This method is being called by CreateRepository here:

.Select(repository => cloneService.CloneRepository(repository.CloneUrl, Path.Combine(directory, repository.Name)))

@jcansdale

jcansdale Sep 25, 2018

Contributor

This method is being called by CreateRepository here:

.Select(repository => cloneService.CloneRepository(repository.CloneUrl, Path.Combine(directory, repository.Name)))

jcansdale and others added some commits Sep 25, 2018

Update DestinationAlreadyExists warning
Change to "A file exists at the destination path."
Sanity check local repository
Check the following conditions:
- Local repository exists at path
- Local repository has an 'origin' remote
- Local repository's URL matches selected repository
Consolidate PathError into PathWarning
There was an issue with PathError appearing at the top. When the error
appeared, the repository list would move down causing a different
repository to be selected. This would clause flickering between two
repositories.

This commit consolidates PathError into PathWarning (which appears
above the path which is being validated).  This avoids the flicker and
puts the warning next to the text box which it applies to.

The logic to block clone or open when there is a file in the way has
been moved to the commands' `canExecute` observable.
@jcansdale

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Sep 27, 2018

Contributor

@grokys re: #1919 (comment)

Should we be checking whether the repository at this path has the same clone URL? I'm thinking of the case where e.g. the user wants to clone a fork and they already have the parent repository at this location.

I've added checks for this and also for when there is no repository at the target path or when the repository doesn't have a remote named origin.

I've moved the warning banner to above the Local path text box that it's validating. This was for a couple of reasons.

  1. Having it appear above the tree view meant that when it appeared, it moved the tree view control that the user might have clicked on. This lead to rapid cycling between two selections.

  2. Moving it to above the field that it's validating means the context is obvious and the warnings can be more concise (we're somewhat restricted for space).

Here are the warnings:

  • When there is no repository at the local path (e.g. an empty folder):
    image

  • When the remote URL of the local repository is different to the URL selected:
    image

  • When there is a file in the way of the local path (in this case there is a file called PReview:
    image

  • When the target repository doesn't have a remote named origin and won't be recognized by GHfVS:
    image

Contributor

jcansdale commented Sep 27, 2018

@grokys re: #1919 (comment)

Should we be checking whether the repository at this path has the same clone URL? I'm thinking of the case where e.g. the user wants to clone a fork and they already have the parent repository at this location.

I've added checks for this and also for when there is no repository at the target path or when the repository doesn't have a remote named origin.

I've moved the warning banner to above the Local path text box that it's validating. This was for a couple of reasons.

  1. Having it appear above the tree view meant that when it appeared, it moved the tree view control that the user might have clicked on. This lead to rapid cycling between two selections.

  2. Moving it to above the field that it's validating means the context is obvious and the warnings can be more concise (we're somewhat restricted for space).

Here are the warnings:

  • When there is no repository at the local path (e.g. an empty folder):
    image

  • When the remote URL of the local repository is different to the URL selected:
    image

  • When there is a file in the way of the local path (in this case there is a file called PReview:
    image

  • When the target repository doesn't have a remote named origin and won't be recognized by GHfVS:
    image

jcansdale added some commits Sep 27, 2018

Show warning when local clone already exists
"You have already cloned to this location. Click 'Open' to open the
local repository."
@jcansdale

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Sep 27, 2018

Contributor
  • It will now always this this when a local clone already exists:
    image
Contributor

jcansdale commented Sep 27, 2018

  • It will now always this this when a local clone already exists:
    image
@jcansdale

This comment has been minimized.

Show comment
Hide comment
@jcansdale

jcansdale Sep 27, 2018

Contributor

Here are the updated warning messages:

image

image

image

image

Contributor

jcansdale commented Sep 27, 2018

Here are the updated warning messages:

image

image

image

image

jcansdale added some commits Sep 27, 2018

Update Clone/Open enabled tests
Add Open_Is_Enabled_When_Path_DirectoryExists test
Fix/update some other tests
@StanleyGoldman

Should the user message here change?

jcansdale added some commits Sep 27, 2018

Add unit tests for ValidatePathWarning
PathWarning_Is_Set_For_Existing_Clone_At_Destination
PathWarning_Is_Set_For_Repository_With_No_Origin
PathWarning_Is_Set_For_Directory_With_No_Repository
PathWarning_Is_Set_For_Existing_Repository_At_Destination_With_Different_Remote
Update ErrorType.ClonedFailed message
Changed ClonedFailed to CloneOrOpenFailed.

@jcansdale jcansdale referenced this pull request Sep 27, 2018

Merged

Documentation updates #1943

@jcansdale jcansdale merged commit b582064 into master Sep 27, 2018

2 checks passed

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

@jcansdale jcansdale deleted the feature/open-from-uri-clone-ui branch Sep 27, 2018

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