Contiguous tabstops not working as expected (2nd selects part of 1st)

[kswedberg]

Prerequisites

• Put an X between the brackets on this line if you have done all of the following:
  • Reproduced the problem in Safe Mode: http://flight-manual.atom.io/hacking-atom/sections/debugging/#using-safe-mode
  • Followed all applicable steps in the debugging guide: http://flight-manual.atom.io/hacking-atom/sections/debugging/
  • Checked the FAQs on the message board for common solutions: https://discuss.atom.io/c/faq
  • Checked that your issue isn't already filed: https://github.com/issues?utf8=✓&q=is%3Aissue+user%3Aatom
  • Checked that there is not already an Atom package that provides the described functionality: https://atom.io/packages

Description

Backspacing on second tabstop deletes part of text in first tabstop when tabstops are contiguous.

Steps to Reproduce

1. Create a simple "foo" snippet, such as:

  "foo":
    prefix: "foo"
    body: """
    ${2:bar}${3:baz}
    """

2. Open a new file and change the syntax to the one the snippet was placed under.
3. Activate the "foo" snippet
4. Type something of 2 characters or more to replace "bar" ("xyzz" for this example)
5. Tab to "baz"

Expected behavior: [What you expect to happen]
Only "baz" should be selected for replacement

Actual behavior: [What actually happens]
"yzzbaz" is selected for replacement. Only the first character of the first tabstop replacement ("x") is left unselected.

Reproduces how often: [What percentage of the time does it reproduce?]
100%

Versions

Atom 1.17.0
MacOs Sierra 10.12.5

[savetheclocktower]

Yeah, I can reproduce this. It’ll be awfully tricky to fix.

Each tab stop is represented by a “marker.” Markers keep track of arbitrary ranges of the document and are quite good at adapting to additions, deletions, and movement, but in this case it’s doing the opposite of what we want in an ambiguous situation.

I’ll use parentheses to illustrate the invisible markers. After tab expansion, you’ve got:

(bar)(baz)

You type x and now you’ve got:

(x)(baz)

The editor knows to put x into the first marker because bar was selected, and that removed all ambiguity. But right now the cursor is between x and baz, so when you type text, it could plausibly get added to either marker. We want this…

(xyzz)(baz)

…but we’re getting this instead:

(x)(yzzbaz)

Markers are something that Atom core handles for us. I’ll see if there’s any way for us to tell it what ought to happen in this situation.

[savetheclocktower]

For reference: using exclusive: true when we create the marker solves this problem but causes 11 new failures out of the 81 test cases in the spec.

[savetheclocktower]

This discussion adds some context. Pinging @nathansobo because I’m curious if this ever came up again. I don’t see any open issues in the text-buffer repo about it, and exclusive is still an all-or-nothing thing — I can’t make the end exclusive but the beginning inclusive.

[savetheclocktower]

Thinking about this some more. When the cursor is right at the border of two tab stops (the end of one and the beginning of another) and a character is typed, which marker should “claim” the new text input?

I think the answer is “whichever tab stop is active,” as in whichever one we’ve cycled into with the Tab key. I had thought that the key to solving this was the ability to tell a marker to make its head exclusive but its tail inclusive, or vice-versa, but I don’t think that’ll solve this in the general case.

Right now, when a snippet is expanded, we place its tab stop markers once, and rely on the built-in functionality of markers to expand/move as necessary and keep track of the evolving boundaries of each tab stop. I think we need to go further than this:

• When a snippet is first expanded, and then again each time the active tab stop changes, we should create each tab stop’s display marker(s) anew. Markers for the active tab stop should get created with exclusive: false, and all other markers should get created with exclusive: true.
• If the newly-created marker is replacing an existing marker, we should use the old marker’s boundaries when creating the new one.
• We can skip re-creating a marker at any point if one already exists and we don’t need to change its exclusive setting.

In other words, if two markers have equal claim to a newly-typed character, the marker that corresponds to the active tab stop should always win out. This seems like it would solve all the cases that I can think of off the top of my head, but the next step here would be to write some new specs and make sure.

[kswedberg]

@savetheclocktower thank you so much for diagnosing the problem! I'm surprised I haven't seen more people who have run into this.

[savetheclocktower]

