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

Fixed inconsistency with vim-surround. fixes #47 and #26. #48

Merged
merged 0 commits into from
Dec 16, 2014

Conversation

Tritlo
Copy link
Contributor

@Tritlo Tritlo commented Dec 15, 2014

@tpope has changed this in vim-surround, so now he doesn't clobber v_s (see tpope/vim-surround@6f0984a). It makes sense to maintain parity with the vim-surround keybindings, right?

@timcharper
Copy link
Collaborator

Yes, it does make sense to keep them in sync, but I'm surprised to see this. S vs s have different meanings, the capital variety surrounding over full lines. I'll try out your patch.

@timcharper timcharper merged commit fac4883 into emacs-evil:master Dec 16, 2014
@syl20bnr
Copy link
Member

@timcharper This is indeed a very surprising move and it comforts people to use s instead of c which does exactly the same thing. I don't hesitate to call this a design flaw in Vim mapping and it bother me that Evil-surround blindly follow this mistake.

More info here: https://github.com/syl20bnr/spacemacs/blob/master/doc/DOCUMENTATION.md#the-vim-surround-case

This is not an issue for spacemacs since I will remap S to s.

By the way, nobody has ever complain about this in evil-surround so there must be an explanation.

@Tritlo
Copy link
Contributor Author

Tritlo commented Dec 16, 2014

@syl20bnr, see issue #26

@timcharper
Copy link
Collaborator

Conforming to upstream behavior by default is likely to reduce surprise the most. If people want to revert the functionality with key-bindings, so be it.

I don't particularly care for the change either and wish Tim Pope stood his ground. The capital-S vs lower-case-s had consistent meaning, and now this is broken. But it happened and I'm sure Tim Pope had a good reason.

@Tritlo
Copy link
Contributor Author

Tritlo commented Dec 16, 2014

I agree with @syl20bnr, people should learn to use c instead of s. However, this should remain in sync to upstream vim-surround.

@syl20bnr
Copy link
Member

I'm not sure Tim Pope had another reason than just fixing it because it broke the workflow of a non negligible number of users. Summoning @tpope :-) What is your official reason behind it ?

You are right that the consistency with vim-surround is more important. The design decision should only concern spacemacs. Sorry for this!

@tpope
Copy link

tpope commented Dec 16, 2014

Yes, it was a very common complaint, and even I, after several years of using my own plugin, would occasionally flub and surround when I was trying to substitute. I think the lack of a motion for s in normal mode makes it perhaps a better choice than c for normal/visual parity, hence why it's such an easy mistake to make, even for me.

And the s/S distinction is still there as S/gS, only I flipped things around a bit such that S is almost always the one you want, leaving gS for a more obscure, mode dependent alternative.

@syl20bnr
Copy link
Member

@tpope I see now the s parity with normal/visual modes, makes sense. That's interesting, I learnt Vim in 2010 with an s enabled surround.vim and I never updated it, so my Vim education has been made with s = surround and c = change without the substitute. Without knowing it I used vc instead of s and cc instead of S (even this one). When Matthías wrote that s did not work in spacemacs I was like oh crap! did I miss something useful in Vim ? So I checked up and was surprised that s does not bring anything new to the table. In fact, where s became S in visual mode for surround.vim, c and s do the same thing. surround is so powerful (I use it often in visual mode because of expand-region) that when I tried the fix from Matthías to revert to the true behavior of surround.vim, I just was not able to bear it, s-does-the-same-thing-as-c, why I should make surround more difficult to type ?

So my point is that your first intent with surround.vim to replace the not so useful s was the right thing to do. I won't make the same mistake with spacemacs and let s for surround and c for change. And thank you Tim and Tim for evil-surround.vim :-)

syl20bnr added a commit to syl20bnr/spacemacs that referenced this pull request Dec 17, 2014
@uu1101
Copy link

uu1101 commented Jan 12, 2015

The commit removes evil-Surround-region, but it should be bound to gS in visual mode.

@timcharper
Copy link
Collaborator

I noticed that; thanks. Commit 5e6bcb3 should resolve that; can you try it?

@uu1101
Copy link

uu1101 commented Jan 19, 2015

Works fine, thank you!

@dimsuz
Copy link

dimsuz commented Jun 4, 2018

So now the evil's is the only implementation of vim-surround which differs in this regard. vim, idea-vim work by using S.

I use vim-editing in multiple IDEs/editors and now, because of this change, it feels really weird for me, because muscle memory thing doesn't work anymore, same key behaves differently depending on the editor...

Is it possible to include some kind of "configuration" for this behavior, so one could switch to using "s" for substitution, "S" for surround in visual mode?

@edkolev
Copy link
Member

edkolev commented Jun 4, 2018

Is it possible to include some kind of "configuration" for this behavior, so one could switch to using "s" for substitution, "S" for surround in visual mode?

I believe that's what the current default mappings are.

If you don't like any, you could easily change them. This is what I use (I'm not sure if it matches exactly the default):

    (evil-define-key 'operator global-map "s" 'evil-surround-edit)
    (evil-define-key 'operator global-map "S" 'evil-Surround-edit)
    (evil-define-key 'visual global-map "S" 'evil-surround-region)
    (evil-define-key 'visual global-map "gS" 'evil-Surround-region)

@dimsuz
Copy link

dimsuz commented Jun 4, 2018

Hmm, for me doing vws' on some word in spacemacs currently surrounds that word with single quotes. I always expect vws to delete that word instead, and cringe inside when it doesn't :)

Thank you for suggestions, I will try.

@edkolev
Copy link
Member

edkolev commented Jun 4, 2018

@dimsuz
Copy link

dimsuz commented Jun 4, 2018

@edkolev Great! Thank you!

I have one clarification though. I feel like the following statement is not completely correct for visual mode s:

s is only useful to delete one character and add more than one character which is a very narrow use case

If it was so, I wouldn't be so attached to using s in visual mode (I guess). My use case is that I often start visual mode (v), then select a word or two in this mode (vww), and only then I press s to replace those two words with something else. So in this case, 's' works on an entire selection I made rather than on first letter.

OTOH, in normal mode s is behaving exactly like you've said, and in normal mode I'd gladly use s for surround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants