atom / snippets

Atom snippets package
MIT License
205 stars 100 forks source link

Correctly place tab stops when there are multiple indented lines #245

Closed 50Wliu closed 6 years ago

50Wliu commented 6 years ago

Requirements

Description of the Change

Before this PR, this is how snippet expansion worked:

  1. The entire snippet body would be inserted into the editor
  2. Tab stop markers would be added
  3. Tabs would be converted to spaces (and vice versa) if necessary
  4. If the snippet consisted of multiple lines, all lines would be indented to match the first line's indentation level

This process worked pretty well, except for one case - when there was a tab stop at the beginning of a subsequent line in a multiline snippet. In this case, the tab stop marker would initially be placed at [line, 0], [line, 0]. However, when indenting that line in step 4, the marker is shifted and becomes [line, 0], [line, indent] since it touched the area where the shifting occurred (normally, if the editor changes and the marker does not include the changed region, both the start and end get shifted). As a result, the leading whitespace would be selected instead of just placing the cursor at the tab stop.

With this PR, snippet expansion now works as follows:

  1. The indentation level is retrieved and added to each subsequent line of the snippet body if multiline
  2. Tab stop markers are shifted to correct for the added indentation
  3. Snippet body with indentation is inserted into the editor
  4. Tab stop markers corrected for indentation are added
  5. Tabs would be converted to spaces (and vice versa) if necessary

Alternate Designs

This could perhaps be done earlier in the file that calls SnippetExpansion.

Benefits

I think this logic is more straightforward, since you don't have to think about how the markers will be internally shifted anymore. It's all done explicitly.

Possible Drawbacks

I don't see any.

Applicable Issues

Fixes #140 Fixes #213

Ingramz commented 6 years ago

After fooling around a little, found this in JavaScript: snippet_broken

Ingramz commented 6 years ago

A little 🐎 in case indents are empty or there is no need for indenting depending on line count.

diff --git a/lib/snippet-expansion.coffee b/lib/snippet-expansion.coffee
index bec664d..46be058 100644
--- a/lib/snippet-expansion.coffee
+++ b/lib/snippet-expansion.coffee
@@ -10,22 +10,20 @@ class SnippetExpansion
     @selections = [@cursor.selection]

     startPosition = @cursor.selection.getBufferRange().start
-    # Get leading indentation level
-    indent = @editor.lineTextForBufferRow(startPosition.row).match(/^\s*/)[0]
-    # Add proper indentation to the snippet
-    body = @snippet.body.replace(/\n/g, '\n' + indent)
+    if @snippet.lineCount > 1 and indent = @editor.lineTextForBufferRow(startPosition.row).match(/^\s*/)[0]
+      # Add proper indentation to the snippet
+      body = @snippet.body.replace(/\n/g, '\n' + indent)

-    tabStopRanges = []
-    for tabStop in @snippet.tabStops
-      ranges = []
-      for range in tabStop
-        newRange = Range.fromObject(range, true) # Don't overwrite the existing range
-        if newRange.start.row # a non-zero start row means that we're not on the initial line
-          # Add on the indent offset so that the tab stops are placed at the correct position
-          newRange.start.column += indent.length
-          newRange.end.column += indent.length
-        ranges.push(newRange)
-      tabStopRanges.push(ranges)
+      tabStopRanges = @snippet.tabStops.map (tabStop) ->
+        tabStop.map (range) ->
+          newRange = Range.fromObject(range, true) # Don't overwrite the existing range
+          if newRange.start.row # a non-zero start row means that we're not on the initial line
+            # Add on the indent offset so that the tab stops are placed at the correct position
+            newRange.start.column += indent.length
+            newRange.end.column += indent.length
+          newRange
+    else
+      {body, tabStops: tabStopRanges} = @snippet

     @editor.transact =>
       newRange = @editor.transact =>

The version without whitespace regex I proposed earlier doesn't work if there is text preceding the snippet trigger, which would have been copied to subsequent lines, so I reverted that part.