@kswedberg, based on the example in this ticket I made a spec:

    describe "when the snippet has two adjacent tab stops", ->
      it "ensures insertions are treated as part of the active tab stop", ->
        editor.setText('t19')
        editor.setCursorScreenPosition([0, 3])
        simulateTabKeyEvent()
        expect(editor.getText()).toBe("barbaz")
        expect(editor.getSelectedBufferRange()).toEqual [[0, 0], [0, 3]]
        editor.insertText('w')
        expect(editor.getText()).toBe('wbaz')
        editor.insertText('at')
        expect(editor.getText()).toBe('watbaz')
        simulateTabKeyEvent()
        expect(editor.getSelectedBufferRange()).toEqual [[0, 3], [0, 6]]
        editor.insertText('foo')
        expect(editor.getText()).toBe('watfoo')

The good news: I was able to make the changes I described above and get this test to pass. The bad news: I broke eight other specs.

Here’s an example of one that my strategy won’t solve:

  "has a placeholder that mirrors another tab stop's content":
    prefix: 't17'
    body: "$4console.${3:log}('${2:uh $1}', $1);$0"

When it expands, $1 is the active tab stop, and there are two cursors because of the mirrored tab stop. If I type foo, the second foo gets included in the last marker because we created it with exclusive: false. But the first foo doesn’t get included in the second-to-last marker because the cursor is at the edge of that $2 marker and we created it with exclusive: true. (I’m not sure if this makes sense. I’m partly just brain-dumping it for my future self.)

I think the next step is to cheat and see how other IDEs handle this. I’ll also look at some of the similar issues and assemble some more examples of problematic snippets. This is only a problem when two tab stops are directly adjacent to one another, so maybe we can cheat and find a way to put implied space between them that isn't actually represented in the editor.

[nathansobo]

I wonder, would being able to make individual endpoints of the marker exclusive be of help? That would take some effort to implement but I wanted to put the idea out there.

[savetheclocktower]

@nathansobo, that was originally what I was planning to ask for before I went on yesterday’s windmill tilt. I think it would probably help, yeah. I’ll try to create an issue on atom/text-buffer today.

Where I’m stuck — and will probably remain stuck until I get some inspiration or steal answers off of someone else’s test — is the step before that. Given a snippet like ${2:bar}${3:baz}, I can describe what I want to happen at each step, and which marker should receive input even in ambiguous scenarios. But I haven’t yet figured out how to describe that intuition in rules that can be written down.

When I figure that part out, my guess is that it will reveal some scenarios where we want a marker to be inclusive on one end and exclusive on the other.

My other concern is how dynamic we need the markers to be. When I draw the markers for ${2:bar}${3:baz}, can I give them rules like “$2 and $3, you're both inclusive on the right and exclusive on the left“ and have those rules be accurate until the snippet is finished? If not, how often do those rules need to change? Is it good enough to update them whenever the active tab stop changes, or do I need to micromanage them and update their exclusivity based on stuff like which direction the cursor approached from?

This doesn’t feel like it should be this much of a brain-teaser.

[kswedberg]

@savetheclocktower if you do want to look at how other IDEs handle this, Visual Studio Code seems to have figured it out—though I don't know how or where. Others, like TextMate, have, too, but VS Code's code base is probably closer to Atom's in that they're both JS derivatives, as far as I can tell: TypeScript/CoffeeScript.

[savetheclocktower]

@kswedberg Way ahead of you! 😜️ Turns out VScode does things extremely similarly to what I’ve got in mind.

They use “decoration” instead of “marker” and “stickiness” instead of “exclusive,” but they make it so that only the active tab stop’s markers grow when the user types at their edges. BUT they also have a special case for when an active placeholder is within an inactive placeholder. If I can figure this out, it‘ll solve the failure I described above — which, BTW, is the only remaining test failure to solve. (The others were regressions with transformed tab stops and I got them fixed.)

[savetheclocktower]

@nathansobo I’ve read the docs and the source code and I’m still not sure of the answer to this question: is it possible (and safe) to change a DisplayMarker’s exclusivity setting after it’s created? Right now I’m just replacing the marker with a new one with identical range and the settings I need, but that feels wasteful.

[nathansobo]

@savetheclocktower That's the only public way to change exclusivity right now. There's a private method called update on the base marker implementation that could be used to build a public setter for exclusivity if you wanted to.

[savetheclocktower]

I think I got it working! In theory!

It's a data-structure nightmare right now because of all the cross-referencing and the need to replace DisplayMarkers in-place, but give me a few days to clean it up and I think this can get solved once and for all.