Fix handling of whitespace when delimiters are on separate lines

[Diomendius]

Currently, there are some issues with linewise surround insertion and changing surrounds where the opening delimiter has only whitespace between it and the end of the line or where the closing delimiter has only whitespace between it and the beginning of the line.

foo

For instance, assuming default configuration, if ySS{ is run on the above example text, the result is the following, with · marking space characters:

{·
····foo
}

If cs{[ is then run, the result is:

[·
····foo
·]

If cs]{ is then run, the result is:

{··
····foo
··}

cs{} will remove the trailing spaces after { and the leading spaces before } one by one, including the leading indentation before } if the entire text were space-indented beforehand. Correctly choosing between the left-hand and right-hand delimiter bindings can avoid the issue, but the current behaviour is neither useful nor intuitive.

This PR fixes the issue with insertion by automatically stripping whitespace from delimiters when line_mode is on. It also refactors linewise visual surround to use line_mode, so as to avoid duplicate logic.

It fixes the issue with changing surrounds by checking for leading and trailing whitespace in change_surround(), and:

• If the opening delimiter is followed by only whitespace until the end of the line, the edit selection for the opening delimiter is expanded until the end of the line and any trailing whitespace in the delimiter to be inserted is removed before inserting it.
• If the closing delimiter is preceded by only whitespace until the beginning of the line, the edit selection for the closing delimiter is shrunk to exclude all preceding whitespace and any leading whitespace in the delimiter to be inserted is removed before inserting it.

The end result is that it generally doesn't matter whether the left-hand or right-hand bindings are used when inserting or changing surrounds where the body text is on separate lines. If there is non-whitespace between the opening delimiter and the end of the line or between the closing delimiter and the beginning of the line, the behaviour of cs/change_surround() is unchanged (per delimiter), which is probably desirable.

In this case automatically stripping whitespace after the opening delimiter would be both visible and surprising, and the whitespace between the closing delimiter and the beginning of the line couldn't be part of the line's indent, so the existing behaviour is reasonable here, too.

Highlighting of delimiters is not changed. The left-hand bindings highlight the spaces they match, and the highlighting that is displayed is the same regardless of whether the new logic applies.

As an aside, though not touched upon in this PR, it's arguable that changing the behaviour of change_surround() to always strip whitespace, such that cs() always results in enclosing parentheses with no space padding at all and cs)( or cs(( always results in enclosing parentheses with a single space of padding, could be desirable. I think this can actually be changed just by changing the relevant patterns in your config to match * instead of ? (zero or more instead of zero or one spaces), so a change to nvim-surround proper isn't necessary unless it's decided that this would be a better default behaviour.

[kylechui]

For the first example, I assume you mean that after ySS{ it produces

{·
····foo
·}

Aside from that, I think I'm having a bit of trouble trying to understand what the proposed PR would do for the examples that you gave. It seems like if the delimiters are on separate lines, you want ( to behave as ). If that's the case, my current take on it is that the "extra edge case" that needs to be handled there isn't super transparent.

Correctly choosing between the left-hand and right-hand delimiter bindings can avoid the issue, but the current behaviour is neither useful nor intuitive.

For the first example that you provided about cs{}, I fail to see how the behavior is unintuitive. It might not be the most efficient way to handle things, but I think it is consistent with the way that the { vs. } bindings are defined.

This isn't to say that I won't accept this PR (eventually), but I do want to try and understand the motivation for this PR more before I decide to merge it in (also additional test cases would be nice, both for mitigating regressions but also to help me understand what behavior you want out of this).

[kylechui]

I do appreciate the time and effort you've put into the PR, and thanks for reminding me that gS and visual line mode have more or less the same implementation! The real question there would be if anybody actually utilizes visual line mode selection, and then gS to get two extra lines of space on each side.

[Diomendius]

For the first example, I assume you mean that after ySS{ it produces

{·
····foo
·}

With indent_lines = false in the config, it produces that. With :set equalprg= indentexpr= nocindent nosmartindent nolisp it actually produces the same result except foo has no indent. If indent_lines is unspecified in the config and any of the aforementioned settings are true/non-empty, it removes the leading space before the } due to autoindent. :set smartindent and :set cindent each cause the result I originally stated. I should have been more specific, since with completely default Nvim and nvim-surround config it doesn't remove the leading space before }, but many users have either cindent or smartindent set and I overlooked this detail because I am one of them.

Aside from that, I think I'm having a bit of trouble trying to understand what the proposed PR would do for the examples that you gave. It seems like if the delimiters are on separate lines, you want ( to behave as ). If that's the case, my current take on it is that the "extra edge case" that needs to be handled there isn't super transparent.

Part of my rationale is simply to bring this plugin more in line with what vim-surround does. In that plugin, ( never inserts whitespace when the body is separated from the delimiters by newlines. Interestingly, cs}{ in vim-surround inserts extra spaces just as nvim-surround does (except when delimiters are on separate lines) but cs{} removes all spaces on the inside of each delimiter and cs{{ replaces any existing padding with single-space padding, which is very similar to what I described at the end of the OP.

The main motivation is that I think this plugin should never insert trailing whitespace, and it shouldn't modify line indent except through autoindent, which can easily be turned off. A case could be made for ySS adding indent (maybe by running :'<,'>>) even with autoindent off, but this is out of scope for this PR and should be independently configurable anyway.

Correctly choosing between the left-hand and right-hand delimiter bindings can avoid the issue, but the current behaviour is neither useful nor intuitive.

For the first example that you provided about cs{}, I fail to see how the behavior is unintuitive. It might not be the most efficient way to handle things, but I think it is consistent with the way that the { vs. } bindings are defined.

It's not hard to figure out why nvim-surround behaves the way it does, but that doesn't mean the way it currently behaves is useful. Currently, { means "{ and } delimiters with a single space of padding" in insertion context and "{ and } delimiters with zero or one spaces of padding each" in match context. You could define { and } even more strictly such that cs{{ on {foo} does not work but cs}{ results in { foo }, but that doesn't seem useful to me either.

Conceptually, I think the ({[… bindings should simply mean "with padding" and the )}]… bindings should simply mean "without padding". I consider both spaces and newlines to be forms of padding. I do not consider line indentation to be padding. I consider mixing newlines and spaces in padding to be a formatting error.

This isn't to say that I won't accept this PR (eventually), but I do want to try and understand the motivation for this PR more before I decide to merge it in (also additional test cases would be nice, both for mitigating regressions but also to help me understand what behavior you want out of this).

More tests never hurt (within reason). I remember seeing your test suite before and it seems fairly painless to work with, so I'll see what I can do.

Some examples of my desired behaviour

``` { ····{ ········foo; ····} } ``` If the cursor is positioned somewhere on `foo;`, `cs{{`, `cs{}`, `cs}{` and `cs}}` should all behave identically and result in unchanged text. `cs{[` should result in ``` { ····[ ········foo; ····] } ``` not ``` { ····[· ········foo; ····] } ```

`cs]{` and `cs[{` on the example below ``` [foo ····bar ] ``` should both result in ``` {·foo ····bar } ``` not ``` {·foo ····bar ·} ```

[kylechui]

Gotcha, I see what you mean now. I've played around a bit more with vim-surround, and I think I have a better idea of what's going on. When I have the time I'll come back to this and actually review the code, and hopefully we can get it merged in!

[Diomendius]

I wrote some tests and fixed a couple of bugs I really should have caught in manual testing before I published the PR. The regex for closing delimiter whitespace during a linewise insert was broken and I didn't catch it because I was testing in an environment that had autoindent enabled. I also missed another bug in change_surround() for a similar reason. The logic for adjusting the selection window around the closing delimiter was wrong, but autoindent hid this from me too.

If feature parity with vim-surround is desired, it does support the idea of adjusting the match expressions for the left-hand/padded bindings to use * instead of ?, though I'm still not pushing this idea very hard, as it's easily user-configurable.

[kylechui]

Thanks a bunch for the PR! (I had to close/reopen the PR to reload the test suite since it got stuck)