Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

[WIP] Add Pull Request filter button to Visual Studio solution explorer #1667

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

laurentkempe
Copy link

@laurentkempe laurentkempe commented May 16, 2018

See #1666

  • Add command to be able to bind a shortcut to
  • See if it is possible to use GitHub.VisualStudio.vsct guidImages pullrequest in place of the resource currently added
  • Hide filter button if the user has not checked out a pull request
  • Add tests
  • Reformat code to follow the existing code's style

image

Related

@grokys
Copy link
Contributor

grokys commented May 16, 2018

This is a great idea @laurentkempe! Please let us know if you have any questions. You can ping me in https://gitter.im/github/VisualStudio if you want to chat.

Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using Mads Kristensen's ExtensibilityTools extension:
https://marketplace.visualstudio.com/items?itemName=MadsKristensen.ExtensibilityTools

I'm seeing the following build error:
image

Does this make sense to you?

@jcansdale
Copy link
Collaborator

I've just pushed a fix so that it compiles in VS 2015. Alas we're still restricted to C# 6.0. 😭

@jcansdale
Copy link
Collaborator

@laurentkempe,

The issue above was that the IDSymbol for pullrequest wasn't defined. I've updated it to this:

    <Bitmaps>
      <Bitmap guid="guidImages" href="Resources\PullRequestFilterCommand.png" usedList="pullrequest"/>
    </Bitmaps>

    <GuidSymbol name="guidImages" value="{775aa523-6c52-4c11-9c28-823c99d15613}" >
      <IDSymbol name="pullrequest" value="1" />
    </GuidSymbol>

We should probably change to use an .imagemanifest, like in the GitHub.VisualStudio project (see GitHub.VisualStudio.imagemanifest).

@jcansdale
Copy link
Collaborator

I think this is a bug in Code Analysis: 😭

image

If I'm reading the code correctly, the result of BuildAbsolutePath is simply being used as a key. I'm going to change it to ToUpperInvariant so as not to uglify the code with CA suppression attributes.

I think there's a bug in CA where it suggests changing ToLowerInvariant
to ToUpperInvariant (I don't think it matters in this case).
Make PullRequestFilterPackageGuids a static class.
Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed the bitmap and CA issues.

We need to compare hierarchy items, which come in lower case, to pull request session files in upper case!
@laurentkempe
Copy link
Author

We should probably change to use an .imagemanifest, like in the GitHub.VisualStudio project (see GitHub.VisualStudio.imagemanifest).

I have no clue how that work @jcansdale

@jcansdale
Copy link
Collaborator

@donokuda do you think we could get a vector version of the PR filter icon? 🙏

@laurentkempe
Copy link
Author

@jcansdale I think there is one in XAML in one of the other project! I was trying to see how we could reuse that one without copying it, and also how we can use XAML in a button declared in vsct files. I don't know if that is possible, but I am quite new to all this VS development

@jcansdale
Copy link
Collaborator

jcansdale commented May 18, 2018

@laurentkempe,

I'm not very familiar with this area myself.

Here are a couple of pertinent links:
https://github.com/jcansdale/VisualStudio/blob/f17e673e42e553a380db0f9d644708c693c6428f/src/GitHub.VisualStudio/GitHub.VisualStudio.imagemanifest#L13
https://github.com/jcansdale/VisualStudio/blob/f17e673e42e553a380db0f9d644708c693c6428f/src/GitHub.VisualStudio/GitHub.VisualStudio.vsct#L348

I think it might simply be a case of copying GitHub.VisualStudio.imagemanifest and putting it next to InlineReviewsPackage.vsct (renaming it to InlineReviewsPackage.imagemanifest).

Then copying the following into InlineReviewsPackage.vsct.

    <GuidSymbol name="guidContextMenuSet" value="{31057D08-8C3C-4C5B-9F91-8682EA08EC27}">
      <IDSymbol name="idGitHubContextMenu" value="0x1000" />
      <IDSymbol name="idGitHubContextMenuGroup" value="0x1001" />
      <IDSymbol name="idGitHubContextSubMenuGroup" value="0x1002" />
      <IDSymbol name="openLinkCommand" value="0x100" />
      <IDSymbol name="copyLinkCommand" value="0x101"/>
      <IDSymbol name="goToSolutionOrPullRequestFileCommand" value="0x0102" />
      <IDSymbol name="idCreateGistCommand" value="0x0400" />
      <IDSymbol name="idBlameCommand" value="0x0500" />
    </GuidSymbol>

It you have time, I'll leave you to try it. 😄

@laurentkempe
Copy link
Author

It you have time, I'll leave you to try it.

I will try for sure, I want to be 🥇

@laurentkempe laurentkempe force-pushed the fixes/1666-filter-pr-solution-explorer branch from 53148d0 to dd860be Compare October 12, 2018 16:59
@codecov
Copy link

codecov bot commented Oct 12, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@7884364). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #1667   +/-   ##
=========================================
  Coverage          ?   40.22%           
=========================================
  Files             ?      407           
  Lines             ?    17370           
  Branches          ?     2393           
=========================================
  Hits              ?     6987           
  Misses            ?     9860           
  Partials          ?      523
Impacted Files Coverage Δ
.../GitHub.InlineReviews/PullRequestFilterProvider.cs 0% <0%> (ø)

@laurentkempe laurentkempe force-pushed the fixes/1666-filter-pr-solution-explorer branch from dd860be to e95d218 Compare October 20, 2018 08:25
@laurentkempe
Copy link
Author

I think it might simply be a case of copying GitHub.VisualStudio.imagemanifest and putting it next to InlineReviewsPackage.vsct (renaming it to InlineReviewsPackage.imagemanifest).

@jameswhite It doesn't work! I also tried to add a copy of git_pull_request.xaml and generate the imagemanifest with
Extensibility Tools for Visual Studio then use it in InlineReviewsPackage.vsct but still the same it always display the button without image. I try to believe that you can only have a png there!

Anyone would have an idea?

I spent 4h today on that and I am 💀

@laurentkempe laurentkempe force-pushed the fixes/1666-filter-pr-solution-explorer branch from e8f0e3e to d54e072 Compare October 21, 2018 12:23
Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the suggested change. 😄

src/GitHub.InlineReviews/InlineReviewsPackage.vsct Outdated Show resolved Hide resolved
Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I meant to request changes rather than approve. 😉

Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the suggested changes inline.

Co-Authored-By: laurentkempe <laurent.kempe@techheadbrothers.com>
@donokuda
Copy link
Contributor

@laurentkempe 👋I created a vector xaml of the pull request filter icon that you created in the original post:

(I named this file pull-request-filter-icon.xaml but I'll defer actual naming to y'all)

<?xml version="1.0" encoding="UTF-8"?>
<!--This file is compatible with Silverlight-->
<Canvas xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" Name="svg72" Width="48" Height="48">
  <Canvas>
    <Path Fill="#FF1B1F23" Stroke="#FFF6F6F6" Data="M14.547 13.257V23.5h-3.233V13.258L3.5 6.886V4.5h18.86v2.386l-7.813 6.37z"/>
    <Path Fill="#FFF6F6F6" Stroke="#FFF6F6F6" Data="M42.5 35.992c1.225.84 2 2.278 2 3.82 0 2.563-2 4.626-4.5 4.626s-4.5-2.063-4.5-4.626a4.63 4.63 0 0 1 2-3.82V23.33c-.016-.45-.155-.776-.47-1.086-.33-.324-.637-.465-1.03-.496h-1.5v4.86l-7.196-7.422 7.196-7.422v4.86h1.52c1.674.066 3.156.768 4.456 2.087 1.28 1.298 1.96 2.837 2.024 4.6v12.679zm-8-19.367v1H34v-1h.501zm-8 6.384V35.99a4.66 4.66 0 0 1 2 3.82c0 2.563-2 4.626-4.5 4.626s-4.5-2.063-4.5-4.626a4.63 4.63 0 0 1 2-3.82V29.1v-6.094a4.66 4.66 0 0 1-2-3.82c0-2.563 2-4.625 4.5-4.625s4.5 2.062 4.5 4.624a4.63 4.63 0 0 1-2 3.822z"/>
    <Path Fill="#FF1B1F23" Data="M42 36.265V23.312c-.06-1.608-.68-3.03-1.88-4.248-1.2-1.217-2.56-1.877-4.12-1.94h-2V13l-6 6.188 6 6.187V21.25h2c.54.04.96.227 1.38.64.42.412.6.866.62 1.422v12.953c-1.18.7-2 2.02-2 3.547 0 2.29 1.78 4.126 4 4.126 2.22 0 4-1.836 4-4.126a4.15 4.15 0 0 0-2-3.547zm-2 6.023c-1.32 0-2.4-1.135-2.4-2.475s1.1-2.476 2.4-2.476c1.3 0 2.4 1.135 2.4 2.475s-1.1 2.476-2.4 2.476zm-12-23.1c0-2.29-1.78-4.125-4-4.125-2.22 0-4 1.835-4 4.124a4.15 4.15 0 0 0 2 3.548v13.53c-1.18.7-2 2.02-2 3.547 0 2.29 1.78 4.126 4 4.126 2.22 0 4-1.836 4-4.126a4.15 4.15 0 0 0-2-3.547v-13.53c1.18-.7 2-2.02 2-3.547zm-1.6 20.625c0 1.36-1.1 2.474-2.4 2.474-1.3 0-2.4-1.134-2.4-2.474 0-1.34 1.1-2.476 2.4-2.476 1.3 0 2.4 1.135 2.4 2.475zM24 21.663c-1.32 0-2.4-1.135-2.4-2.476 0-1.34 1.1-2.475 2.4-2.475 1.3 0 2.4 1.135 2.4 2.476 0 1.34-1.1 2.475-2.4 2.475z"/>
  </Canvas>
</Canvas>

I didn't know how to implement this separate to test it out, so what I did was replace an existing xaml file and looked at the result in dark, light, and blue theme 😅:

screen shot 2018-10-22 at 10 27 55 pm

screen shot 2018-10-22 at 10 20 03 pm

screen shot 2018-10-22 at 10 19 40 pm

I think this looks good in case you wanted to apply the xaml to your pull request. Let me know if there's anything else I can do to help out. Thanks for bearing with my late delivery with the icon 😄

@donokuda donokuda removed their assignment Oct 23, 2018
@laurentkempe
Copy link
Author

laurentkempe commented Oct 26, 2018

I think this looks good in case you wanted to apply the xaml to your pull request. Let me know if there's anything else I can do to help out. Thanks for bearing with my late delivery with the icon

Thanks a lot @donokuda for the great icon, it looks awesome!

Here it is in place

image
image
image

On thing that might be improved is that the size of the filtering, and the other part of the icon do not match 100%

image

You can better see it here

devenv_2018-10-26_09-19-40

@laurentkempe laurentkempe force-pushed the fixes/1666-filter-pr-solution-explorer branch from d131c40 to 9eaeb06 Compare November 18, 2018 08:16
@laurentkempe laurentkempe force-pushed the fixes/1666-filter-pr-solution-explorer branch from 9eaeb06 to f1d3741 Compare November 1, 2019 21:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants