New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from Oct 28, 2017

Conversation

Projects
None yet
2 participants
@50Wliu
Member

50Wliu commented Oct 2, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

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

This comment has been minimized.

Show comment
Hide comment
@Ingramz

Ingramz Oct 18, 2017

Contributor

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

Contributor

Ingramz commented Oct 18, 2017

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

Show outdated Hide outdated lib/snippet-expansion.coffee Outdated
@Ingramz

This comment has been minimized.

Show comment
Hide comment
@Ingramz

Ingramz Oct 19, 2017

Contributor

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.

Contributor

Ingramz commented Oct 19, 2017

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.

50Wliu added some commits Sep 30, 2017

More 🎨
Makes it more clear that body and tabStops are top-level variables

@50Wliu 50Wliu merged commit 84da051 into master Oct 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the wl-correct-tab-stops branch Oct 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment