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

Feature: Link from code to GitHub via context menus #191

Merged
merged 26 commits into from Feb 22, 2016

Conversation

Projects
None yet
4 participants
@shana
Collaborator

shana commented Jan 12, 2016

Fixes #106

This needs:

  • PR #187 - New menu handling system in master
  • PR #220 - Status bar notification service
  • PR #221 - Octicons update
  • Notification when link is copied on the status bar
  • Code review

shana and others added some commits Jan 7, 2016

Stub of "Get Link" feature
Quick stub of the VS file apis required for a potential "Get Link"
feature - gets the current file name and line number to build a github
url of it for sharing.
Attempt at fixing tests
Tests are failing on the buildbot and it looks like it's because one
test is clobbering data for another test. The only thing these tests
share is the global ServiceProvider set, so put these test classes in
the same collection so xunit doesn't parallelize them.

@shana shana self-assigned this Jan 13, 2016

@shana shana added the feature label Jan 13, 2016

@shana shana changed the title from Feature: Link from code to GitHub via context menus to [WIP] Feature: Link from code to GitHub via context menus Jan 13, 2016

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Feb 8, 2016

Collaborator

This is how it looks like currently.

@donokuda @Haacked Should we keep these context menu entries like this (considering we have potentially more context menus to add), or should we have a GitHub context menu entry with these as submenus?

copylink

Collaborator

shana commented Feb 8, 2016

This is how it looks like currently.

@donokuda @Haacked Should we keep these context menu entries like this (considering we have potentially more context menus to add), or should we have a GitHub context menu entry with these as submenus?

copylink

@shana shana changed the title from [WIP] Feature: Link from code to GitHub via context menus to Feature: Link from code to GitHub via context menus Feb 8, 2016

@shana shana assigned Haacked and unassigned shana Feb 8, 2016

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Feb 8, 2016

Collaborator

@Haacked This is ready for 👀. A few things in here are part of PR #220 so might want to deal with that one first.

Collaborator

shana commented Feb 8, 2016

@Haacked This is ready for 👀. A few things in here are part of PR #220 so might want to deal with that one first.

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Feb 8, 2016

Member

People gonna be upset if we add too much to the context menu. If there isn't already a grouping that makes sense, we should group our commands in a top level "GitHub" menu.

Member

Haacked commented Feb 8, 2016

People gonna be upset if we add too much to the context menu. If there isn't already a grouping that makes sense, we should group our commands in a top level "GitHub" menu.

@donokuda

This comment has been minimized.

Show comment
Hide comment
@donokuda

donokuda Feb 8, 2016

Member

People gonna be upset if we add too much to the context menu. If there isn't already a grouping that makes sense, we should group our commands in a top level "GitHub" menu.

👍 I can see us having to do this anyway as we integrate more features (such as creating a gist.)

Member

donokuda commented Feb 8, 2016

People gonna be upset if we add too much to the context menu. If there isn't already a grouping that makes sense, we should group our commands in a top level "GitHub" menu.

👍 I can see us having to do this anyway as we integrate more features (such as creating a gist.)

Show outdated Hide outdated src/GitHub.Exports/Extensions/SimpleRepositoryModelExtensions.cs
public static string CurrentSha(this ISimpleRepositoryModel repository)
{
var repo = GitService.GitServiceHelper.GetRepo(repository.LocalPath);
return repo.Commits.FirstOrDefault()?.Sha;

This comment has been minimized.

@Haacked

Haacked Feb 8, 2016

Member

Could repo be null here? Should this be repo?.Commits.FirstOrDefault()?.Sha?

@Haacked

Haacked Feb 8, 2016

Member

Could repo be null here? Should this be repo?.Commits.FirstOrDefault()?.Sha?

Show outdated Hide outdated src/GitHub.Exports/Extensions/SimpleRepositoryModelExtensions.cs
}
public static Uri GenerateUrl(this ISimpleRepositoryModel repository, string path = null,
int startLine = -1, int endLine = -1)

This comment has been minimized.

@Haacked

Haacked Feb 8, 2016

Member

I'd like to avoid optional arguments in public APIs because of potential ambiguities and versioning issues: http://haacked.com/archive/2010/08/10/versioning-issues-with-optional-arguments.aspx/

@Haacked

Haacked Feb 8, 2016

Member

I'd like to avoid optional arguments in public APIs because of potential ambiguities and versioning issues: http://haacked.com/archive/2010/08/10/versioning-issues-with-optional-arguments.aspx/

Show outdated Hide outdated src/GitHub.Exports/Extensions/SimpleRepositoryModelExtensions.cs
var commitSha = repository.CurrentSha();
var uri = GenerateUrl(cloneUrl.ToRepositoryUrl().AbsoluteUri, commitSha, path, startLine, endLine);
return new UriString(uri).ToUri();

This comment has been minimized.

@Haacked

Haacked Feb 8, 2016

Member

On line 66 you generate a URL using cloneUrl.ToRepositoryUrl().AbsoluteUri so you know this is going to be an http or https URL (that's what ToRepositoryUrl guarantees. So you could simplify the previous two lines to...

var uri = GenerateUrl(cloneUrl.ToRepositoryUrl().AbsoluteUri, commitSha, path, startLine, endLine);
return new Uri(uri);

Alternatively, because we have an implicit conversion, you could also simplify it to...

UriString uri = GenerateUrl(cloneUrl.ToRepositoryUrl().AbsoluteUri, commitSha, path, startLine, endLine);
return new uri.ToUri();

I leave it to you. There's nothing explicitly wrong with what you have here. 😄

@Haacked

Haacked Feb 8, 2016

Member

On line 66 you generate a URL using cloneUrl.ToRepositoryUrl().AbsoluteUri so you know this is going to be an http or https URL (that's what ToRepositoryUrl guarantees. So you could simplify the previous two lines to...

var uri = GenerateUrl(cloneUrl.ToRepositoryUrl().AbsoluteUri, commitSha, path, startLine, endLine);
return new Uri(uri);

Alternatively, because we have an implicit conversion, you could also simplify it to...

UriString uri = GenerateUrl(cloneUrl.ToRepositoryUrl().AbsoluteUri, commitSha, path, startLine, endLine);
return new uri.ToUri();

I leave it to you. There's nothing explicitly wrong with what you have here. 😄

Show outdated Hide outdated src/GitHub.Exports/Extensions/SimpleRepositoryModelExtensions.cs
static string GenerateUrl(string basePath, string sha, string path, int startLine = -1, int endLine = -1)
{
var sb = new StringBuilder(basePath);

This comment has been minimized.

@Haacked

Haacked Feb 8, 2016

Member

For small number of string concatenations, I usually don't choose to use StringBuilder because the overhead of it isn't paid off until you get to a certain number of concatenations.

Given this is not performance critical code (it's not called often and not in a tight loop) I think it probably doesn't matter either way.

Jon Skeet has a great blog post about the subject. http://jonskeet.uk/csharp/stringbuilder.html

@Haacked

Haacked Feb 8, 2016

Member

For small number of string concatenations, I usually don't choose to use StringBuilder because the overhead of it isn't paid off until you get to a certain number of concatenations.

Given this is not performance critical code (it's not called often and not in a tight loop) I think it probably doesn't matter either way.

Jon Skeet has a great blog post about the subject. http://jonskeet.uk/csharp/stringbuilder.html

Show outdated Hide outdated src/GitHub.VisualStudio/Helpers/ActiveDocumentSnapshot.cs
{
[Export(typeof(IActiveDocumentSnapshot))]
[PartCreationPolicy(CreationPolicy.NonShared)]
class ActiveDocumentSnapshot : IActiveDocumentSnapshot

This comment has been minimized.

@Haacked

Haacked Feb 8, 2016

Member

Can we export private classes?

@Haacked

Haacked Feb 8, 2016

Member

Can we export private classes?

This comment has been minimized.

@Haacked

Haacked Feb 9, 2016

Member

bump I'm surprised this works. Maybe I shouldn't be?

@Haacked

Haacked Feb 9, 2016

Member

bump I'm surprised this works. Maybe I shouldn't be?

This comment has been minimized.

@shana

shana Feb 10, 2016

Collaborator

It works via reflection, so it doesn't actually care about the visibility of the implementation. But it should be public, yes, for consistency.

@shana

shana Feb 10, 2016

Collaborator

It works via reflection, so it doesn't actually care about the visibility of the implementation. But it should be public, yes, for consistency.

Minor formatting change
Just want to feel like I'm contributing something. 😛
Show outdated Hide outdated src/GitHub.VisualStudio/Menus/CopyLink.cs
var link = GenerateLink();
if (link == null)
return;
Clipboard.SetText(link.AbsoluteUri);

This comment has been minimized.

@Haacked

Haacked Feb 9, 2016

Member

The documentation suggests Clipboard.SetText can only throw an ArgumentNullException. We've learned the hard way that's not true. For example, the .NET source code shows it could throw a SecurityException.

Also, this interacts with COM and what happens in COM land, doesn't stay in COM land but craps potential exceptions all over your best laid plans.

So we should probably wrap this in a try/catch so it doesn't crash the extension if adding to the Clipboard fails. In GitHub Desktop, we retry a couple times in case it's some weird intermittent failure. We don't have good data to know if it's possible that a retry would succeed though. So I'd just wrap it in a try/catch, write out a message, and the user can try again.

@Haacked

Haacked Feb 9, 2016

Member

The documentation suggests Clipboard.SetText can only throw an ArgumentNullException. We've learned the hard way that's not true. For example, the .NET source code shows it could throw a SecurityException.

Also, this interacts with COM and what happens in COM land, doesn't stay in COM land but craps potential exceptions all over your best laid plans.

So we should probably wrap this in a try/catch so it doesn't crash the extension if adding to the Clipboard fails. In GitHub Desktop, we retry a couple times in case it's some weird intermittent failure. We don't have good data to know if it's possible that a retry would succeed though. So I'd just wrap it in a try/catch, write out a message, and the user can try again.

This comment has been minimized.

@shana

shana Feb 9, 2016

Collaborator

The System.Windows Clipboard behaves differently from the System.Windows.Forms one, but yeah, it can still throw completely random exceptions whenever it feels like it. I'll follow your suggestion and print out a message if it fails.

@shana

shana Feb 9, 2016

Collaborator

The System.Windows Clipboard behaves differently from the System.Windows.Forms one, but yeah, it can still throw completely random exceptions whenever it feels like it. I'll follow your suggestion and print out a message if it fails.

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Feb 9, 2016

Member

🚒

Member

Haacked commented Feb 9, 2016

🚒

@Haacked Haacked assigned shana and unassigned Haacked Feb 9, 2016

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Feb 9, 2016

Collaborator

Oh totally! I haven't gone through the code with a formatter, probably should always do that for external contributors 😛

Collaborator

shana commented on 68aef19 Feb 9, 2016

Oh totally! I haven't gone through the code with a formatter, probably should always do that for external contributors 😛

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Feb 9, 2016

Collaborator

I've moved things to a submenu and cleaned up. The icons look like crap though, even though they're xaml icons. Feels like the xaml versions of the octicons that we have have strokes that are too wide?
screen shot 2016-02-09 at 2 04 53 pm
screen shot 2016-02-09 at 2 05 09 pm

Collaborator

shana commented Feb 9, 2016

I've moved things to a submenu and cleaned up. The icons look like crap though, even though they're xaml icons. Feels like the xaml versions of the octicons that we have have strokes that are too wide?
screen shot 2016-02-09 at 2 04 53 pm
screen shot 2016-02-09 at 2 05 09 pm

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Feb 9, 2016

Member

I think we need an :octocat: icon for the top level GitHub menu item. What do you think @donokuda and @shana?

Member

Haacked commented Feb 9, 2016

I think we need an :octocat: icon for the top level GitHub menu item. What do you think @donokuda and @shana?

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Feb 9, 2016

Collaborator

@Haacked Would love to, but unfortunately it seems VS does not support icons on menu entries that lead to submenus 😢

Collaborator

shana commented Feb 9, 2016

@Haacked Would love to, but unfortunately it seems VS does not support icons on menu entries that lead to submenus 😢

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Feb 9, 2016

Member

@Haacked Would love to, but unfortunately it seems VS does not support icons on menu entries that lead to submenus

😢 /cc @acangialosi feature request!

Member

Haacked commented Feb 9, 2016

@Haacked Would love to, but unfortunately it seems VS does not support icons on menu entries that lead to submenus

😢 /cc @acangialosi feature request!

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Feb 9, 2016

Member

@shana I just have the one question about the private class that's exported.

Member

Haacked commented Feb 9, 2016

@shana I just have the one question about the private class that's exported.

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Feb 10, 2016

Collaborator

@Haacked I've gone and 🔨 all the things you mentioned. I still need to fix the icons so they look half decent, so don't merge this quite yet.

Collaborator

shana commented Feb 10, 2016

@Haacked I've gone and 🔨 all the things you mentioned. I still need to fix the icons so they look half decent, so don't merge this quite yet.

@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Feb 10, 2016

Member

Slowly takes finger off the big green button

Member

Haacked commented Feb 10, 2016

Slowly takes finger off the big green button

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Feb 10, 2016

Collaborator

Collaborator

shana commented Feb 10, 2016

@niik

This comment has been minimized.

Show comment
Hide comment
Member

niik commented Feb 10, 2016

@shana A+

shana added some commits Feb 19, 2016

Add theming
Any xaml file that includes a SharedDictionaryManager resource
dictionary with the uri "Theme.xaml" will get a themed dictionary
included instead that tracks the VS theme and gets switched at runtime
whenever the VS theme changes.
@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Feb 19, 2016

Collaborator

@Haacked

Fixed the icon sizes. Also fixed the colors to match the colors of other context menus in VS. In the process, added a little extra code to support theming, so now we have three separate theme xaml files that define colors for the three themes VS comes with - Dark, Light and Blue.

Any xaml file that includes a SharedDictionaryManager resource dictionary with the uri "Theme.xaml" will get a themed dictionary included instead that tracks the VS theme and gets switched at runtime whenever the VS theme changes. If VS is running an unknown theme, the code detects whether it's a dark or light theme (based on brightness) and loads our dark or light theme file to match.

Test run

green for dark theme, blue for blue theme, red for light theme
runtime-theme-change

Dark

screen shot 2016-02-19 at 11 09 46 pm

Light

screen shot 2016-02-19 at 11 11 18 pm

Blue

screen shot 2016-02-19 at 11 11 04 pm

Collaborator

shana commented Feb 19, 2016

@Haacked

Fixed the icon sizes. Also fixed the colors to match the colors of other context menus in VS. In the process, added a little extra code to support theming, so now we have three separate theme xaml files that define colors for the three themes VS comes with - Dark, Light and Blue.

Any xaml file that includes a SharedDictionaryManager resource dictionary with the uri "Theme.xaml" will get a themed dictionary included instead that tracks the VS theme and gets switched at runtime whenever the VS theme changes. If VS is running an unknown theme, the code detects whether it's a dark or light theme (based on brightness) and loads our dark or light theme file to match.

Test run

green for dark theme, blue for blue theme, red for light theme
runtime-theme-change

Dark

screen shot 2016-02-19 at 11 09 46 pm

Light

screen shot 2016-02-19 at 11 11 18 pm

Blue

screen shot 2016-02-19 at 11 11 04 pm

@donokuda

This comment has been minimized.

Show comment
Hide comment
@donokuda

donokuda Feb 19, 2016

Member

Any xaml file that includes a SharedDictionaryManager resource dictionary with the uri "Theme.xaml" will get a themed dictionary included instead that tracks the VS theme and gets switched at runtime whenever the VS theme changes. If VS is running an unknown theme, the code detects whether it's a dark or light theme (based on brightness) and loads our dark or light theme file to match.

tumblr_nozh54mtna1s31h01o1_400

green for dark theme, blue for blue theme, red for light theme

Are these colors just for demonstrating how theming works?

Member

donokuda commented Feb 19, 2016

Any xaml file that includes a SharedDictionaryManager resource dictionary with the uri "Theme.xaml" will get a themed dictionary included instead that tracks the VS theme and gets switched at runtime whenever the VS theme changes. If VS is running an unknown theme, the code detects whether it's a dark or light theme (based on brightness) and loads our dark or light theme file to match.

tumblr_nozh54mtna1s31h01o1_400

green for dark theme, blue for blue theme, red for light theme

Are these colors just for demonstrating how theming works?

@shana

This comment has been minimized.

Show comment
Hide comment
@shana

shana Feb 19, 2016

Collaborator

Are these colors just for demonstrating how theming works?

@donokuda Yeah, I couldn't actually tell the difference. I'm actually using the same color for those icons on all three themes, and VS is apparently inverting the color on the dark theme when rendering the icons (got me so confused initially when I set it to white and it showed up black and vice-versa...), so I set them to RGB to make sure it was working (and recorded proof for future self)

Collaborator

shana commented Feb 19, 2016

Are these colors just for demonstrating how theming works?

@donokuda Yeah, I couldn't actually tell the difference. I'm actually using the same color for those icons on all three themes, and VS is apparently inverting the color on the dark theme when rendering the icons (got me so confused initially when I set it to white and it showed up black and vice-versa...), so I set them to RGB to make sure it was working (and recorded proof for future self)

@donokuda

This comment has been minimized.

Show comment
Hide comment
@donokuda

donokuda Feb 19, 2016

Member

👍 That's what I thought. Love this!

Member

donokuda commented Feb 19, 2016

👍 That's what I thought. Love this!

@shana shana assigned Haacked and unassigned shana Feb 20, 2016

@shana shana referenced this pull request Feb 22, 2016

Closed

Design time theme colors #224

1 of 1 task complete
@Haacked

This comment has been minimized.

Show comment
Hide comment
@Haacked

Haacked Feb 22, 2016

Member

This feature feels like magic!

Member

Haacked commented Feb 22, 2016

This feature feels like magic!

Haacked added a commit that referenced this pull request Feb 22, 2016

Merge pull request #191 from github/feature/link-to-vs
Feature: Link from code to GitHub via context menus

@Haacked Haacked merged commit cadea92 into master Feb 22, 2016

3 checks passed

GitHub CLA @shana has accepted the GitHub Contributor License Agreement.
Details
VisualStudio Build #2788170 succeeded in 90s
Details
jenkins/build_log Jenkins Build Log
Details

@Haacked Haacked deleted the feature/link-to-vs branch Feb 22, 2016

@shana shana referenced this pull request Feb 23, 2016

Merged

Design time theme colors #225

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