Merge all indentation related code into one service. #33989

CyrusNajmabadi · 2019-03-09T04:31:09Z

This removes the strange and complex split we had between SmartIndent and SmartTokenCommandHandler. For some reason, we had two entirely different integration points into VS to handle where we should indent to when 'enter' is hit. This added a lot of complexity and made it very difficult to understand what was going on.

Furthermore, this split meant that no actual features could ask the simply question: "where should new code on this line be indented to?".

Now, all logic is handled by the IIndentationService. The internal implementation of IIndentationService is still a bit complex in that it has two separate codepaths for 'indentation' versus 'token indentation'. However, at least it's all in one place now instead of being scattered all over the place.

CyrusNajmabadi · 2019-03-09T04:31:47Z

src/EditorFeatures/CSharp/Formatting/CSharpEditorFormattingService.cs

        public bool SupportsFormatOnPaste => true;
        public bool SupportsFormatSelection => true;
-        public bool SupportsFormatOnReturn => true;
+        public bool SupportsFormatOnReturn => false;


All the changes in this file come from #33983 that PR should be merged first.

CyrusNajmabadi · 2019-03-09T04:33:03Z

src/EditorFeatures/CSharp/Formatting/Indentation/SmartTokenFormatterCommandHandler.cs

-    [Order(After = PredefinedCommandHandlerNames.Rename)]
-    [Order(Before = PredefinedCommandHandlerNames.Completion)]
-    internal class SmartTokenFormatterCommandHandler :
-        AbstractSmartTokenFormatterCommandHandler


This type is completely unnecessary. I don't know why we had it. We already exposed the ISmartIndentProvider VS type to expose this functionality. So all this code was removed and integrated into IIndentationService.

CyrusNajmabadi · 2019-03-09T04:33:19Z

src/EditorFeatures/CSharp/Formatting/Indentation/WhitespaceExtensions.cs

-        }
-#endif
-    }
-}


All dead code.

CyrusNajmabadi · 2019-03-09T04:33:41Z

src/EditorFeatures/CSharp/SplitStringLiteral/SplitStringLiteralCommandHandler.StringSplitter.cs

                var newDocument = Document.WithSyntaxRoot(newRoot);

-                var indentationService = (IBlankLineIndentationService)newDocument.GetLanguageService<ISynchronousIndentationService>();
+                var indentationService = newDocument.GetLanguageService<Indentation.IIndentationService>();


this is from the #33978 PR.

why not just use IIndentationService?

src/EditorFeatures/CSharpTest/Formatting/Indentation/SmartIndenterTests.cs

CyrusNajmabadi · 2019-03-09T04:34:50Z

...ures/Core/Implementation/Formatting/Indentation/AbstractSmartTokenFormatterCommandHandler.cs

-namespace Microsoft.CodeAnalysis.Editor.Implementation.Formatting.Indentation
-{
-    internal abstract class AbstractSmartTokenFormatterCommandHandler :
-        IChainedCommandHandler<ReturnKeyCommandArgs>


completely uneeded. This just replicates the functionality that VS and the ISmartIndentProvider already take care of for us.

are you sure? I remember you and sam arguing that formatting and smart identation is so important and related to musle memory so unless we can prove it doesn't change behavior, we should try to change it. is that now changed?

I can't prove a negative. But i can provide a lot of evidence this is safe. Specifically, the other class only handled hitting enter after typing using (...). We've already discussed and designed a new approach for handling that that hte IDE is fine with. Also, we have many tests covering indentation behavior, and we added more tests during the work to remov ethe need for AbstractSmartTokenFormatterCommandHandler. These help validate that the behavior we have is the one we want.

CyrusNajmabadi · 2019-03-09T04:35:23Z

src/EditorFeatures/Core/Implementation/Formatting/Indentation/WhitespaceExtensions.cs

-{
-    internal static class WhitespaceExtensions
-    {
-        public static bool IsFirstTokenOnLine(this SyntaxToken token, ITextSnapshot snapshot)


never used anywhere in the product.

src/EditorFeatures/TestUtilities/Formatting/CoreFormatterTestsBase.cs

CyrusNajmabadi · 2019-03-09T04:37:13Z

src/Workspaces/CSharp/Portable/Indentation/CSharpIndentationService.cs

+        protected override ISmartTokenFormatter CreateSmartTokenFormatter(OptionSet optionSet, IEnumerable<AbstractFormattingRule> formattingRules, SyntaxNode root)
+        {
+            return new CSharpSmartTokenFormatter(optionSet, formattingRules, (CompilationUnitSyntax)root);
+        }


thse moved here from SmartTokenFormatCommandHandler.

heejaechang · 2019-06-12T23:23:36Z

@jinujoseph so, these seem to make subtle behavior changes on indentation. those subtle changes, I hard to imagine most of the people will ever notice but pretty sure there will be a person who cares in the world.

with many formatting, indentation, auto brace completion options, I can't know all possible behavior changes it could have.

but code clean up looks nice and behavior changes seems something I don't care much.

I guess the biggest change is before when we don't care, we returned null or nothing to indicate someone else to take care, but now, we always return something which could break some random extension somewhere in the world.

but again, seems so corner case... so I am +1 but, you probably need to aware so that if we get a bug from someone saying we broke their test or something, we can respond.

CyrusNajmabadi · 2019-06-13T03:15:55Z

@heejaechang Feel free to merge when you have a chance :)

CyrusNajmabadi · 2019-06-13T14:57:15Z

Also, @heejaechang, if you merge this, please do not squash. It will make further downstream PRs very difficult. Thanks! :)

stale

CyrusNajmabadi · 2019-06-14T16:11:35Z

@heejaechang @jinujoseph woudl love if we could move forward and merge this. There are about 2-3 more followup PRs that depend on this that i'm blocked on. Thanks!

CyrusNajmabadi · 2019-06-19T16:05:01Z

@jinujoseph can you let me know when this will get through the review process? Thanks!

CyrusNajmabadi · 2019-06-24T18:42:40Z

@jinujoseph @jasonmalinowski can you advise on what's going with this PR? Thanks!

jinujoseph · 2019-06-24T22:09:01Z

Design meeting notes

Overall feels like the right design to move towards a separate service. This could potentially break peoples existing tests, which we would suggest them to change there tests. There could be other unknown issues which we will have to address as they arise. This also needs another pair of review.

@mavasani to review
To be merged for 16.3.Preview2 ( Master should be available for 16.3 by EOW)

jinujoseph · 2019-06-24T22:09:48Z

blocked so we don't merge this in 16.3.Preview1 branch

CyrusNajmabadi · 2019-06-25T17:03:38Z

@jinujoseph why is this blocked?

CyrusNajmabadi · 2019-06-26T20:01:10Z

@jinujoseph @jasonmalinowski Where can this be merged into?

jasonmalinowski · 2019-06-27T19:08:02Z

I think it was marked while we were juggling branches -- this is now fine to merge into master. @mavasani still waiting for a review I believe?

mavasani · 2019-06-27T19:14:41Z

I had a chat with Jinu offline and I think @sharwell might want to review this one given his work in this area. He should be back on Monday to review it.

CyrusNajmabadi · 2019-07-01T21:12:42Z

I had a chat with Jinu offline and I think @sharwell might want to review this one given his work in this area. He should be back on Monday to review it.

@sharwell can you take a look please?

mavasani

LGTM based on my limited knowledge in this space.

CyrusNajmabadi · 2019-07-02T20:38:07Z

Thanks!

CyrusNajmabadi added 20 commits March 8, 2019 15:38

Move IndentationService down to Feature layer.

38f644f

MOve to workspaces

c5fa15b

Cleanup

6864d02

Fix.

fcafe82

Merge branch 'simplifyEnterHandling' into mergeIdentation

da753dc

Share more code

461f342

Remove unused methods

551f2ea

Rename method

fa2c5d6

Work

ed863d0

work

3aefaa9

move ISmartTokenFormatter down as well

5505579

Merge branch 'moveIndentationDown' into mergeIdentation

07b913b

Pass token all the way back out

88d7ba0

simplify

ca7cf85

Move code from command handler into indentation service

3b0b6b4

apply and determine indentation

c1ce379

Update tests

943afa2

correct offset

60b860e

remove old code

a226a9c

Cleanup

028dfa9

CyrusNajmabadi requested a review from a team as a code owner March 9, 2019 04:31

CyrusNajmabadi commented Mar 9, 2019

View reviewed changes

src/EditorFeatures/CSharp/Formatting/Indentation/WhitespaceExtensions.cs Outdated

}

