-
Notifications
You must be signed in to change notification settings - Fork 240
Handle Html indentation ourselves, rather than using the IDE formatter #12623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
a6b2bd6
71c5305
c06c22e
94d066b
d8a3fca
eb6d9a2
e19c614
e9795e9
60b6257
0b2b168
b4b9a6d
143126c
8ac743c
4b6666c
a12f688
df04f2a
7a3f323
e3ef464
61a0354
d3a6f15
385c2fe
63e4db3
e823f02
38c5bac
b57255c
419de94
b9a8fc3
ee8f1bd
e728adf
77f4e63
0047c0a
9d5cf02
ec1da85
71686f6
e4070bd
ea3f882
ce5daef
d73855c
3c3052e
227b67b
b326574
3d72258
8dfee13
7aac958
e4feaa9
794ac7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.Collections.Immutable; | ||
| using System.Linq; | ||
| using System.Threading; | ||
|
|
@@ -87,13 +88,26 @@ private async Task<ImmutableArray<TextChange>> FilterIncomingChangesAsync(Format | |
| var syntaxRoot = codeDocument.GetRequiredSyntaxRoot(); | ||
| var sourceText = codeDocument.Source.Text; | ||
| SyntaxNode? csharpSyntaxRoot = null; | ||
| ImmutableArray<int> scriptAndStyleElementBounds = default; | ||
|
|
||
| using var changesToKeep = new PooledArrayBuilder<TextChange>(capacity: changes.Length); | ||
|
|
||
| foreach (var change in changes) | ||
| { | ||
| // We don't keep changes that start inside of a razor comment block. | ||
| var node = syntaxRoot.FindInnermostNode(change.Span.Start); | ||
|
|
||
| // We don't keep indentation changes that aren't in a script or style block | ||
| // As a quick check, we only care about dropping edits that affect indentation - ie, are before the first | ||
| // whitespace char on a line | ||
| var line = sourceText.Lines.GetLineFromPosition(change.Span.Start); | ||
| if (change.Span.Start < line.GetFirstNonWhitespacePosition() && | ||
| !ChangeIsInsideScriptOrStyleElement(change)) | ||
| { | ||
| context.Logger?.LogMessage($"Dropping change {change} because it's not in a script or style block"); | ||
| continue; | ||
| } | ||
|
|
||
| // We don't keep changes that start inside of a razor comment block. | ||
|
davidwengier marked this conversation as resolved.
|
||
| var comment = node?.FirstAncestorOrSelf<RazorCommentBlockSyntax>(); | ||
| if (comment is not null && change.Span.Start > comment.SpanStart) | ||
| { | ||
|
|
@@ -190,6 +204,60 @@ async Task<bool> ChangeIsInStringLiteralAsync(FormattingContext context, RazorCS | |
|
|
||
| return false; | ||
| } | ||
|
|
||
| bool ChangeIsInsideScriptOrStyleElement(TextChange change) | ||
| { | ||
| // Rather than ascend up the tree for every change, and look for script/style elements, we build up an index | ||
| // first and just reuse it for every subsequent edit. | ||
| InitializeElementBoundsArray(); | ||
|
|
||
| // If there aren't any elements we're interested in, we don't need to do anything | ||
| if (scriptAndStyleElementBounds.Length == 0) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| var index = scriptAndStyleElementBounds.BinarySearch(change.Span.Start); | ||
|
|
||
| // If we got a hit, then we're on the tag itself, which is not inside the tag | ||
| if (index >= 0) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // If we don't get a hit, the complement of the result is the index to the next largest item in the array. Since we add | ||
| // things to the array in pairs, we know that if that index is even, we're outside an element, so we want to ignore identation. | ||
| // This only works if the array is ordered, but we can assume it is because we built it in order, and it makes no sense to nest | ||
| // script tags within style tags, or vice versa. | ||
| index = ~index; | ||
| if (index % 2 == 0) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| void InitializeElementBoundsArray() | ||
| { | ||
| if (!scriptAndStyleElementBounds.IsDefault) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| using var boundsBuilder = new PooledArrayBuilder<int>(); | ||
|
|
||
| foreach (var element in syntaxRoot.DescendantNodes(static node => node is RazorDocumentSyntax or MarkupBlockSyntax or MarkupElementSyntax or CSharpCodeBlockSyntax).OfType<MarkupElementSyntax>()) | ||
|
davidwengier marked this conversation as resolved.
Outdated
|
||
| { | ||
| if (RazorSyntaxFacts.IsScriptOrStyleBlock(element)) | ||
| { | ||
| boundsBuilder.Add(element.SpanStart); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly never thought of that. I guess it would work though, if the binary search fails you know you're not in a tag? Not sure it's worth changing, but I guess I'm open to it if you think this way is confusing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, if binary search fails, not in a matching span. I think it would be a bit easier to understand, but it's not like this is that complicated anyways.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Played around with this, and yeah I like it much better. Some interesting edges at trying to get the span of just the contents of the tag, because the compiler can be annoying with whitespace, but definitely easier to understand a complicated span calculation and simple search, than the other way around. Also added direct tests |
||
| boundsBuilder.Add(element.Span.End); | ||
| } | ||
| } | ||
|
|
||
| scriptAndStyleElementBounds = boundsBuilder.ToImmutableAndClear(); | ||
| } | ||
| } | ||
|
|
||
| internal TestAccessor GetTestAccessor() => new(this); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.