Add option to ignore current line when trimming whitespace #12

Merged
merged 19 commits into from Mar 3, 2014

Projects

None yet

4 participants

@watsonian
Contributor

It's entirely possible that there's a better way to do this, but I came up with this solution. This adds a new ignoreWhitespaceOnCurrentLine option. When enabled, it prevents saves from trimming whitespace on the current line. This is to catch situations where you've just started a new method definition, have the leading indentation in place, and then save. You obviously don't want that whitespace to disappear. Or situations where you're typing a Markdown list, start a new bullet point with a space, and then save. It works with multiple cursors too.

Depends on atom/text-buffer#4 being merged to get TextBuffer#backwardsScan.

watsonian added some commits Feb 28, 2014
@watsonian watsonian Don't touch whitespace on the current line.
It's fairly common to start a method definition and then save
just before starting in on that method. The result is that the
method indentation is removed. This happens in a number of
situations. With this change, whitespace on the current line
is ignored if there's ONLY whitespace on that line.
f362fe6
@watsonian watsonian Add some tests! ac916ac
Contributor

For a visual example of what this actually does:

before save

xx
def somethingxxx
xxC
end

after save


def something
xxC
end

where x is a space and C is the cursor. Prior to this change you would have ended up with:


def something
C
end

@mutle mutle commented on an outdated diff Feb 28, 2014
lib/whitespace.coffee
if grammarScopeName is 'source.gfm'
# GitHub Flavored Markdown permits two spaces at the end of a line
[whitespace] = match
replace('') unless whitespace is ' ' and whitespace isnt lineText
+ else if ignoreCurLine and onlyWhitespace and whitespaceRow is cursorRow
mutle
mutle Feb 28, 2014

How about this instead to get rid of the empty statement?

else if not (ignoreCurLine and onlyWhitespace and whitespaceRow is cursorRow)
  replace('')
watsonian added some commits Feb 28, 2014
@watsonian watsonian No need for the no-op line. 16dcf1d
@watsonian watsonian Ignore whitespace regardless on current line. f05441e
@watsonian watsonian Fix off-by-one error. ab52008
Contributor

This works fairly well now. I'm going to investigate getting this to work with multiple cursors and also see if it's possible to trim all whitespace after the current cursor position.

watsonian added some commits Feb 28, 2014
@watsonian watsonian Shortcircuit instead so it's applied to markdown too. f481c07
@watsonian watsonian Looks like it was an Atom bug and not an off-by-one error. e9e2f21
@nathansobo nathansobo and 1 other commented on an outdated diff Mar 1, 2014
lib/whitespace.coffee
@@ -24,8 +24,16 @@ class Whitespace
@subscribe buffer, 'destroyed', =>
@unsubscribe(buffer)
- removeTrailingWhitespace: (buffer, grammarScopeName) ->
+ removeTrailingWhitespace: (editor, grammarScopeName) ->
+ buffer = editor.getBuffer()
+ ignoreCurLine = atom.config.get('whitespace.ignoreWhitespaceOnCurrentLine')
nathansobo
nathansobo Mar 1, 2014 Contributor

Can you spell out this variable name? ignoreCurrentLine

watsonian
watsonian Mar 1, 2014 Contributor

Yep, will do.

Contributor

Okay, sorted out the weird seemingly off-by-one issue. The problem has to do with the indexes changing as the preceding whitespace is removed on other lines. I'm going to try changing this so the buffer scan goes in reverse to see if that solves it.

@nathansobo nathansobo and 1 other commented on an outdated diff Mar 1, 2014
spec/whitespace-spec.coffee
@@ -52,6 +52,41 @@ describe "Whitespace", ->
editor.save()
expect(editor.getText()).toBe "don't trim me \n"
+ describe "whitespace.ignoreWhitespaceOnCurrentLine config", ->
nathansobo
nathansobo Mar 1, 2014 Contributor

I'd like to see these specs structured as follows:

describe "when 'whitespace.ignoreWhitespaceOnCurrentLine' is true", ->
  it "removes the whitespace from all lines, excluding the current line", ->

describe "when 'whitespace.ignoreWhitespaceOnCurrentLine' is false", ->
  it "removes the whitespace from all lines, including the current line", ->