#endif

}

}

Copy link

Member Author

CyrusNajmabadi Mar 9, 2019

Choose a reason for hiding this comment

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

All dead code.

CyrusNajmabadi commented Mar 9, 2019

View reviewed changes

src/EditorFeatures/CSharpTest/Formatting/Indentation/SmartIndenterTests.cs Show resolved Hide resolved

CyrusNajmabadi commented Mar 9, 2019

View reviewed changes

src/EditorFeatures/TestUtilities/Formatting/CoreFormatterTestsBase.cs Show resolved Hide resolved

CyrusNajmabadi commented Mar 9, 2019

View reviewed changes

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jun 12, 2019

heejaechang approved these changes Jun 12, 2019

View reviewed changes

jinujoseph changed the base branch from master to release/dev16.3-preview1 June 14, 2019 19:02

jinujoseph added this to the 16.3.P1 milestone Jun 14, 2019

jinujoseph added Blocked and removed Need Design Review The end user experience design needs to be reviewed and approved. labels Jun 24, 2019

jinujoseph modified the milestones: 16.3.P1, 16.3 Jun 24, 2019

jasonmalinowski changed the base branch from release/dev16.3-preview1 to master June 27, 2019 19:07

jinujoseph removed the Blocked label Jun 30, 2019

Merge remote-tracking branch 'upstream/master' into mergeIdentation

4745f40

mavasani approved these changes Jul 2, 2019

View reviewed changes

mavasani merged commit c52002b into dotnet:master Jul 2, 2019

CyrusNajmabadi deleted the mergeIdentation branch July 2, 2019 20:38

CyrusNajmabadi mentioned this pull request Mar 2, 2020

Simplify smart indentation implementation. #15711

Closed

Merge all indentation related code into one service. #33989

Merge all indentation related code into one service. #33989

Uh oh!

Conversation

CyrusNajmabadi commented Mar 9, 2019

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!

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!

Choose a reason for hiding this comment

Uh oh!

heejaechang commented Jun 12, 2019

Uh oh!

CyrusNajmabadi commented Jun 13, 2019

Uh oh!

CyrusNajmabadi commented Jun 13, 2019

Uh oh!

CyrusNajmabadi commented Jun 14, 2019

Uh oh!

CyrusNajmabadi commented Jun 19, 2019

Uh oh!

CyrusNajmabadi commented Jun 24, 2019

Uh oh!

jinujoseph commented Jun 24, 2019

Uh oh!

jinujoseph commented Jun 24, 2019

Uh oh!

CyrusNajmabadi commented Jun 25, 2019

Uh oh!

CyrusNajmabadi commented Jun 26, 2019

Uh oh!

jasonmalinowski commented Jun 27, 2019

Uh oh!

mavasani commented Jun 27, 2019

Uh oh!

CyrusNajmabadi commented Jul 1, 2019

Uh oh!

mavasani left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants