Add support for simple regex transformations in tab stops. #260

savetheclocktower · 2017-12-18T22:25:36Z

Note: this is my first deep-dive into adding an Atom feature and I’m happy to get pushback on code formatting, architectural choices, and the like.

Description of the Change

This implements a slimmed-down version of snippet transformations. TextMate’s version of this feature is documented here.

For example, this snippet:

<${1:p}></${1/[ ]+.*$//}>

can be used like this:

Design

I went through a bunch of possible ways to design this. Here’s what I landed on:

Update the pegjs file so that we can parse the new syntax.
Introduce the concept of an Insertion, which belongs to a tab stop and can either mirror or transform its input.
Upon expanding a snippet or moving to a new tab stop, we check if the active tab stop has at least one transformation. If so, we observe the buffer so that every transactional update that happens while that tab stop is active (except for the transactions we initiate) results in a recomputation of the transform output.
For undo and redo to work the way we want, we have to take the changes that occur as a result of transformations and fold them into the change that kicked off the transformation in the first place. And we need to spy on the history provider so that we can skip trying to reapply transformations upon undo or redo.

Worth noting

Mirrored tab stops have always been treated like multiple selections…

…and that doesn’t change. But transformed tab stops are not treated like selections, because I think a user wouldn’t reasonably expect the same input to produce different output when applied to multiple selections.

Transformed tab stops cannot have placeholders; the first transformation gets applied as soon as the snippet is expanded, so any placeholder value would likely be lost immediately.

What is and isn't included

As explained in this comment, I wanted to go minimal here. The find-and-replace syntax roughly corresponds to the two arguments you'd pass to String#replace (the regex is automatically given the global flag). There's one extra feature that I brought over from TextMate format strings: the ability to control case through the \u, \l, \U, \L, and \E flags:

\u and \l will force the next character to uppercase or lowercase, respectively.
\U and \L will force the rest of the string to uppercase or lowercase, respectively (or until we encounter a different case directive or \E).
\E stops the effects of \U and \L.

There are lots of other features of TextMate format strings, but a lot of them are obscure. I went with this small extension because it's quite useful and is also supported by Sublime Text's snippet transformation syntax. Other features, like conditional insertions, could be added rather easily in the future just by updating the parser and the Insertion class.

Alternate Designs

I don’t like how a snippet expansion needs to listen to TextBuffer changes in order to do transformations, but I didn’t see another way. An asynchronous listener doesn’t act quickly enough. But we do listen at the transaction level (TextBuffer#onDidChangeText) rather than the keystroke level (TextEditor#onDidChange).

If there is a simpler way to do this, I’m all for it.

Benefits

Makes snippets far more useful. There are a couple more examples in the (duplicate) ticket that I opened.

Possible Drawbacks

Not sure. Increased code complexity, perhaps?

This PR doesn't change any existing behavior — barely any of the existing tests even needed to be touched — and the theoretical performance penalty of observing TextBuffer transactions should only be encountered when using a snippet with a transformation; for existing use cases there should be no performance regression.

Applicable Issues

Ticket #40 contains the original request.

…so we can consolidate history entries across multiple expansions. Add tests.

nathansobo

Aside from some minor changes, this is looking really excellent. Solid test coverage, well thought out design, a great PR description, and a super substantial contribution. This PR was a pleasure to review.

I plan on testing this out tomorrow morning. @ungb or @Ben3eeE, either of your help breaking this would be appreciated.

nathansobo · 2018-01-03T23:59:23Z

spec/snippets-spec.coffee

@@ -608,6 +634,7 @@ describe "Snippets extension", ->
        editor.insertText('t9b')
        simulateTabKeyEvent()
        editor.getCursors()[0].destroy()
+        buf = editor.getCursorBufferPosition()


Is this needed?

nathansobo · 2018-01-04T00:02:37Z

lib/tab-stop-list.js

@@ -0,0 +1,46 @@
+/** @babel */


It would be nice to avoid using Babel on new files. Everything except the fancy import syntax works fine.

Cool, was unclear on what the best practice was for new files.

nathansobo · 2018-01-04T00:11:40Z

lib/snippets.coffee

+
+    # Create a checkpoint here to consolidate all the changes we just made into
+    # the transaction that prompted them.
+    @makeCheckpoint(editor)


If I'm reading this right, you're doing this to group any transformations with a previous transaction because the transaction is already closed by the time you receive the event. But I'm confused as to how the initial checkpoint preceding any changes gets populated.

Incidentally, in 1.25 we're adding a groupLastChanges method to the buffer that lets you combine the last two transactions on the undo stack, which might make this easier. But you can't really use it until Atom 1.25 hits stable without breaking this package's tests.

Working off of recollection here, but I think this is the deal:

The initial snippet expansion is easy because it's invoking a command. Everything that we do in response to that command is automatically atomic because of the TextEditor#transact call.

The checkpoint logic is needed only for typing that happens while a snippet is active but not in response to a command invocation. Consider: If I have a snippet like $1\n${1/./\\u$1} (i.e., transforming what I type to all-caps on the following line):

I type its abbreviation and expand the snippet.

I type a.

Atom inserts A on the following line.

We're observing text editor changes, so we act in between steps 2 and 3, but we have to bundle the changes of steps 2 and 3 into one atom — i.e., we have to create a transaction that includes something that already happened. The way I arrived at to do this was to make the checkpoint after the initial snippet expansion and then make a new one each time we did step 3. And right before I make a new checkpoint I bundle everything together that happened since the last checkpoint.

This took a lot of time to reason through. I wrote a lot of tests around this precisely because I suspected I didn't understand it correctly.

It does sound like groupLastChanges would make this easier — it would theoretically allow me to toss out checkpoints altogether and duct-tape the changes i'm making with the user action that prompted them.

After re-reading your question, I'm pretty sure all I did was restate the part that you already understood. The shorter answer to your question is “I don't know.” I’ll investigate when I get a chance.

The way I arrived at to do this was to make the checkpoint after the initial snippet expansion and then make a new one each time we did step 3

Actually I think this answered my question perfectly.

Well, after my long comment I skimmed the source again and can't 100% guarantee that the code does what I claimed it did in the part you quoted. 🤷‍♂️ I can't find the part where I make the initial checkpoint; the makeCheckpoint method seems like it only gets called after we set up the listener, not on initial snippet expansion.

nathansobo · 2018-01-04T00:13:39Z

lib/snippets.coffee

@@ -298,6 +299,13 @@ module.exports =
  expandSnippetsUnderCursors: (editor) ->
    return false unless snippet = @snippetToExpandUnderCursor(editor)

+    @getStore(editor).observeHistory({


We should really supply a flag in the change event indicating when the change is coming from an undo or redo. If you'd have any interest in adding that, I'd be happy to shepherd it. Then we could simplify this.

Yeah, that’s certainly a better idea. I’ll investigate.

nathansobo · 2018-01-04T00:15:17Z

spec/insertion-spec.js

+const range = new Range(0, 0);
+
+describe('Insertion', () => {
+


Super minor, but noticing some extra newlines in the file such as this one.

nathansobo · 2018-01-04T20:03:28Z

lib/editor-store.coffee

@@ -0,0 +1,63 @@
+SnippetHistoryProvider = require './snippet-history-provider'


Would you be cool doing this file in ordinary JS?

I went ahead and made the changes myself to save you the trouble since it's been a while since you opened this.

nathansobo · 2018-01-04T21:54:48Z

Published as snippets@1.2.0. Thanks for the great work adding this. You combined a bunch of APIs in a pretty complex piece of code to add a really cool feature. I've gone ahead and given you commit bit on this repository ⚡️.

savetheclocktower · 2018-01-04T21:56:29Z

Thanks! I'll revisit this when 1.25 is out, and the undo/redo thing will be on my radar as well.

nobleach · 2018-06-01T18:16:28Z

Forgive my misunderstanding but, is this actually available in non-beta releases? I see it's published in snippets@1.2.0 (current is 1.3.3).

savetheclocktower · 2018-06-01T18:32:51Z

Yeah, it went out with Atom 1.25.0.

nobleach · 2018-06-01T18:37:29Z

That's great news! Looks fantastic. I'll dig around in the source a bit to see how to use it. Thanks!

Andrew Dupont and others added 10 commits December 18, 2017 11:04

Refactor to support the idea of “transformed” tab stop insertions.

49d8456

Tweaks.

3f79087

Lots of fixes. Finally tamed undo/redo, I think.

57cafea

Move history management up one level…

4ccec2e

…so we can consolidate history entries across multiple expansions. Add tests.

Work in progress.

31d1041

Fix failing tests.

7b91b5c

Fix linting errors.

ef9d5c5

More linting fixes.

0210965

Clean up the PEG file.

765f64a

Fix parsing of escaped forward slash in transformation pattern.

c60f546

savetheclocktower mentioned this pull request Dec 22, 2017

Regex Support #40

Closed

Linting.

ee659fd

nathansobo self-assigned this Jan 3, 2018

nathansobo previously requested changes Jan 4, 2018

View reviewed changes

nathansobo reviewed Jan 4, 2018

View reviewed changes

Nathan Sobo added 9 commits January 4, 2018 13:34

Convert editor-store to JS

9dddeb6

Remove Bebel dependency in insertion.js and convert to standard style

f717db5

Remove Babel dep from insertion-spec and convert to standard style

66411d8

Remove decaffeinate comment

02cfc5e

Remove Babel dependency and convert to standard code style

887db8c

Remove Babel dependency and convert to standard code style

7a5dffb

Remove Babel dependency and convert to standard code style

4bb8986

Clean up

f094cb2

Refer to TM snippet syntax docs and note missing features

21fe733

nathansobo merged commit b7b6200 into atom:master Jan 4, 2018

Ingramz mentioned this pull request Jan 4, 2018

Undoing snippets leaks snippet tab stops at ends #261

Closed

aysner mentioned this pull request Feb 17, 2018

Feature request: Convert Case #170

Closed

savetheclocktower mentioned this pull request Apr 9, 2018

Fix incorrect parsing of placeholders that point to other tab stops. #267

Merged

savetheclocktower mentioned this pull request Sep 21, 2018

Always terminate a snippet when the cursor leaves a tab stop… #273

Merged

savetheclocktower deleted the transformations branch September 21, 2018 17:41

savetheclocktower mentioned this pull request Sep 23, 2018

Programmable snippets #109

Closed

savetheclocktower mentioned this pull request Feb 2, 2023

Remove implicit g flag from snippet transformations pulsar-edit/snippets#7

Merged

		@@ -0,0 +1,63 @@
		SnippetHistoryProvider = require './snippet-history-provider'

Add support for simple regex transformations in tab stops. #260

Add support for simple regex transformations in tab stops. #260

Uh oh!

Conversation

savetheclocktower commented Dec 18, 2017

Description of the Change

Design

Worth noting

What is and isn't included

Alternate Designs

Benefits

Possible Drawbacks

Applicable Issues

Uh oh!

nathansobo 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!

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!

nathansobo Jan 4, 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!

nathansobo commented Jan 4, 2018

Uh oh!

savetheclocktower commented Jan 4, 2018

Uh oh!

nobleach commented Jun 1, 2018

Uh oh!

savetheclocktower commented Jun 1, 2018

Uh oh!

nobleach commented Jun 1, 2018

Uh oh!

Uh oh!

nathansobo Jan 4, 2018 •

edited

Loading