Skip to content
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

Open from clipboard url #1771

Merged
merged 69 commits into from Jul 13, 2018

Conversation

Projects
None yet
4 participants
@jcansdale
Copy link
Collaborator

jcansdale commented Jul 9, 2018

What this PR does

This PR is the first component of some more more general - Open from GitHub URL - functionality. It enables navigation from a URL that's in the clipboard to a file in the active solution. It won't offer to change branches or open a different solution or folder. The idea is to hand over to future clone/open UI when necessary before continuing the navigate operation.

It is designed to be complimentary with existing Copy link to clipboard functionality (available from the GitHub code context menu). You should be able to navigate back to any URL created with GitHub > Copy link to clipboard by using the GitHub > Open from clipboard command.

image

To do

  • Show a sensible warning when attempting to open an unsupported GitHub URL type
  • Only show GitHub > Open from clipboard when there's an active repository

What works

// Open a file from a named named branch
https://github.com/github/VisualStudio/blob/master/src/common/SolutionInfo.cs

// Open a single line from a named branch
https://github.com/github/VisualStudio/blob/e1bfa231c2d7dea100f09aaeda4697855327e8b7/src/common/SolutionInfo.cs#L6

// Open a selection from a named branch
https://github.com/github/VisualStudio/blob/e1bfa231c2d7dea100f09aaeda4697855327e8b7/src/common/SolutionInfo.cs#L20-L22

// Open a file from a permalink
https://github.com/github/VisualStudio/blob/e1bfa231c2d7dea100f09aaeda4697855327e8b7/README.md

// Open a single line form a permalink
https://github.com/github/VisualStudio/blob/e1bfa231c2d7dea100f09aaeda4697855327e8b7/src/GitHub.VisualStudio/source.extension.vsixmanifest#L15

// Open a selection from a permalink
https://github.com/github/VisualStudio/blob/e1bfa231c2d7dea100f09aaeda4697855327e8b7/src/GitHub.VisualStudio/LICENSE.txt#L9-L19

// Open a permalink for a file that has changed in the current solution
 https://github.com/github/VisualStudio/blob/a3920b770b65fe5a6034550cad69b9b58f5e00dd/README.md

// Open a permalink for a range of lines that have changed in the current solution
https://github.com/github/VisualStudio/blob/c21d2b0e4f5166a95d99aea51f1c25b724543f06/src/GitHub.VisualStudio/Services/GitHubServiceProvider.cs#L190-L201

// Open a file as it was on a named tag
https://github.com/github/VisualStudio/blob/v2.2.0.4/src/GitHub.VisualStudio/GitHubPackage.cs

// Open a file 4 commits behind master
https://github.com/github/VisualStudio/blob/master~4/README.md

// Permalinks on someone else's fork
https://github.com/jcansdale/VisualStudio/blob/e1bfa231c2d7dea100f09aaeda4697855327e8b7/src/GitHub.VisualStudio/GitHubPackage.cs#L36

// File from a named branch one someone else's fork (assuming the file exists)
https://github.com/jcansdale/VisualStudio/blob/master/src/GitHub.VisualStudio/GitHubPackage.cs

Warnings

// Attempt to open a file on a different repository
https://github.com/github/VisualStudioops/blob/master/src/common/SolutionInfo.cs
"Please open the repository 'VisualStudioops' and try again"

// Attempt to open a branch doesn't exist in the local repository
https://github.com/github/VisualStudio/blob/awesome-branch/src/common/SolutionInfo.cs
"Couldn't find target URL in current repository. Try again after doing a fetch."

// Attempt to open a branch from a fork that doesn't exist in the current repository
https://github.com/jcansdale/VisualStudio/blob/awesome-branch/src/common/SolutionInfo.cs
"The target URL has a different owner to the current repository."

// Attempt to open when the clipboard is empty
"Couldn't a find a GitHub URL in clipboard"

// Attempt to open when there is non-URL text in clipboard
"Couldn't a find a GitHub URL in clipboard"

