Using builtin file & folder icons #1744

Neme12 · 2018-06-16T18:39:04Z

Before:

After:

Neme12 · 2018-06-16T18:54:30Z

src/GitHub.VisualStudio/GitHub.VisualStudio.csproj

      <DependentUpon>RepositoryRecloneView.xaml</DependentUpon>
    </Compile>
+    <Compile Include="Views\GitHubPane\DirectoryIsExpandedToImageMonikerConverter.cs" />
+    <Compile Include="Views\GitHubPane\FileNameToImageMonikerConverter.cs" />


Where would be an appropriate place for these converters? I noticed there is a folder with some converters in GitHub.UI but that project doesn't have a lot of dependencies on VS itself (which these do). Also these aren't really general purpose converters like the other ones seem to be, they're made specifically for one view. But this isn't a good place either (I assume). Where should I put them?

Personally I think that where you've put them is the best place for them: as you say they're tied to a particular view, so it makes sense to put the converters alongside. Not sure if anyone else things differently though?

Neme12 · 2018-06-16T19:00:20Z

src/GitHub.VisualStudio/Views/GitHubPane/PullRequestFilesView.xaml

-                      We need to change the color of the file icon when the file is deleted, but applying the style
-                      to OcticonImage directly borks the designer (see #1410). Work around this by applying the color
-                      to a parent control and let the foreground be inherited by the icon.
-                    -->


I noticed this used to gray out the icon if the item was deleted. This can't really be done with arbitrary colored icons. I tried replacing the color change with reduced opacity, but it didn't look very good in my opinion. And the built-in team explorer view doesn't do anything like that either (it's only the text that is grayed out, not the icon). Is it OK to remove this?

Before:

After:

(icon is the same for deleted and normal files)

Yeah that's fine with me!

Neme12 · 2018-06-16T19:08:26Z

src/GitHub.VisualStudio/Views/GitHubPane/PullRequestDetailView.xaml

+                                    xmlns:theming="clr-namespace:Microsoft.VisualStudio.PlatformUI;assembly=Microsoft.VisualStudio.Imaging"
                                    Background="{DynamicResource GitHubVsToolWindowBackground}"
                                    Foreground="{DynamicResource GitHubVsWindowText}"
+                                    theming:ImageThemingUtilities.ImageBackgroundColor="{Binding RelativeSource={RelativeSource Self}, Path=Background.Color}"


This is important to ensure that the icons have the right background in all themes. Transparent doesn't work (it seems to behave almost the same as white). This has to be the real background color, that's why it's all the way up here where the actual background is set.

If this was set to Transparent, it would look like this on a dark theme:

Correct behavior:

~~Just ran into this on the PR review authoring view. I've tried moving this property to PullRequestFilesView and it seems to work fine. What am I missing?~~

Ignore me, you're right.

Neme12 · 2018-06-16T22:06:32Z

Note that this also shows any custom icons correctly. For example after installing this extension, which adds many new file icons to solution explorer:
https://marketplace.visualstudio.com/items?itemName=MadsKristensen.FileIcons

we can see all of them in the PR view:

In this case the icon for .editorconfig is actually provided by the extension and not part of Visual Studio (the icon for .gitignore is mapped by the extension too but the image itself comes from VS in KnownMonikers).

Neme12 · 2018-06-16T23:38:53Z

I'm not sure what's failing in CI. Can someone help please? Everything is building for me locally.

grokys · 2018-06-17T09:23:43Z

@Neme12 just popping in to say thanks so much for this! This looks excellent; it's something I've been wanting to do for ages but it was just never high enough priority for me.

