Fix incorrect multiline indentation inserting non-snippet text

[zardoy]

I added failing tests to illustrate the problem.

I tried different approaches, and it seems only insertText of completion and editor.insertText does whitespace adjusting in VSCode. First one causes editor scroll jump higher, and a second one messes up with undo stack. So I tried to do whitespace normalization manually, however:

1. It doesn't seem to be logical that we first remove all whitespace characters (adjustMultilineIndentation) and then add them back (code that adds this pr).
2. There is an issue with .not:

    if (
        a && (b &&
        a
        .a
        .b).not
    )  {}

[codecov[bot]]

Codecov Report

Base: 97.84% // Head: 97.81% // Decreases project coverage by -0.03% :warning:

Coverage data is based on head (b04272e) compared to base (b83e0d2). Patch coverage: 98.79% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #83      +/-   ##
===========================================
- Coverage    97.84%   97.81%   -0.04%     
===========================================
  Files           21       21              
  Lines         1209     1236      +27     
  Branches       225      226       +1     
===========================================
+ Hits          1183     1209      +26     
- Misses          25       26       +1     
  Partials         1        1              

Impacted Files	Coverage Δ
src/completionItemBuilder.ts	95.83% <93.75%> (-0.57%)	:arrow_down:
src/postfixCompletionProvider.ts	93.46% <100.00%> (+0.17%)	:arrow_up:
src/templates/baseTemplates.ts	100.00% <100.00%> (ø)
src/templates/castTemplates.ts	100.00% <100.00%> (ø)
src/templates/consoleTemplates.ts	100.00% <100.00%> (ø)
src/templates/customTemplate.ts	100.00% <100.00%> (ø)
src/templates/forTemplates.ts	100.00% <100.00%> (ø)
src/templates/ifTemplates.ts	100.00% <100.00%> (ø)
src/templates/newTemplate.ts	100.00% <100.00%> (ø)
src/templates/notTemplate.ts	95.38% <100.00%> (+0.07%)	:arrow_up:
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[zardoy]

I also checked that my proposed workaround for snippets in https://github.com/ipatalas/vscode-postfix-ts/pull/73#issuecomment-1249724849 doesn't do whitespace adjustment as well.

Just leaving a reference: https://github.com/microsoft/vscode/blob/058e70bff799899843e110b8d263788dc3aa83be/src/vs/editor/contrib/snippet/browser/snippetSession.ts#L388 it seems to be straightforward

[zardoy]

@ipatalas Sorry I was completely killed last evening and as I see didn't describe things clearly here.

So with https://github.com/ipatalas/vscode-postfix-ts/pull/73 I broke inserting text for non-snippet templates (like console or return) when node had leading whitespace characters. As I described in the body, there is no reliable way to reuse this logic from vscode, but we can do it manually since its not complicated.

I finally managed to provide my final solution to this. The only issue is with .not template:

tl;dr I do not understand why https://github.com/ipatalas/vscode-postfix-ts/blob/eb7f989d7c3b9f5d2641f75007a29367cfd3ff98/src/utils/invert-expression.ts#L36 this is being called on text that is returned only in some cases. Why we don't normalize final text result?

Also let me know what you think about it, as it touches every template.

[ipatalas]

Hi, sorry, I already had a very simple solution for that few hours ago but had to go offline :) Anyway I think that fixes the problem: https://github.com/ipatalas/vscode-postfix-ts/commit/c66c8f6213244cd4b067bc8bb6f7b5f6ed48037f All tests are green and it's one liner fix :D The thing is that I was already trying to fix whitespace problems with that adjustMultilineIndentation function. For snippet strings vscode does that on its own as you mentioned so now I just turned off this adjustMultilineIndentation for non-snippet completions and apparently it works quite well.

It has small issue with your .not example but this might be related to what you discovered in that invert-expression.ts. I don't remember why it's not fixing the output. Perhaps at the beginning it was working fine, the code there was less complex and then it grew but this stayed because nobody complained ;)

[ipatalas]

Kind of fixed .not partially:

The other alternative doesn't work perfectly (adds one extra new line which shouldn't be there) but maybe I'll find a way. Actually this fix means removing some code - I like that :D Perhaps VS Code API has changed many things since this adjustMultilineIndentation was first introduced. It looks like it's no longer needed in most places.

[zardoy]

c66c8f6

Yeah, you might noticed that this PR has a bit more code changes, because I'm replicating VSCode's normalization behavior (that's why I'm wrapping output result, not code). Sorry if I overengineered this, didn't see 1 line solution here :)

All tests are green and it's one liner fix

Sorry, but there are still cases that it doesn't fix:

{
    "name": "custom",
    "body": "1\n\t1\n{{expr}}",
    "when": [
        "identifier",
        "expression",
        "function-call"
    ]
},

Or even better, without {{expr}}: 1\n\t1\n1

Examples:

    a
    .b
    .c{custom}

Correct (just add $0 to the end):

    1
		1
	a
	.b
	.c

Incorrect:

	1
	1
a
	.b
	.c

[ipatalas]

It's not a final solution, i didn't push last commit because I'm still trying to figure this out. Anyway I'm advocating this solution to avoid having two places that adjust whitespaces. I'd rather remove the old solution (which I sort of did) and then start over with the fixes. Otherwise it will be hard to maintain if two functions are doing more or less the same in two different places.

[ipatalas]

All right, fixed that first problem:

The template is clearly reversible for both alternatives with no side effects. It still won't work with your custom template example, it's another story because the template replacement is multi-line and contains whitespaces. That will be next step where this PR's code might come handy. I reviewed that quickly and the idea itself looks pretty good, I had the same idea with IndentInfo object to pass additional information.

[ipatalas]

Ok, pushed the changes and now .not should work much better with those whitespaces. Be careful when resolving conflicts as we both did changes in the same lines. I'll try to review and test this PR tomorrow and then it would also be done for custom templates 👍🏻

[zardoy]

Sorry for the delay, I somehow missed the fact that pr with .not template was merged. I also added a multiline test with custom template that I mentioned above. Let me know what you think.

[zardoy]

@ipatalas Hey! It'd be great if you published this fix on marketplace :)

By the way, I'm just curious do you personally use this extension, or you moved away from TS development ?

[ipatalas]

@ipatalas Hey! It'd be great if you published this fix on marketplace :)

Hey, sorry, somehow thought I did that already. Anyway, just published.

By the way, I'm just curious do you personally use this extension, or you moved away from TS development ?

Unfortunately not at the moment. I used to be a fullstack and then I was using it extensively in day to day work but currently I do almost only BE at my current project hence I don't have a need to use it. Of course I have it installed and ready to be used whenever I have anything JS/TS related to do but it happens only once or twice a month.