Skip to content
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

Tune behavior of ignore whitespace on current line #169

Merged
merged 3 commits into from May 30, 2018

Conversation

Projects
None yet
4 participants
@t9md
Copy link
Contributor

t9md commented Jan 11, 2018

Requirements

Fix #76

Description of the Change

Previously ignoreWhitespaceOnCurrentLine was not appropriately respected when one buffer is opened in multiple editors.
This PR improve behavior of ignoreWhitespaceOnCurrentLine how most user expect(I believe so).

In case test case's intention was not clear, see following illustration what happens on save.

Condition

When two editor open same file. Two editor shares same buffer.

  • | represent cursor
  • _ represent trailing white spaces
  • editor1 have cursor at 1st line
  • editor2 have cursor at 2nd line
+-------------------------+-------------------------+
| editor1                 | editor2                 |
+-------------------------+-------------------------+
| line1____|              | line1____               |
| line2____               | line2____|              |
| line3                   | line3                   |
|                         |                         |
+-------------------------+-------------------------+

case 1: saved when editor1 is active

result: editor1's cursor line was excluded

+-------------------------+-------------------------+
| editor1(active)         | editor2                 |
+-------------------------+-------------------------+
| line1____|              | line1____               |
| line2                   | line2|                  |
| line3                   | line3                   |
|                         |                         |
+-------------------------+-------------------------+

case 2: saved when editor2 is active

result: editor2's cursor line was excluded

+-------------------------+-------------------------+
| editor1                 | editor2(active)         |
+-------------------------+-------------------------+
| line1|                  | line1                   |
| line2____               | line2____|              |
| line3                   | line3                   |
|                         |                         |
+-------------------------+-------------------------+

case 3: saved when either editor1 nor editor2 was active

This situation happens when Window: Save All command was fired.

result: just removed all trailing spaces

+-------------------------+-------------------------+
| editor1                 | editor2                 |
+-------------------------+-------------------------+
| line1|                  | line1                   |
| line2                   | line2|                  |
| line3                   | line3                   |
|                         |                         |
|                         +-------------------------+
|                         | editor3(active)         |
|                         +-------------------------+
|                         | abc                     |
|                         | def                     |
+-------------------------+-------------------------+

Alternate Designs

Currently same buffer have multiple buffer.onWillSave hook.
By making this subscription per buffer basis, I think most behavior frustration described in #76 would be fixed.
But as far as I experimented it in my mind, the behavioral result would be same in that case.

So as my understanding making buffer.onWillSave subscription to per buffer-basis is kind of refactoring.
So not included in this PR.

Benefits

Users are no longer surprised by trailing WS at cursor line is removed in spite of ignoreWhitespaceOnCurrentLine is true.
This was happened when multiple same file is opened in multiple editors.

Possible Drawbacks

None

Applicable Issues

Fix #76

@ungb

This comment has been minimized.

Copy link

ungb commented Jan 25, 2018

@t9md, I tried testing this fix out and I'm not seeing this working...

I added a console.log to ensure that your changes are being used.

I followed the steps in your example and looked at the issue in question. Let me know if I'm missing something.

I'm seeing the following:
repro

@t9md

This comment has been minimized.

Copy link
Contributor Author

t9md commented Jan 26, 2018

@ungb

The behavior you showed in your GIF is expected behavior of this PR.
Which is same scenario I described as case 2: saved when editor2 is active in first comment.

When editor is saved, right pane is active, so trailing-WS at line1 is preserved, but left pane's cursor line(line2) is NOT preserved since it's not active pane when editor was saved.

I feel this behavior is natural,
You expect left inactive pane's cursor line(line2)'s trailing-WS too?

In that case, I feel it bug since

  • Saving on right active pane editor does not remove tralingWS of line2 which line have no cursor on right pane editor.
  • While I'm editing in right pane editor, I'm totally forgetting about cursor line of inactive pane(left pane editor).

@daviwil daviwil referenced this pull request May 29, 2018

Closed

Iteration Plan: May 29, 2018 #17426

8 of 17 tasks complete
@daviwil

This comment has been minimized.

Copy link
Member

daviwil commented May 30, 2018

Just tested this out, looks good to me! The new tests also look good. Thanks a lot @t9md!

@daviwil daviwil merged commit b3b7dd0 into atom:master May 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.