I think the reason that CI is failing is that CI is building on VS2015 and you're using a C#7 feature here. Up until recently VS2017 was having problems with the extension, it now works however so we're (at least I'm) hoping to move to 2017 soon.

I'll give this a proper review on Monday, but it looks great from a first glance!

Neme12 · 2018-06-17T10:34:17Z

@grokys Thanks. Please consider adding <LangVersion>6</LangVersion> to the project file. It will prevent contributors from using newer language features and set a standard for everyone, no matter which version of VS they happen to be using. On top of that, it will also prevent the IDE from offering suggestions or refactorings for features that can't be used.

Doing this is always a good idea even if you move to VS 2017 so that there's never confusion about which language version we're using - whether it be C# 6, 7.0, 7.1, 7.2 or 7.3.

edit: I made a PR: #1746

Neme12 · 2018-06-17T10:41:06Z

There's another error in CI:

error CA1812: Microsoft.Performance : 'DirectoryIsExpandedToImageMonikerConverter' is an internal class that is apparently never instantiated.

This is incorrect. The class is used from XAML :-/

From the doc: Parses a moniker string. The moniker can be of the form "{guid};id" or the name of a well-known moniker (e.g. "KnownMonikers.FolderClosed")

Neme12 · 2018-06-17T11:27:24Z

I was able to simplify this further and find the proper way to do this when I stumbled upon IVsImageService2 by looking through APIs in intellisense 😄

grokys

Thanks @Neme12, this is really excellent! Really makes for a much better experience.

Everything here looks 👍 to me: going to wait a little so other team members can take a look, but this looks good to go to me!

…cons

Neme12 · 2018-06-18T11:29:46Z

src/GitHub.VisualStudio/Views/GitHubPane/FileNameToImageMonikerConverter.cs

        {
+            // In design mode, imageService will be null
+            if (DesignerProperties.GetIsInDesignMode(new DependencyObject()))
+                return default(ImageMoniker);


Alternatively, we could just check if imageService is null and return KnownMonikers.Document as a fallback, even though it should never be null when running inside VS.

What we return doesn't matter because these icons can't be shown in the designer anyway

Neme12 · 2018-06-18T14:11:32Z

Sorry for so many merge commits. I didn't notice that you added a merge commit in my branch and just used the regular sync feature in VS, so that created another one. Now I saw this message with "This branch is out-of-date with the base branch" which I've never seen before and was curious about what it does 😄 So that's another merge commit... But interestingly, that message is still there and didn't go away. Could that be a bug?

grokys · 2018-06-18T16:53:56Z

No, it's probably just that another PR was merged in the meantime. Don't worry about the merge commits, they're pretty much a fact of life ;)

grokys · 2018-06-18T16:54:48Z

@meaghanlewis this looks good to me. If you could take a look and it looks good to you I think we should merge asap

meaghanlewis · 2018-06-18T17:34:54Z

This looks great to me! Thanks @Neme12 🎉

Neme12 · 2018-06-18T19:40:09Z

Note: When writing code, I was often wondering if there is any preferred code style for this repository that I should use. For example: would you prefer to be explicit about access modifiers (private, internal) or omit them? are braces required? can I use expression bodied methods? are there any naming conventions for const fields? should System directives come first when sorting? string.IsNullOrEmpty vs String.IsNullOrEmpty?

If there are any standard conventions for these, it would be nice to codify them inside .editorconfig so that contributors can be aware of them, obide by them if necessary and the IDE can help them do the right thing. If there are any concerns about setting a hard rule, these could be set to a "suggestion" (not a warning). The same could be done for naming styles. Even if the severity is set to none (it's not a suggestion, warning or an error if you get this wrong) it can still be helpful because the IDE will generate code based on this preferred style when using refactorings.

But given that there were no comments on the style I chose, I'm wondering if it's maybe not considered that important in this codebase and everyone is free to use the style they prefer? (or did I just get everything right? 😄) If that's the case, doing this might not bring that much value after all. I don't know the preferences, so that's why I'm asking and suggesting ideas. 🎉 What do you think? Would that be a good fit for this repo?

jcansdale · 2018-06-19T08:52:18Z

@Neme12 This would be an excellent fit for this repo! It's just a question of carving out the time. If you're up for driving it in #1750, that would be great. 😄

Could you add a list of questions that would need answering? I'm not very familiar with .editorconfig files (not sure about the rest of the team).

See #1744 (comment) for information on why this needs to be done here and not in `PullRequestFilesView`.

Using builtin file & folder icons

2f4da26

Neme12 commented Jun 16, 2018

View reviewed changes

Neme12 mentioned this pull request Jun 16, 2018

PR changes view should use builtin VS icons #1745

Closed

Removing C# 7 language features

8cf9a26

Neme12 added 2 commits June 17, 2018 12:45

Adding suppression for CA1812

054d10c

Using IVsImageService2.TryParseImageMoniker, which supports both formats

49e46af

From the doc: Parses a moniker string. The moniker can be of the form "{guid};id" or the name of a well-known moniker (e.g. "KnownMonikers.FolderClosed")

Neme12 force-pushed the icons branch from 8f8a058 to 49e46af Compare June 17, 2018 11:09

Using IVsImageService2.GetImageMonikerForFile

be54a16

grokys approved these changes Jun 18, 2018

View reviewed changes

grokys and others added 3 commits June 18, 2018 08:48

Merge branch 'master' into icons

203f329

Fixing crash in WPF designer

b2945cb

Merge branch 'icons' of https://github.com/Neme12/VisualStudio into i…

79ced6e

…cons

Neme12 commented Jun 18, 2018

View reviewed changes

Merge branch 'master' into icons

2fbc919

Merge branch 'master' into icons

debe368

meaghanlewis merged commit 0c5e175 into github:master Jun 18, 2018

Neme12 deleted the icons branch June 18, 2018 17:44

Neme12 mentioned this pull request Jun 18, 2018

Consider adding C# code style preferences to editorconfig #1750

Closed

grokys added a commit that referenced this pull request Jul 20, 2018

Fix icon background in PullRequestReviewAuthoringView.

df9cc43

See #1744 (comment) for information on why this needs to be done here and not in `PullRequestFilesView`.

grokys mentioned this pull request Jul 20, 2018

Fix icon background in PullRequestReviewAuthoringView. #1791

Merged

Using builtin file & folder icons #1744

Using builtin file & folder icons #1744

Uh oh!

Conversation

Neme12 commented Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Neme12 Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokys Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

Neme12 Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokys Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

Neme12 Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokys Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 commented Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Neme12 commented Jun 16, 2018

Uh oh!

grokys commented Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Neme12 commented Jun 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Neme12 commented Jun 17, 2018

Uh oh!

Neme12 commented Jun 17, 2018

Uh oh!

grokys left a comment

Choose a reason for hiding this comment

Uh oh!

Neme12 Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

Neme12 Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

Neme12 commented Jun 18, 2018

Uh oh!

grokys commented Jun 18, 2018

Uh oh!

grokys commented Jun 18, 2018

Uh oh!

meaghanlewis commented Jun 18, 2018

Uh oh!

Neme12 commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcansdale commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Neme12 commented Jun 16, 2018 •

edited

Loading

Neme12 Jun 16, 2018 •

edited

Loading

Neme12 Jun 16, 2018 •

edited

Loading

Neme12 Jun 16, 2018 •

edited

Loading

grokys Jul 20, 2018 •

edited

Loading

Neme12 commented Jun 16, 2018 •

edited

Loading

grokys commented Jun 17, 2018 •

edited

Loading

Neme12 commented Jun 17, 2018 •

edited

Loading

Neme12 commented Jun 18, 2018 •

edited

Loading

jcansdale commented Jun 19, 2018 •

edited

Loading