Fix cursor offset tracking #4397

ds300 · 2018-04-29T21:15:07Z

Howdy! 👋 😄

This PR is fixing a few things related to the cursorOffset option:

Fix two bugs that were present before
- If the cursor appeared before any ast nodes, it would error. (at least in TypeScript)
- The cursor tracking was not accounting for comments preceding a node
Improve tracking with a more nuanced approach (fixes --cursor-offset output is wrong sometimes #1981)
Allow cursorOffset to be used with rangeStart and rangeEnd (fixes Allow cursorOffset to be used with rangeStart and rangeEnd #1869)

I'll explain how I did number 2, to help you read this PR.

How cursor offset was tracked before, in broad terms:

find the 'cursor node', i.e. the smallest AST node which encompasses the cursor.
calculate the relative cursor offset from the start of that node
track that node until the final output text
calculate the final cursorOffset as being the start position of the cursor node in the final text + the relative offset calculated in step 2.

This was producing bad results because if the text between the start of the AST node and the cursor would change (e.g. whitespace being inserted or deleted) then the offset calculated in step 2 is useless and should be ignored.

The approach I take in this PR is to use that relative offset as long as the text didn't change. Otherwise, I calculate a character diff between the input and output text for the cursor node, and then figure out where the best place for the cursor would be from that diff.

In order to do that, i had to "wrap" the printed version of the cursor node using two 'cursor placeholder's, one at either end. Previously there was only one inserted at the beginning.

Let me know if you have any more questions.

Thanks for the amazing work on Prettier. I can't live without it. 🙇

j-f1

Awesome!

j-f1 · 2018-04-29T21:40:46Z

tests/cursor/jsfmt.spec.js

+
+
+  const z = 9
+`)


What about

const y = 5 <|> const z = 9

IMO that should either put it on the blank line between the two or right after the 5.

Yeah it puts it on the blank line between the two. I added this test case just now 👍

duailibe · 2018-04-30T12:07:08Z

src/main/core.js

+    // diff old and new cursor node texts, with a special cursor
+    // symbol inserted to find out where it moves to
+
+    const CURSOR = Symbol("cursor");


I think we can move this to the file top level (together with UTF8BOM)

duailibe · 2018-04-30T12:08:19Z

src/main/core.js

+    for (const entry of cursorNodeDiff) {
+      if (entry.removed) {
+        if (entry.value.indexOf(CURSOR) > -1) {
+          break;


Maybe?

if (entry.removed && entry.value.indexOf(CURSOR) > -1) {

It's not the same thing, because there's an else clause. Would need to change the else to else if (!entry.removed) in order for that to work, which IMO is a bit more confusing. But happy to change it if you prefer.

Oops sorry! No need to change

duailibe · 2018-04-30T12:14:00Z

src/main/core.js

-  return text.slice(0, rangeStart) + rangeTrimmed + text.slice(rangeEnd);
+  return {
+    formatted: formatted,
+    cursorOffset: cursorOffset


return { formatted, cursorOffset }

=)

duailibe · 2018-04-30T12:18:10Z

src/main/core.js

+    result = Object.assign({}, result, {
+      formatted: trimmed + opts.newLine,
+      cursorNodeStart
+    });


You can change the result object here

Removing result = and {}, should have that effect.

duailibe · 2018-04-30T12:20:30Z

src/main/core.js

+      } else {
+        i += entry.count;
+      }
+    }


let cursorOffset = newCursorNodeStart; for (const entry of cursorNodeDiff) { /* code */ cursorOffset += entry.count; } return { formatted: result.formatted, cursorOffset };

duailibe · 2018-04-30T12:22:44Z

tests/cursor/__snapshots__/jsfmt.spec.js.snap

+  "original": "return        <|> 15",
+}
+`;
+


I think we can improve these snapshots, see below

duailibe · 2018-04-30T13:22:27Z

I pushed a change to use our current infrastructure for tests, it's been working really well.. just create files in tests/cursor/, run tests and it'll save snapshots with only the code. Objects like { original: "", formated: "" } in snapshots have too much noise

ds300 · 2018-04-30T14:03:33Z

Cool stuff with the snapshots! I'll try to get around to those small changes later today.

ds300 · 2018-04-30T18:08:43Z

All done @duailibe !

duailibe · 2018-04-30T18:21:16Z

@ds300 Looks good!

I forgot to mention I only moved some of the tests to snapshots to show how to do it. What do you think making them all like that? Just put each test in a new file (don't worry about having too many) :)

ds300 · 2018-04-30T22:42:32Z

Haha, I wasn't sure whether you'd finish. I did the rest just now. Cheers!

duailibe · 2018-05-01T12:43:13Z

Thank you!!! 💯 👏 🎉

vjeux

Thanks for fixing all those issues! I believed that cursor preservation was going to be critical and required to have a good format on save experience but it turned out I was wrong, people used it without it. But this is something that I think will provide a better user experience nonetheless!

vjeux · 2018-05-01T12:54:02Z

tests/cursor/__snapshots__/jsfmt.spec.js.snap

+
+    thisWillBeFormatted(2, 3);
+
+    thisWontBeFormatted  (2, 9<|>0  ,)


Those snapshot formats are really cool, super easy to understand what’s going on!

ds300 · 2018-05-02T23:13:32Z

I forgot to say that the reason I was working on this is that I was building a little toy text editor to see what it would feel like if prettier's formatting happened with a smooth animated transition. Turns out it feels great — https://code-in-motion.herokuapp.com/

vjeux · 2018-05-02T23:16:03Z

This sounds awesome!

I’m currently in vacation with just my iphone and it doesn’t seem to be working. I’d love if you could make a small video :)

ds300 · 2018-05-02T23:29:32Z

sure, here's a small contrived example

vjeux · 2018-05-02T23:36:19Z

That looks slick! Do you do it “on save” or are you able to do it on arbitrary keystrokes that would lead to a code that actually parses?

ds300 · 2018-05-02T23:41:24Z

just on save so far. I tried to do it after every keystroke but it doesn't work because sometimes prettier deletes things you type like commas :) so to work automatically it would need some basic heuristics about when to format and when not to format.

But also triggering it manually gives this nice cathartic feeling, for me at least.

David Sheldrick added 4 commits April 29, 2018 20:09

handle cusor position relative to comments correctly

90ac3a5

use more robust method of cursor offset tracking

2a8742e

add support for cursorOffset with rangeStart/End

5369b03

add tests for cursorOffset

0962aab

j-f1 reviewed Apr 29, 2018

View reviewed changes

David Sheldrick added 7 commits April 29, 2018 22:43

use old JS syntax for node4

d4fad0e

add another cursor offset test case

a6097cb

remove trailing whitespace in cursor tests

7f51c6a

fix null findNodeAtOffset handling

5fe603e

remove dead code

d6ffb52

add more test cases

2659ad6

use indexOf for node4

d7f54bf

ds300 force-pushed the fix/cursor-everything branch from b57f553 to d7f54bf Compare April 29, 2018 22:50

David Sheldrick and others added 3 commits April 30, 2018 08:31

revert comments.js

11ceb5b

add more comment tests

59c35aa

change tests infra

e020418

duailibe reviewed Apr 30, 2018

View reviewed changes

duailibe and others added 5 commits April 30, 2018 11:53

Fix AST_COMPARE

075f472

move CURSOR to top level of file

96ed8c2

mutate result

3b2475c

use es6 property shorthand

ffb4a13

simplify diff offset calculation

bc5f85c

put remaining test cases in individual files

81c9c44

duailibe approved these changes May 1, 2018

View reviewed changes

duailibe merged commit d77f5e9 into prettier:master May 1, 2018

vjeux reviewed May 1, 2018

View reviewed changes

lipis added this to the 1.13 milestone May 9, 2018

duailibe mentioned this pull request May 28, 2018

cursorOffset not maintained with comment #4584

Open

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Aug 7, 2018

lock bot locked as resolved and limited conversation to collaborators Aug 7, 2018

Uh oh!

Fix cursor offset tracking #4397

Fix cursor offset tracking #4397

Uh oh!

Conversation

ds300 commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-f1 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

duailibe Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

duailibe commented Apr 30, 2018

Uh oh!

ds300 commented Apr 30, 2018

Uh oh!

ds300 commented Apr 30, 2018

Uh oh!

duailibe commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ds300 commented Apr 30, 2018

Uh oh!

duailibe commented May 1, 2018

Uh oh!

vjeux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ds300 commented May 2, 2018

Uh oh!

vjeux commented May 2, 2018

Uh oh!

ds300 commented May 2, 2018

Uh oh!

vjeux commented May 2, 2018

Uh oh!

ds300 commented May 2, 2018

Uh oh!

Uh oh!

ds300 commented Apr 29, 2018 •

edited

Loading

duailibe Apr 30, 2018 •

edited

Loading

duailibe commented Apr 30, 2018 •

edited

Loading