// Attempt to open when current solution isn't in a repository
"There is no active repository to navigate"
(note, we shouldn't show the command in this situation)

// Blame/Annotate isn't supported on VS 2015
// When a file is different in the working directory we show
"This file has changed since the permalink was created"

Known Issues

// Attempt to open currently unsupported GitHub URL type
https://github.com/github/VisualStudio/commit/c909f4527f8b5d2972c2b7b7dee1867083fdeef2
https://github.com/github/VisualStudio/pull/1773
"Couldn't find target URL in current repository. Try again after doing a fetch."

Reviewers

  • My warning messages are terrible! Suggestions for improvements would be much appreciated
  • Are there other warnings that I should be showing?
  • Is the name/icon for the command okay: Open from clipboard
  • The code for the original Open from GitHub.../OpenFromUrlCommand spike still exists, but isn't surfaced on the UI anywhere. It is only accessible by biding a keyboard shortcut to GitHub.OpenFromUrl. Is this okay or should it be removed?

jcansdale added some commits Jun 22, 2018

Add GitHub.OpenFromUrl command
Use `GitHub.OpenFromUrl <url>` from the VS Command Window to clone a
target URL. This spike uses existing `ShowReCloneDialog` method.
Update UriString to handle extended GitHub URLs
Expect Owner="github", RepositoryName="VisualStudio" from:
https://github.com/github/VisualStudio/blob/master/README.md

Not Owner="master", RepositoryName="README.md"
Support opening target from GitHub URL
Only files on master branch are currently supported.
Allow navigating to URL from clipboard
Trim line number when using FindPath.
Checkout or open using default path
Prototype this using the happy path where we use the default repository
location.
Don't open folder if current solution is inside it
If we're already in an appropriate context, simply open the target file.
Update UriString to allow http URLs with owner
The following URL will now have an owner of "owner":
https://github.com/owner
Previously it would make a repository name of "owner".
Find GitHub context from Chrome window title
Add code to parse window titles and find the GitHub context from the
topmost Chrome browser.
Add support for opening from topmost browser
If there is no URL in the paste buffer, use the GitHub context from the
topmost browser.
Support syncing the file open in topmost browser
When browser is in the context of a file, open a file with the same
name after repository is opened.
Factor GitHub URL parsing into FindContextFromUrl
OpenFromUrlCommand now works with GitHubContext objects rather than
directly with URLs.
Convert from GitHub to Visual Studio line numbers
Visual Studio line numbers are zero based.
Add support for file links that use a commit SHA
Assume that branch names don't contain a '/' (sic).
Fix a few typos
Navigate to line when line number information is available.
Update title regex to work with Firefox
Search for Chrome then Firefox windows.
Need to find a way to search in order of height.
Highlight line when single line is selected
Previously highlighting only worked when a range was selected.
Use VsShellUtilities.OpenDocument instead of
DTE.ItemOperations.OpenFile.
Factor TryOpenFile into GitHubContextService.
Select just the target lines
Don't continue to column 0 on the line below.
Rename Treeish to TreeishPath
GitHub URLs combine branches/SHAs and tree paths.

For example:
https://github.com/github/visualstudio/tree/master/src

Git would represent "master/src" as the tree-ish "master:src". Because
looking at the URL we don't know where the branch stops and the tree
starts, "master/src" is being stored as the TreeishPath property.
@grokys
Copy link
Contributor

grokys left a comment

This seems to work well! I personally think the error messages are good enough for now. This feature could really use a UI to help ease people into it, but this is good as a MVP!

Got a few questions on the code, and also it would be good to get some XML documentation in there.

[TestCase("https://git01.codeplex.com/nuget", "git01.codeplex.com", "nuget", null,
Description = "We assume the first component is the owner")]
[TestCase("https://example.com/vpath/foo/bar", "example.com", "vpath", "foo")]
[TestCase("https://example.com/vpath/foo/bar.git", "example.com", "vpath", "foo")]

This comment has been minimized.

@grokys

grokys Jul 12, 2018

Contributor

vpath here suggests "virtual path" which I assume is because enterprise installations can have a virtual path before the owner? I this PR changes the parsing of this to not work with vpaths, but maybe we should change the string vpath to something else here?

void InvokeService<T1, T2, T3, T4>(Type serviceType, string method, T1 arg1, T2 arg2, T3 arg3, T4 arg4)
{
var service = serviceProvider.GetService(serviceType);
var action = (Action<T1, T2, T3, T4>)Delegate.CreateDelegate(typeof(Action<T1, T2, T3, T4>), service, method);

This comment has been minimized.

@grokys

grokys Jul 12, 2018

Contributor

Ohh, magic! Could you explain what's happening here? :)

This comment has been minimized.

@jcansdale

jcansdale Jul 12, 2018

Author Collaborator

The IGitExt2 interface was introduced in an update of Visual Studio 2017, so we can't be sure it exists. I'm grabbing the IGitExt2 type by name and if it exists constructing and invoking a delegate on the AnnotateFile method.

My helper method was retrieving the service and calling the method. I've simplified it to only create and invoke the delegate. Using reflection was just too ugly. ;-)

The method is now:

            void Invoke<T1, T2, T3, T4>(object target, string method, T1 arg1, T2 arg2, T3 arg3, T4 arg4)
            {
                var action = (Action<T1, T2, T3, T4>)Delegate.CreateDelegate(typeof(Action<T1, T2, T3, T4>), target, method);
                action.Invoke(arg1, arg2, arg3, arg4);
            }
namespace GitHub.App.Services
{
[Export(typeof(IGitHubContextService))]
public class GitHubContextService : IGitHubContextService

This comment has been minimized.

@grokys

grokys Jul 12, 2018

Contributor

We now have GitHub.Services.Vssdk - seeing as this service mostly deals with the VSSDK, would that be a better place for it?

This comment has been minimized.

@jcansdale

jcansdale Jul 12, 2018

Author Collaborator

Given its dependencies on the Microsoft.VisualStudio.* assemblies, I think this makes sense.

using LibGit2Sharp;
using Task = System.Threading.Tasks.Task;

namespace GitHub.App.Services

This comment has been minimized.

@grokys

grokys Jul 12, 2018

Contributor

Remove the .App from the namespace.

return context;
}

public GitHubContext FindContextFromBrowser()

This comment has been minimized.

@grokys

grokys Jul 12, 2018

Contributor

I thought we'd decided not to put this feature in?

This comment has been minimized.

@jcansdale

jcansdale Jul 12, 2018

Author Collaborator

This feature is on a back-burner, but the code and the hope is to eventually reactivate it.

The command that uses it GitHub.OpenFromUrl has been removed from the UI. It was previously surfaced via File > Open > Open from GitHub.... The command that is surfaced via GitHub > Open from clipboard is GitHub.OpenFromClipboard`.

It does make sense to remove it from GitHub.OpenFromUrl (which needs to be explicitly bound to a keyboard shortcut).

jcansdale added some commits Jul 12, 2018

Show dialog when attempting to open non-blob URL
"Couldn't open from 'https://github.com/owner/name'. Only URLs that
link to repository files are currently supported."
We don't support vpath in a URL
Can't find anything about vpaths being a thing on GitHub Enterprise.
Remove FindContextFromBrowser from hidden command
The `GitHub.OpenFromUrl` command isn't exposed on the UI but still
shouldn't be calling FindContextFromBrowser .
@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

jcansdale commented Jul 12, 2018

@grokys,

I started moving GitHubContextService to the GitHub.Services.Vssdk project, but realized it has dependencies on LibGit2Sharp and System.Windows. I'm wondering if it would be better to keep GitHub.Services.Vssdk for purer VS SDK helper classes (of which there could be many).

What do you think?

@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

jcansdale commented Jul 12, 2018

This feature could really use a UI to help ease people into it, but this is good as a MVP!

It would be nice if we could have a Google style, combined search and URL box:
image

Search or enter a GitHub URL...

image

What do you reckon @donokuda, @sguthals & @grokys? 😄

@meaghanlewis

This comment has been minimized.

Copy link
Contributor

meaghanlewis commented Jul 12, 2018

@jcansdale this feature is working as expected . It's nice being able to easily open files from what's pasted in the clipboard!

I think the error messages are good as they are. Whenever I saw a message it was usually always clear what was happening.

The name/icon Open from clipboard is fine for now in this MVP version. It will be exciting to see a UI with this feature so it can be discovered more easily. Once that happens, it will be great to get metrics hooked up.

As for the code from the original spike, it's probably okay to keep around if you think you'll refer back to it in the future. Otherwise it might be good to remove any unused code.

@meaghanlewis meaghanlewis added this to the 2.5.4 milestone Jul 12, 2018

@grokys

This comment has been minimized.

Copy link
Contributor

grokys commented Jul 12, 2018

As for the code from the original spike, it's probably okay to keep around if you think you'll refer back to it in the future. Otherwise it might be good to remove any unused code.

Yeah, I generally prefer to keep unused code out of master, as it tends to rot. You can always keep it in a branch for easy recall later! I'll leave this to be your call though.

@donokuda

This comment has been minimized.

Copy link
Member

donokuda commented Jul 12, 2018

This feature could really use a UI to help ease people into it, but this is good as a MVP!

It would be nice if we could have a Google style, combined search and URL box:
image

Search or enter a GitHub URL...

image

What do you reckon @donokuda, @sguthals & @grokys? 😄

I like this idea! I'm a fan of explicit actions over convenience (e.g., reading the clipboard contents without user input) since it gives a level of control back to the user by letting them double check if they're opening the right URL. I think you were using the search bar as a general example, but what are your thoughts of having a dialog with a text field instead?

Maybe something similar to:

It is designed to be complimentary with existing Copy link to clipboard functionality (available from the GitHub code context menu). You should be able to navigate back to any URL created with GitHub > Copy link to clipboard by using the GitHub > Open from clipboard command.

Only thing I'm not sure about is having the entry point for opening a URL in the editor context menu. This is primarily because I expect actions in the editor context menu to be related to the file at hand. While I understand the reasoning of "Open from clipboard" being a complimentary action to "Copy link to clipboard," I think the times when someone will want to copy a link to their clipboard vs open a link from GitHub are so far apart that the connection between the actions won't be obvious enough (Holy run-on-sentence, batman! 😄)

I think having this action apart of the File > Open from GitHub... menu makes more sense. Is the expectation that we'll move this command to there and deprecate the context menu?

@jcansdale

This comment has been minimized.

Copy link
Collaborator Author

jcansdale commented Jul 13, 2018

@donokuda,

I think you were using the search bar as a general example, but what are your thoughts of having a dialog with a text field instead?

This was also a specific suggestion because users are now very familiar with the combined address/search field. I think it was originally pioneered in Chrome, but Firefox and Edge have followed. The GitHub pane is also a kind of browser for GitHub that internally has its own URL scheme.

Having a dialog with a text field will greatly expand our options when it comes to communicating to the user what kind of URLs are supported and what action they want to take. E.g. when opening a URL from another a different fork, do they want to navigate in the current solution or open/clone the specific fork? We'll definitely need this for the Open from GitHub... command.

I think having this action apart of the File > Open from GitHub... menu makes more sense. Is the expectation that we'll move this command to there and deprecate the context menu?

I'd be fine with that. File > Open > Open from GitHub... is by far the more discoverable place.

TBH, I think the discoverability of the Code context > GitHub commands is pretty horrible. I bet most people don't even know that you can create a Gist from some selected text!

We should maybe consider moving some of these command to a main GitHub menu close to where the Team menu is now. Users could then more easily discover Create a Gist from selection and maybe in future Create an issue from selection (once we have issue support).

jcansdale added some commits Jul 13, 2018

Only show command when in context of git repo
Since `Open from clipboard` requires a repository to navigate, we only
want to advertise the command when in the context of a repository.

@jcansdale jcansdale force-pushed the feature/open-from-clipboard-url branch from eca86c0 to bec5eca Jul 13, 2018

Lazy initialize the UIContext
This broke unit tests and probably should be done on UI thread.
@grokys

This comment has been minimized.

Copy link
Contributor

grokys commented Jul 13, 2018

This was also a specific suggestion because users are now very familiar with the combined address/search field.

This could be something for the GitHubHub maybe? I don't think the GitHub pane filter would be a very obvious place to put that functionality, but it might work in the GitHubHub.

TBH, I think the discoverability of the Code context > GitHub commands is pretty horrible.

You also need to have a text editor open, if you don't you have to open a random file in order to paste the URL!

grokys and others added some commits Jul 13, 2018

@grokys

grokys approved these changes Jul 13, 2018

@meaghanlewis meaghanlewis merged commit 49ad951 into master Jul 13, 2018

2 checks passed

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

@meaghanlewis meaghanlewis deleted the feature/open-from-clipboard-url branch Jul 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.