Add support for setting tabs/spaces preference in Tools->Options by allisonchou · Pull Request #3317 · dotnet/razor

allisonchou · 2021-03-24T08:51:23Z

Summary of the changes

Adds support for tabs/spaces user settings, i.e. when you go into Tools->Options now, you should now see a nifty new Razor (ASP.NET Core) section. This replaces the existing ASP.NET Core Razor Language Services in Tools->Options, which previously wasn't hooked up to anything.
A big part of this PR is @ajaybhargavb's work from Respect tabs vs spaces when generating code #1606, so thank you Ajay!
This PR also should go in with:
VS PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/309612 (needs to go in first before this PR)
Roslyn PR: [LSP] Respect settings passed in by LSP in format handlers roslyn#52044
ASPNetCore PR: Allow Razor generated documents to respect tabs/spaces settings aspnetcore#31211

A couple of notes about what currently doesn't work:

UPDATE: This seems to be working now. In the lower right-hand corner of the editor, the correct current TABS/SPC option is displayed. However, switching between tabs and spaces by clicking on a different option is still not currently supported, and the current option isn't "checked" like it should be. I'm not sure why, but this will probably take more time digging into the VS side to investigate. I don't think this is is a major blocker since users can just set their preference in Tools->Options, but it's something we should look into in the future. https://github.com/dotnet/aspnetcore/issues/28003 has already been created for this.
In the Tabs page of Tools->Options there's an "Indenting" section (see first screenshot). None of these options actually do anything, since Razor uses language-configuration.json to power indenting. All three radio buttons should probably be disabled, but current VS infrastructure seems to only support disabling "Smart" indent specifically, so I left all three for now. Created follow-up issue here: https://github.com/dotnet/aspnetcore/issues/31184

Fixes:
https://github.com/dotnet/aspnetcore/issues/30254
https://github.com/dotnet/aspnetcore/issues/27101
https://github.com/dotnet/aspnetcore/issues/28003

…ttings

ryanbrandenburg

Looks good overall, some questions about tests but if you're confident in the answers no need to block on it.

ryanbrandenburg · 2021-03-24T17:07:02Z

 {
    [PackageRegistration(UseManagedResourcesOnly = true)]
-    [AboutDialogInfo(PackageGuidString, "ASP.NET Core Razor Language Services", "#110", "#112", IconResourceID = "#400")]
+    [AboutDialogInfo(PackageGuidString, "Razor (ASP.NET Core)", "#110", "#112", IconResourceID = "#400")]


I assume this name is #PMApproved?

Yep. There was an email thread a few weeks back and this was the decided upon name.

ryanbrandenburg · 2021-03-24T17:09:28Z

        {
            // Arrange
-            var expectedOptions = new RazorLSPOptions(Trace.Messages, enableFormatting: false, autoClosingTags: true);
+            var expectedOptions = new RazorLSPOptions(Trace.Messages, enableFormatting: false, autoClosingTags: true, insertSpaces: true, tabSize: 4);


I expected some settings other than insertSpaces:true, tabSize: 4 to be tested here, is the actual value of the properties just not doing anything interesting at this layer?

Good point! I added some other tests in my latest push.

NTaylorMullen

Oh man this is exciting, really awesome to see this flow through! 👏

…ttings

TanayParikh

👏 👏 👏

Just a couple nits;

TanayParikh · 2021-03-25T23:50:12Z

+            {
+                // JToken.ToObject could potentially throw here if the user provides malformed options.
+                // If this occurs, catch the exception and return the default value.
+                return token.ToObject<T>();


I forget if it's possible for this to return null. If so, would:

Suggested change

return token.ToObject<T>();

return token.ToObject<T>() ?? defaultValue;

be preferable?

Hmm, the docs don't specify whether it can return null - I guess it wouldn't hurt to include just in case.

NTaylorMullen

90% there and looking awesome! One last blocking comment would be how you manage the attaching/unattaching from the ITextView state

allisonchou · 2021-03-26T00:51:11Z

+            {
+                // JToken.ToObject could potentially throw here if the user provides malformed options.
+                // If this occurs, catch the exception and return the default value.
+                return token.ToObject<T>();


Hmm, the docs don't specify whether it can return null - I guess it wouldn't hurt to include just in case.

allisonchou · 2021-03-27T05:22:24Z

@dotnet/razor-tooling @NTaylorMullen I've pushed some changes that update the behavior in the TextViewConnectionListener. Would appreciate any feedback!

Also, I mentioned in the PR description that the active spaces/tabs option (https://github.com/dotnet/aspnetcore/issues/28003) wasn't working, but it seems I was mistaken. The user just has to type a bit in the document before being able to freely toggle between tabs/spaces. (The same behavior is observed in C#.) I've updated the PR description accordingly.

NTaylorMullen

Overall looks great wooohooo!! I just had some suggestions for cleanup that might be valuable / reduce the amount of work going on in the textviewconnection listener.

NTaylorMullen · 2021-03-27T23:05:01Z

+                    // All TextViews share the same options, so we only need to listen to changes for one.
+                    _textBuffer.Properties[RazorEditorTrackedView] = textView;
+
+                    var razorEditorTextViewOptions = _editorOptionsFactory.GetOptions(textView);


Could you elaborate on why we need both the textview options here and the buffer ones above? Also, given that you only track one textview at a time, and assuming it's needed, it'll be important to have a good comment explaining why we only ever update options on one and not all

Added these comments to the code:

// We need to keep track of and update both the TextView and TextBuffer options. Updating // the TextView's options is necessary so 'SPC'/'TABS' in the bottom right corner of the // view displays the right setting. Updating the TextBuffer is necessary since it's where // LSP pulls settings from when sending us requests.

// A change in Tools->Options settings only kicks off an options changed event in the view // and not the buffer, i.e. even if we listened for TextBuffer option changes, we would never // be notified. As a workaround, we listen purely for TextView changes, and update the // TextBuffer options in the TextView listener as well.

^this latter issue was discovered via testing. I'm not sure why a Tools->Options change doesn't kick off a TextBuffer option changed event even if we listen for it, but I think this workaround works.

@gundermanc lol VS' details make me sad

NTaylorMullen · 2021-03-27T23:08:01Z

+                        var updatedTextView = _activeTextViews[0];
+                        _textBuffer.Properties[RazorEditorTrackedView] = updatedTextView;
+
+                        var updatedRazorEditorTextViewOptions = _editorOptionsFactory.GetOptions(updatedTextView);


Very interesting, so it definitely seems like the TextView option tracking is important. Will be curious to understand what this piece does. I'm sure this was a fun one to optimize against 😆

ghost · 2021-03-29T19:56:34Z

Hello @allisonchou!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

allisonchou added 16 commits March 9, 2021 20:44

Preliminary changes

e0f9c75

Further updates

c9ab4e1

resolve merge conflicts

f9c36c0

revert changes to global.json

8a080bc

Further changes (Ajay's + trying to modify OptionsMonitor)

5e66d0e

Revert

8c5786c

More changes

779a7c0

Cleanup

8c1f907

resolve merge conflict

d801b9b

Hacky approach

5c5f4d3

json updates

5fb46d4

Cleanup

2e8af59

Add options monitor file

2091aaa

Cleanup

0ae1376

Merge remote-tracking branch 'origin/main' into allichou/TabsSpacesSe…

9f9a3c5

…ttings

Cleanup

ee2cb61

allisonchou requested review from a team, NTaylorMullen, TanayParikh, ryanbrandenburg and sharwell as code owners March 24, 2021 08:51

allisonchou commented Mar 24, 2021

View reviewed changes

Comment thread src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

ryanbrandenburg approved these changes Mar 24, 2021

View reviewed changes

NTaylorMullen suggested changes Mar 24, 2021

View reviewed changes

allisonchou mentioned this pull request Mar 24, 2021

Allow Razor generated documents to respect tabs/spaces settings dotnet/aspnetcore#31211

Merged

allisonchou added 3 commits March 25, 2021 13:09

Merge remote-tracking branch 'origin/main' into allichou/TabsSpacesSe…

e261813

…ttings

Code review feedback

86e82c0

Fix redundant comment

43d8947

allisonchou requested a review from NTaylorMullen March 25, 2021 23:07

TanayParikh approved these changes Mar 25, 2021

View reviewed changes

Comment thread src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/DefaultRazorConfigurationService.cs Outdated

Comment thread ...r/src/Microsoft.AspNetCore.Razor.LanguageServer/RemoteProjectSnapshotProjectEngineFactory.cs Outdated

TanayParikh reviewed Mar 25, 2021

View reviewed changes

NTaylorMullen suggested changes Mar 26, 2021

View reviewed changes

Code review feedback pt2

0fa0197

allisonchou commented Mar 26, 2021

View reviewed changes

allisonchou requested a review from NTaylorMullen March 26, 2021 04:45

NTaylorMullen reviewed Mar 26, 2021

View reviewed changes

Comment thread .../src/Microsoft.VisualStudio.LanguageServerClient.Razor/RazorLSPTextViewConnectionListener.cs Outdated

Comment thread .../src/Microsoft.VisualStudio.LanguageServerClient.Razor/RazorLSPTextViewConnectionListener.cs Outdated

TextView updates

e31ad33

allisonchou requested a review from NTaylorMullen March 27, 2021 05:22

Cleanup

830ec16

NTaylorMullen approved these changes Mar 27, 2021

View reviewed changes

allisonchou added 2 commits March 29, 2021 12:39

Code review feedback

7249fce

Clean up record

c2e9b61

NTaylorMullen approved these changes Mar 29, 2021

View reviewed changes

Comment thread .../src/Microsoft.VisualStudio.LanguageServerClient.Razor/RazorLSPTextViewConnectionListener.cs Outdated

Comment thread .../src/Microsoft.VisualStudio.LanguageServerClient.Razor/RazorLSPTextViewConnectionListener.cs Outdated

Refactor

9f012b9

allisonchou added the auto-merge Squash merge once all PR checks are complete and reviewers have approved label Mar 29, 2021

ghost merged commit c8f02b9 into main Mar 29, 2021

ghost deleted the allichou/TabsSpacesSettings branch March 29, 2021 20:18

allisonchou mentioned this pull request May 5, 2021

Rename Basic to Visual Basic in Visual Studio options page dotnet/roslyn#53134

Merged

This was referenced Oct 1, 2021

Disable Razor indentation options in Tools->Options #4310

Closed

Respect indent size setting in Razor LSP and Tools->Options #4316

Open

This pull request was closed.

	return token.ToObject<T>();
	return token.ToObject<T>() ?? defaultValue;

Conversation

allisonchou commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the changes

Uh oh!

Uh oh!

ryanbrandenburg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TanayParikh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

allisonchou commented Mar 27, 2021

Uh oh!

NTaylorMullen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Mar 29, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

allisonchou commented Mar 24, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.