I'm not convinced we need a separate test for lines that are only whitespace vs lines that have trailing whitespace. Thoughts?

watsonian
watsonian Mar 1, 2014 Contributor

You're right. It mattered initially because I was only going to have it work against lines that only had whitespace, but since then I changed course and they're not needed.

watsonian added some commits Mar 1, 2014
@watsonian watsonian s/ignoreCurLine/ignoreCurrentLine/ ee65af4
@watsonian watsonian Switch to using backwardsScan.
This fixes a bug where the cursor index would change out from under
you as the replacements in the scan were happening. This was causing
the cursor row to be mis-identified, which was resulting in inconsistent
row identification (which looked sort of like an off-by-one error).
By scanning from the end of the buffer, we prevent the index from getting
mangled like that and end up with the correct cursor row.
9ceaa58
@watsonian watsonian referenced this pull request in atom/text-buffer Mar 1, 2014
Merged

Add backwardsScan function #4

watsonian added some commits Mar 1, 2014
@watsonian watsonian Give specs better names. Remove unnecessary specs. 14c11a5
@watsonian watsonian Actually use the correct cursor position in the specs. 18d642c
@watsonian watsonian Add support for multiple cursors. 8b6a11b
@watsonian watsonian Update specs for multiple cursor support. 5961b67
Contributor

@nathansobo Okay, I think this is ready for review now.

watsonian added some commits Mar 1, 2014
@watsonian watsonian Use setCursorBufferPosition() first to collapse cursors. f658ea0
@watsonian watsonian Get the buffer position correct for second cursor. 52bc414
@nathansobo nathansobo and 1 other commented on an outdated diff Mar 1, 2014
spec/whitespace-spec.coffee
+ it "removes the whitespace from all lines, excluding the current lines", ->
+ editor.insertText "1 \n2 \n3 \n"
+ editor.setCursorBufferPosition([1,3])
+ editor.addCursorAtBufferPosition([2,3])
+ editor.save()
+ expect(editor.getText()).toBe "1\n2 \n3 \n"
+
+ describe "when 'whitespace.ignoreWhitespaceOnCurrentLine' is false", ->
+ [originalConfigValue] = []
+ beforeEach ->
+ originalConfigValue = atom.config.get("whitespace.ignoreWhitespaceOnCurrentLine")
+ expect(originalConfigValue).toBe true
+ atom.config.set("whitespace.ignoreWhitespaceOnCurrentLine", false)
+
+ afterEach ->
+ atom.config.set("whitespace.ignoreWhitespaceOnCurrentLine", originalConfigValue)
nathansobo
nathansobo Mar 1, 2014 Contributor

You shouldn't need to reset this as the config is restored after each spec run. If that's not the case there's something wrong.

nathansobo
nathansobo Mar 1, 2014 Contributor

For that reason, I'd say ditch the beforeEach calls and just do everything in the it.

watsonian
watsonian Mar 1, 2014 Contributor

Ah, okay. I was following what was already in place with the whitespace.ensureSingleTrailingNewline tests. If that's not needed, I can pull it out for sure.

watsonian added some commits Mar 1, 2014
@watsonian watsonian The config value is automatically restored on each spec run. 0384362
@watsonian watsonian Cleanup ensureSingleTrailingNewline specs. 6b2ee17
@watsonian watsonian Fix GFM tests. bddc6fd
@watsonian watsonian Cleanup and a broken spec. d7dc98f
Contributor

Okay, this is looking good, but since it required a new text-buffer API we're going to need to wait for the next Atom release to merge and publish it. Should be soon.

Contributor

@nathansobo 🤘 thanks for helping me out with this! 😁

Habbie commented Mar 3, 2014

+1 for this feature! Came here to complain about annoying interaction between https://github.com/atom/whitespace and https://github.com/atom/autosave, and this would fix it!

Contributor

The text-buffer change in atom/text-buffer#4 is in the latest Atom release now, so merging this! 🤘

@watsonian watsonian merged commit e354303 into master Mar 3, 2014
@watsonian watsonian deleted the ignore-current-line branch Mar 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment