Add a link to Tools>Options>…>Code Style to link to EditorConfig documentation. Bug#23513 #24279

JieCarolHu · 2018-01-17T04:05:30Z

Fixes : #23513

Customer scenario

Add a link to EditorConfig documentation in Tools>Options>…>Code Style

Bugs this fixes

#23513

Workarounds, if any

Not needed

Risk

low risk because it only adds a hyper link the code style page

Performance impact

Low perf impact because no extra allocations/no complexity changes

Is this a regression from a previous update?

No

Root cause analysis

Not needed.

How was the bug found?

Not applicable

Test documentation updated?

Not needed

jinujoseph · 2018-01-17T07:28:57Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs


 namespace Microsoft.VisualStudio.LanguageServices.Implementation.Options
 {
    internal partial class GridOptionPreviewControl : AbstractOptionPageControl
    {
+        private static string useEditorConfigUrl = "https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference";


@kuhlenh should we create a fwklink instead here ?

Or an aka.ms link. Correct: no harcoding URLs though.

jinujoseph · 2018-01-17T07:29:51Z

@JieCarolHu , Thanks for the fast fix here. Can you also paste what the UI looks like

sharwell

Since this is a control/UI change, can you include a screenshot showing the result of the change? I also have a few questions in the diff.

sharwell · 2018-01-17T15:57:06Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml

@@ -5,7 +5,7 @@
    xmlns:converters="clr-namespace:Microsoft.VisualStudio.LanguageServices.Implementation.Options.Converters"
    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
-    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" 
+    xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"


💡 Try to avoid changes to unrelated lines in the file. Either of the following approaches is acceptable:

Disable the IDE feature which is causing this whitespace to be removed when the file is edited and/or saved

When committing changes to Git, commit the changes on a line-by-line basis while manually avoiding the lines which only have whitespace changes

I don't see any option in Visual Studio to disable the auto removing trailing whitespace, maybe we should build one? ;)
https://social.msdn.microsoft.com/Forums/vstudio/en-US/3c2eb7a1-3ddc-4b67-8105-ee30ab7661d2/how-to-prevent-visual-studio-from-removing-all-trailing-whitespaces?forum=vcgeneral

I figured out if I append ?w=1 at the end of the GitHub url, it does not show the whitespace change. check out this: https://github.com/dotnet/roslyn/pull/24279/files?w=1

sharwell · 2018-01-17T15:59:18Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml

@@ -35,12 +35,18 @@
            <converters:MarginConverter x:Key="MarginConverter" />
        </Grid.Resources>
        <Grid.RowDefinitions>
+            <RowDefinition Height="30" />


💡 The height for this row should (probably) be automatically determined based on the row content.

<RowDefinition Height="Auto" />

sharwell · 2018-01-17T16:01:06Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs

@@ -29,6 +40,17 @@ internal partial class GridOptionPreviewControl : AbstractOptionPageControl
            _createViewModel = createViewModel;
        }

+        private void LearnMoreHyperlink_RequestNavigate(object sender, RequestNavigateEventArgs e)
+        {
+            if (e.Uri == null)


❔ Is this possible?

could you pls elaborate?

Is it possible for e.Uri to be null? It seems like this is an unnecessary null check.

I searched through the Roslyn solution code base, and it looks like we always check if the uri is null before calling BrowserHelper.StartBrowser(uri). And I prefer to do unnecessary check than the possibility of running into NullReferenceException later..

sharwell · 2018-01-17T16:07:16Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs

        internal AbstractOptionPreviewViewModel ViewModel;
        private readonly IServiceProvider _serviceProvider;
        private readonly Func<OptionSet, IServiceProvider, AbstractOptionPreviewViewModel> _createViewModel;

+        // to leave a space between the end of sentence and "learn more" link
+        public static string CodeStylePageHeader => ServicesVSResources.Code_style_header_use_editor_config + " ";


💡 Add the space in the view, not here.

sharwell · 2018-01-17T16:12:34Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml

            <RowDefinition Height="*" />
            <RowDefinition Height="5" />
            <RowDefinition Height="*" />
        </Grid.RowDefinitions>
+        <TextBlock Text="{x:Static local:GridOptionPreviewControl.CodeStylePageHeader}" >


💭 I would have expected the text to be set using the following as a child element, before the hyperlink:

<Run Text="..."/>

❓ Does this need TextWrapping="Wrap" for cases where the dialog is not wide enough?

sharwell · 2018-01-17T16:13:17Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml

            <RowDefinition Height="*" />
            <RowDefinition Height="5" />
            <RowDefinition Height="*" />
        </Grid.RowDefinitions>
+        <TextBlock Text="{x:Static local:GridOptionPreviewControl.CodeStylePageHeader}" >
+            <Hyperlink RequestNavigate="LearnMoreHyperlink_RequestNavigate" NavigateUri="{x:Static local:GridOptionPreviewControl.CodeStylePageHeaderLearnMoreUri}">


💡 You should be able to use Margin="2,0" on either the Hyperlink or the nested TextBlock element to add the space.

Added after the CodeStylePageHeader text
Hyperlink does not support Margin; if I add Margin to TextBlock, then I can see the hyperlink underline actually starts with an empty space

sharwell · 2018-01-17T16:15:58Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs

+        // to leave a space between the end of sentence and "learn more" link
+        public static string CodeStylePageHeader => ServicesVSResources.Code_style_header_use_editor_config + " ";
+        public static string CodeStylePageHeaderLearnMoreText => ServicesVSResources.Learn_more;
+        public static Uri CodeStylePageHeaderLearnMoreUri = new Uri(useEditorConfigUrl);


💡 Place fields above properties

💡 This field can be made readonly

sharwell · 2018-01-17T16:17:21Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs

        internal AbstractOptionPreviewViewModel ViewModel;
        private readonly IServiceProvider _serviceProvider;
        private readonly Func<OptionSet, IServiceProvider, AbstractOptionPreviewViewModel> _createViewModel;

+        // to leave a space between the end of sentence and "learn more" link
+        public static string CodeStylePageHeader => ServicesVSResources.Code_style_header_use_editor_config + " ";
+        public static string CodeStylePageHeaderLearnMoreText => ServicesVSResources.Learn_more;


❓ In our normal pattern to create wrapper properties for text like this? It seems easier to simply bind the control directly to ServicesVSResources.Learn_more.

Yes, I think there was some screwy problem if you did it directly. @dpoeschl might remember?

kuhlenh · 2018-01-17T19:29:52Z

@JieCarolHu here is the fwlink: https://go.microsoft.com/fwlink/?linkid=866541

jasonmalinowski

Requesting changes to ensure the link is replaced with either a fwlink or aka.ms link.

jasonmalinowski · 2018-01-17T19:30:35Z

src/VisualStudio/Core/Def/ServicesVSResources.resx

@@ -1015,4 +1015,7 @@ I agree to all of the foregoing:</value>
  <data name="Decompiler_Legal_Notice_Title" xml:space="preserve">
    <value>Decompiler Legal Notice</value>
  </data>
-</root>
+  <data name="Code_style_header_use_editor_config" xml:space="preserve">
+    <value>The settings configured here only apply to your machine. To configure these settings to travel with your solution, use EditorConfig.</value>


".editorconfig files"? (with the dot). People who know what the file is might understand this better then, as right now it's less obvious that's the mechanism being used here.

are you talking about the text? I will wait @kuhlenh to comment on this.

Good call Jason. I think that sounds better.

jasonmalinowski · 2018-01-17T19:32:24Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs


 namespace Microsoft.VisualStudio.LanguageServices.Implementation.Options
 {
    internal partial class GridOptionPreviewControl : AbstractOptionPageControl
    {
+        private static string useEditorConfigUrl = "https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference";


Or an aka.ms link. Correct: no harcoding URLs though.

jasonmalinowski · 2018-01-17T19:32:41Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs


 namespace Microsoft.VisualStudio.LanguageServices.Implementation.Options
 {
    internal partial class GridOptionPreviewControl : AbstractOptionPageControl
    {
+        private static string useEditorConfigUrl = "https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference";


const so that way it can't be changed.

jasonmalinowski · 2018-01-17T19:33:16Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs

        internal AbstractOptionPreviewViewModel ViewModel;
        private readonly IServiceProvider _serviceProvider;
        private readonly Func<OptionSet, IServiceProvider, AbstractOptionPreviewViewModel> _createViewModel;

+        // to leave a space between the end of sentence and "learn more" link
+        public static string CodeStylePageHeader => ServicesVSResources.Code_style_header_use_editor_config + " ";
+        public static string CodeStylePageHeaderLearnMoreText => ServicesVSResources.Learn_more;


Yes, I think there was some screwy problem if you did it directly. @dpoeschl might remember?

jasonmalinowski · 2018-01-17T19:34:57Z

@JieCarolHu How does it look if we make the window smaller so it wraps?

JieCarolHu · 2018-01-18T01:12:34Z

Screenshots

sharwell

❔ Can you provide the second screenshot showing the layout of the dialog when wrapping is required to fit the first sentence?

sharwell · 2018-01-18T19:12:20Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs

 using Microsoft.CodeAnalysis.Options;
+using Microsoft.VisualStudio.LanguageServices.Implementation.Utilities;


💡 Can you make sure that the added items are all used (remove unused usings)?

sharwell · 2018-01-18T19:13:20Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs

@@ -29,6 +40,17 @@ internal partial class GridOptionPreviewControl : AbstractOptionPageControl
            _createViewModel = createViewModel;
        }

+        private void LearnMoreHyperlink_RequestNavigate(object sender, RequestNavigateEventArgs e)
+        {
+            if (e.Uri == null)


Is it possible for e.Uri to be null? It seems like this is an unnecessary null check.

sharwell · 2018-01-18T19:14:55Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml

            <RowDefinition Height="*" />
            <RowDefinition Height="5" />
            <RowDefinition Height="*" />
        </Grid.RowDefinitions>
+        <TextBlock TextWrapping="Wrap">
+            <Run Text="{x:Static local:GridOptionPreviewControl.CodeStylePageHeader}"/>
+            <Run Text=" "/>


❕ Can you get a second reviewer to confirm that this is an acceptable alternative to setting the Margin property on the Learn More text box to provide the spacing?

@jasonmalinowski could you take a look at this? Thanks

Doing a space this way is what I'd prefer actually since I think it's a better semantic representation of "what we want". Imagine you're a screen reader processing this. The space it'll know is a space and is a word break.

@jasonmalinowski Thanks, I was just asking since it differed from another instance of a similar feature in the project. 👍

@sharwell I wouldn't assume in this case that one had a reason for it.

jasonmalinowski

Looks good to me modulo the nit about const variable naming.

jasonmalinowski · 2018-01-18T20:29:42Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml

            <RowDefinition Height="*" />
            <RowDefinition Height="5" />
            <RowDefinition Height="*" />
        </Grid.RowDefinitions>
+        <TextBlock TextWrapping="Wrap">
+            <Run Text="{x:Static local:GridOptionPreviewControl.CodeStylePageHeader}"/>
+            <Run Text=" "/>


Doing a space this way is what I'd prefer actually since I think it's a better semantic representation of "what we want". Imagine you're a screen reader processing this. The space it'll know is a space and is a word break.

jasonmalinowski · 2018-01-18T20:32:55Z

src/VisualStudio/Core/Impl/Options/GridOptionPreviewControl.xaml.cs


 namespace Microsoft.VisualStudio.LanguageServices.Implementation.Options
 {
    internal partial class GridOptionPreviewControl : AbstractOptionPageControl
    {
+        private const string useEditorConfigUrl = "https://go.microsoft.com/fwlink/?linkid=866541";


Nit: const variables should be PascalCased.

jinujoseph · 2018-01-18T20:36:45Z

@JieCarolHu this needs to retarget 15.6.x branch and Ask mode template to be filled

sharwell · 2018-01-18T22:08:21Z

@jinujoseph for ask mode (now rebased onto dev15.6.x)

jinujoseph · 2018-01-18T22:10:34Z

@Pilchie for ask mode approval

Pilchie · 2018-01-18T22:28:31Z

Approved - thanks!

JieCarolHu added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 17, 2018

JieCarolHu requested a review from a team as a code owner January 17, 2018 04:05

jinujoseph reviewed Jan 17, 2018

View reviewed changes

sharwell requested changes Jan 17, 2018

View reviewed changes

jasonmalinowski requested changes Jan 17, 2018

View reviewed changes

JieCarolHu removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 18, 2018

sharwell requested a review from kuhlenh January 18, 2018 19:04

sharwell approved these changes Jan 18, 2018

View reviewed changes

kuhlenh approved these changes Jan 18, 2018

View reviewed changes

jasonmalinowski approved these changes Jan 18, 2018

View reviewed changes

JieCarolHu added 6 commits January 18, 2018 16:06

add text and hyperlink to C# code style page

619213b

move newly added text into resource file

a5110f6

address code review comments

0e386ba

address more comments

6e9e9e2

use .editorconfig files

ab205e4

remove unused usings

abb68e7

sharwell force-pushed the bug23513 branch from 5f9dfef to abb68e7 Compare January 18, 2018 22:06

sharwell changed the base branch from master to dev15.6.x January 18, 2018 22:07

sharwell added this to the 15.6 milestone Jan 18, 2018

jinujoseph approved these changes Jan 18, 2018

View reviewed changes

Pilchie added the Approved to merge label Jan 18, 2018

use PascalCase for const name

3cf6aa6

JieCarolHu merged commit 5a9c469 into dotnet:dev15.6.x Jan 18, 2018

JieCarolHu deleted the bug23513 branch January 18, 2018 23:55

		using Microsoft.CodeAnalysis.Options;
		using Microsoft.VisualStudio.LanguageServices.Implementation.Utilities;

Add a link to Tools>Options>…>Code Style to link to EditorConfig documentation. Bug#23513 #24279

Add a link to Tools>Options>…>Code Style to link to EditorConfig documentation. Bug#23513 #24279

Conversation

JieCarolHu commented Jan 17, 2018 • edited by sharwell

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinujoseph commented Jan 17, 2018 • edited

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JieCarolHu Jan 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuhlenh commented Jan 17, 2018

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski commented Jan 17, 2018

JieCarolHu commented Jan 18, 2018 • edited

sharwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinujoseph commented Jan 18, 2018

sharwell commented Jan 18, 2018

jinujoseph commented Jan 18, 2018

Pilchie commented Jan 18, 2018

JieCarolHu commented Jan 17, 2018 •

edited by sharwell

jinujoseph commented Jan 17, 2018 •

edited

JieCarolHu Jan 18, 2018 •

edited

JieCarolHu commented Jan 18, 2018 •

edited