Plugin proposal: Surround

[ascandella]

Sub-issue of #590

I, along with others, are interested in getting TPope's surround plugin implemented here.

I don't really understand ViML so it would likely be a re-implementation rather than a transcription.

Leaving this issue open for others to subscribe and possibly contribute to. Eventually if I have enough free time I will try to implement myself. It will likely be a subset (for example, I don't even use tags like ysiw<em> but if the architecture is correct this shouldn't be too hard.

cc @rebornix

NOTE: per discussion on other tickets, this would be a "core" feature (e.g. not a separate plugin/requirejs) that would be behind a feature flag.

Open question: on by default, or off by default?

[johnfn]

Definitely gets a thumbs up from me.

I'm down to implement it with a flag that is on by default as it doesn't
overlap any Vim features (as I understand).

The surround.vim plugin is only about 600 lines:
https://github.com/tpope/vim-surround/blob/master/plugin/surround.vim

Even though I don't really understand viml, the source doesn't seem that
bad to parse. A straight-up transcription, while incredibly boring and
uninteresting to do, would probably be the best way to get the
functionality into VSCodeVim. It also makes the work highly
parallelizeable, so if anyone else wants to join in, they could. If you're
committed to doing the whole thing though, please feel free to do it any
way you want. :)

On Mon, Aug 15, 2016 at 4:21 PM, Aiden Scandella notifications@github.com
wrote:

Sub-issue of #590 #590

I, along with others, are interested in getting TPope's surround plugin
https://github.com/tpope/vim-surround implemented here.

I don't really understand ViML so it would likely be a re-implementation
rather than a transcription.

Leaving this issue open for others to subscribe and possibly contribute
to. Eventually if I have enough free time I will try to implement myself.
It will likely be a subset (for example, I don't even use tags like
ysiw but if the architecture is correct this shouldn't be too hard.

cc @rebornix https://github.com/rebornix

NOTE: per discussion on other tickets, this would be a "core" feature
(e.g. not a separate plugin/requirejs) that would be behind a feature flag.

Open question: on by default, or off by default?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#610, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKPQdaDsraBYRVWuwONwAkdKV1T_if1ks5qgJIVgaJpZM4Jkh5T
.

Grant

[johnfn]

Wow, this issue has certainly gotten a lot of activity.

PRs welcomed. 😉

[ascandella]

I actually started looking into implementing it but couldn't figure out how to bind the keys properly. I'll push my branch up showing what i tried and let an expert explain why it wasn't working...

[ascandella]

So regardless of what business logic is necessary, I was looking into how to hook up the keybindings correctly. I tried adding this to actions.ts:

diff --git i/src/actions/actions.ts w/src/actions/actions.ts
index 3f542ba..f9b1001 100644
--- i/src/actions/actions.ts
+++ w/src/actions/actions.ts
@@ -1097,6 +1097,16 @@ export class ChangeOperator extends BaseOperator {
 }

 @RegisterAction
+export class ChangeSurroundOperator extends BaseOperator {
+  public keys = ["c", "s"];
+  public modes = [ModeName.Normal];
+
+  public async run(vimState: VimState, start: Position, end: Position): Promise<VimState> {
+    return vimState;
+  }
+}
+
+@RegisterAction
 export class PutCommand extends BaseCommand {
     keys = ["p"];
     modes = [ModeName.Normal];

(and then put a breakpoint inside async run)

But it wouldn't match the chord. I'm pretty sure I'm missing something obvious, but this is when I stopped and went back to my crushing backlog of "real" (aka paid) work

[johnfn]

My best guess is that the change operator gets triggered immediately when you press "c", so an action with "c" "s" can't be triggered.

[johnfn]

A better idea might be to make "s" plus whatever actions come next for the surround command into movements and handle them specially in the change operator class.

[ascandella]

Yea, I've stared at this a few times and not gotten anywhere in my limited freetime. I'm trying to look for prior art where a movement/command takes input after, but all I've found is stuff like the code for iw which knows the range/modification already.

I think I just need to read through the entire codebase to truly understand the right place to hook everything in.

[johnfn]

Believe it or not, but as far as we've gotten, we don't actually have any commands that take input after being triggered. The only real equivalent we have is the /, which actually drops you into a new state while it's taking new input (a rather complex solution for what you want to do).

What we do have is operator objects that can be combined with motion objects that come later. If you do c, a ChangeOperator gets added to recordedState. But we don't actually trigger any action until operatorReadyToExecute returns true, which will happen when you enter a movement.

So if you want to handle something like cs"' I would suggest making a motion which is s"' which records some information about what you want to substitute along with returning the range you which to operate on as an IMotion.

Then once the motion was finished and the change operator was finally run, you could look for the stuff that s"' recorded and run the surround.

I have to say though, it looks like our current infrastructure is not 100% ready for surround. Along with the above issue, I also see:

• The issue of having movements that take arbitrarily long numbers of keystrokes followed by an enter so you can do stuff like cs'<tag>. This is not too hard.
• The fact that the motion is s"' for a change operator, but s" for a delete operator. Currently we handle all motions the same for all operators, so this would need to be special cased somehow.

[monkoose]

I do not mind myself working in vim without surround plugin. Thats bad that you have implementation issues for now. Still like your vim plugin the most.

[johnfn]

Oh. The implementation issues I was talking about should certainly not stop anyone from trying to implement surround. They're just things to take note, not show stoppers!

[xconverge]

Might be a little easier to read if someone is looking for ideas on how this could be done

https://github.com/jcartledge/sublime-surround/blob/master/Surround.py

[johnfn]

I said I wouldn't do it, but then I did it anyways: Surround.vim has been added to VSCodeVim as of #1238 and v0.5.0.

Please try it out and let me know how it works. I have never used Surround in my life, so it's very possible that I am missing some things. :)

(Oh, and by the way, did I mention you can donate..? 😄 )

[kylpo]

I've been on the fence all weekend about sticking with vim or switching to vscode. This PR has made my decision significantly easier. Not everything is seamless from vim yet, but boy is it close, and I'm confident now that it'll get there. Your momentum and dedication to the project is laudable - thank you!

(Also, thanks for the reminder of the donation link. Totally worth "paying" for features like this)

[xconverge]

@johnfn is a legend

[ascandella]

/me bows

…

On Tue, Jan 24, 2017 at 4:29 PM Sean Kelly ***@***.***> wrote: @johnfn <https://github.com/johnfn> is a legend — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#610 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF7P7665cir5uRv1-___upw9M3qOYITks5rVpK_gaJpZM4Jkh5T> .