regression: the repeat (.) operator broke on commit 66b8f89

[ninrod]

If I surround a word, ysiwb for example and then try to use the . operator to surround another word, I get: After 0 kbd macro iterations: evil-motion-range: Args out of range: "", 0

Is this a bug or is the dot operator unsupported?

thanks in advance.

[ninrod]

Hi. I'm getting this in spacemacs from git head (develop branch).

After 0 kbd macro iterations: evil-motion-range: Args out of range: "", 0

steps to reproduce:

just surround a word with ysiw" for example. then try to repeat the operation with the . operator.

what the above message be printed in the minibuffer.

[sooheon]

Just to clarify, this is not spacemacs specific. Just a vanilla config of evil-surround has the same error.

[ninrod]

Yes. just install evil, install evil-surround, and apply the reproducible procedure.

[ninrod]

This really is a core functionality of the package. The whole point of the surround package is to allow the user to repeat the surround operation: surrounding is already covered by vim keybinds: ciw""<ESC>P already surrounds the word, but it is not reapeatable.

[noctuid]

I have a little experience fixing problems with/adding support for repeating. It may not be too hard to fix, so I'll probably look at it (but it may be a while before I have any time).

[noctuid]

I took a quick look, and the problem is that the wrong keystrokes are being recorded for y and c (e.g. cs"' records css"c; ysiw" records ys"). d works fine from what I've tested. I may have some time later today to see if it can quickly be fixed. If evil-surround was like vim-operator-surround and bound new operators to new keys instead of highjacking the y/c/d operators, this would potentially be a lot easier to deal with.

Edit: Yeah, the only reason a workaround is required is because the operators are invoked from other operators. They'll work as-is fine if you bind them to their own keys. Fixing c looks like it wouldn't be hard though. I'm not sure about y.

[sooheon]

Thank you for looking into it!

[noctuid]

Since all the information is available for c, a quick fix is pretty simple (in evil-surround-change):

...
(let ((key (evil-read-key)))
      ;; add these lines
      (setq evil-repeat-info (butlast evil-repeat-info 2))
      (evil-repeat-record (concat (list char key)))
      (evil-repeat-stop)
...

This seems to work fine for what I've tried. If this was added, there should be a check to ensure that these modifications are only run for evil-surround-change accessed under the c operator (e.g. just set some variable in evil-surround-edit).

Quickly looking at y, I'm not sure if it's possible to fix because iw (or whatever the text object/motion key(s) is/are) doesn't appear in (this-command-keys) like it normally would. Having the deletion and change bound under c and d makes sense, but ys..'s behavior has nothing to do with y, so I don't really think it's that much of a problem to just bind it to its own key. Still, I'll look into an alternate way of doing this when I get the chance.

[ninrod]

@noctuid, thanks for your effort on this bug. I've tried the fix for cs but it did not rendered the desired results. I do not get an error message, but nothing happens either.

Am I doing it the right way? here is the diff: ninrod/evil-surround@c4fdf0ce91205c1e9529ba8901b2f5cc0e3201c6

[ninrod]

Reported on spacemacs: syl20bnr/spacemacs#7662. Seems like a regression of syl20bnr/spacemacs#4641

[ninrod]

Funny. In theory #74 should have been the fix for this.

[ninrod]

@noctuid, I'm summoning @lislon, the author of #74 who said this:

I wrapped ys/yS operators with raw record commands, because they are handled by evil-yank operator, that is marked as not repeatable. Fixes #24.

@lislon, do you think we've a reggression here?

[ninrod]

an interesting test. If we use ysiw] and then try to repeat with ., I get this:

After 0 kbd macro iterations: evil-motion-range: Wrong type argument: commandp, (keymap (keymap (83 . evil-surround-line) (115 . evil-line) (121 . evil-line)) (keymap) (keymap "Auxiliary keymap for Operator-Pending state" (83 . evil-Surround-edit) (115 . evil-surround-edit)) (keymap (105 keymap (74 . evil-indent-plus-i-indent-up-down) (73 . evil-indent-plus-i-indent-up) (105 . evil-indent-plus-i-indent) (97 . evil-inner-arg) (37 . evilmi-inner-text-object) (111 . evil-inner-symbol) (116 . evil-inner-tag) (96 . evil-inner-back-quote) ...) (97 keymap (101 . evil-entire-entire-buffer) (74 . evil-indent-plus-a-indent-up-down) (73 . evil-indent-plus-a-indent-up) (105 . evil-indent-plus-a-indent) (97 . evil-outer-arg) (37 . evilmi-outer-text-object) (111 . evil-a-symbol) (116 . evil-a-tag) ...)) (keymap) (keymap (115 . s-map-prefix) (f8 . eyebrowse-switch-to-window-config-8) (f7 . eyebrowse-switch-to-window-config-7) (f6 . eyebrowse-switch-to-window-config-6) (f5 . eyebrowse-switch-to-window-config-5) (f4 . eyebrowse-switch-to-window-config-4) (f3 . eyebrowse-switch-to-window-config-3) (f2 . eyebrowse-switch-to-window-config-2) (f1 . eyebrowse-switch-to-window-config-1) ...) (keymap (3 keymap (12 . evil-lisp-state-map-prefix))) (keymap (44 . term-map-prefix) "Auxiliary keymap for Normal state") (keymap "Auxiliary keymap for Normal state" (37 . evilmi-jump-items)) ...)

[ninrod]

confirmed: This is a reggression. We have a culprit: 66b8f89d4fc117d083198dac0ce9d6a65b0c5585

I've found it through a git bisect.

[ninrod]

@sooheon @noctuid and gentleman, I've fixed it here. Basically I've just reverted 66b8f89 and everything is working fine again.

[noctuid]

Nice. It doesn't seem like evil-read-key is needed for anything here, but it still seems strange that it breaks recording like this. I shouldn't have assumed repeating never worked correctly.

Edit: It looks like this happens if read-key-sequence is used in the interactive clause instead, so the problem isn't with evil-read-key.

[wasamasa]

I'd strongly suggest adding tests to both prove repetition works (for a known set of cases) and to ensure regressions encountered previously don't happen again.

[ninrod]

Nice. How can we do that @wasamasa? I'm new in elisp and don't know any test frameworks.

[sooheon]

@ninrod The developer of lispy has a very comprehensive test suite here: https://github.com/abo-abo/lispy/blob/master/lispy-test.el

[wasamasa]

@ninrod I prefer buttercup over ERT, but for that specific kind of tests ecukes might be a better fit.

[ninrod]

@wasamasa thanks for the suggestions. what do you think about undercover?

[ninrod]

can I integrate ecukes with something like travis? same for buttercup?

[wasamasa]

Disregard what I've said about ecukes, Evil is simulating key input just fine for its own tests. I've mistakenly assumed ecukes does something magic...

what do you think about undercover?

I haven't ever used undercover, so no opinion on it, other than me generally finding code coverage a misleading statistic to strive for.

can I integrate ecukes with something like travis? same for buttercup?

Sure.

[ddoherty03]

I'm new to evil mode and ran into this early in playing with it in spacemacs. Is the fix up in MELPA yet? Is there a reason this is still an open issue?

[ninrod]

@ddoherty03, It's already fixed on my fork and I've already opened pull request #93, but I think the main reason is that the package creator @timcharper is away and nobody stood up to the task of mantaining the package yet.

If you want a quick fix, you can use my fork:

git clone my fork to ~/.emacs.d/lisp/ninrod/evil-surround and then load it up. for example, using using-package:

(use-package evil-surround :load-path "lisp/ninrod/evil-surround"
  :config (global-evil-surround-mode 1))

working wonderfully for me here.

[ninrod]

I tried to contact the package author. Let's see if he will respond.

[ddoherty03]

Thanks for the explanation ninrod.

[ddoherty03]

For anyone wanting to use ninrod's fork in a stock spacemacs, I had to do the following: (1) exclude the evil-surround package in dotspacemacs/layers

dotspacemacs-excluded-packages '(evil-surround)

and (2) manually load the local version in dotspacemacs/user-init:

    (with-eval-after-load 'evil
      (progn
        (load "~/.spacemacs.d/ded/lisp/evil-surround/evil-surround.el")
        (global-evil-surround-mode 1)))

With the file path changed to where ever you cloned ninrod's fork.

Trying to use ninrod's snippet above did not work in dotspacemacs/user-config because the stock evil-surround was already loaded by then and did not work in dotspacemacs/user-init because use-package was not available at that point.

[ninrod]

@ddoherty03, @timcharper gave me push access. I've merged the fix so this should be fixed on melpa in a few hours. Tks.

[ddoherty03]

Thanks, @ninrod. Big help.

[ninrod]

np problem, you can throw away my fork and use evil-surround directly from melpa now.

and just a quick tip, when you use the with-eval-after-load form you can drop the progn form:

  (with-eval-after-load 'evil
        (load "~/.spacemacs.d/ded/lisp/evil-surround/evil-surround.el")
        (global-evil-surround-mode 1))

this should be sufficient.

[braham-snyder]

For what it's worth, #93 (well, I suppose the following bug likely existed before the regression #93 fixed) makes one-key bindings to evil-surround-edit (is that the right way to go about binding a single-key to surround? or does someone know a better way?) repeat their surrounding with either two i chars or two a chars instead of the desired character, depending on whether an inner/outer textobj was used.

For example, with r set to evil-surround-edit: riw". on word gives i"word"i instead of ""word"".

This is not the correct way to fix it, but one really quick fix for those looking for one is to edit the definition of evil-surround-edit: just replace the evil-surround-call-with-repeat at the very end with call-interactively.

[ninrod]

@braham-snyder, I don't know if that is supported. Can you you lay down your setup for the "one-key" surround binding? edit: Actually, might be useful to open a separated issue in your specific case.

[braham-snyder]

(use-package evil-surround
    :init

    ;;; FIXME: R fails to surround  with newlines
    ;; (for more consistent surround and snipe keys)
    (evil-define-key 'visual evil-surround-mode-map
      "r" 'evil-surround-region
      "R" 'evil-Surround-region)
    (evil-define-key 'operator evil-surround-mode-map
      "r" 'evil-surround-edit
      "R" 'evil-Surround-edit)
    (evil-define-key 'operator evil-surround-mode-map
      "s" nil
      "S" nil)
    ;; stock r and R aren't all that useful--can just use cl/h instead of r, and
    ;; the apex of R's usefulness is on the level of replacing an existing hex
    ;; color with white... no?
    (evil-define-key 'normal evil-surround-mode-map
      "r" 'evil-surround-edit
      "R" 'evil-Surround-edit))

[ninrod]

shouldn't you place all of those in a :config block? seems like those would be clobbered when evil-surround actually loads. Or perhaps use with-eval-after-load 'evil-surround?

How are you getting evil-surround with no :ensure t? Why no (global-evil-surround-mode 1)?

Just curious.

[braham-snyder]

shouldn't you place all of those in a :config block? seems like those would be clobbed when evil-surround actually loads. Or perhaps use with-eval-after-load 'evil-surround?

@ninrod I was just lazy and fortunate: according to https://www.reddit.com/r/emacs/comments/4e4ja5/lazy_loading_leader_bindings_with_usepackage/d1xfvyk/ evil-define-key automatically defers its bindings until the given map actually exists.

How are you getting evil-surround with no :ensure t?

I'm using Spacemacs, which happens to include evil-surround by default--though you might find (setq use-package-always-ensure t) useful.

Why no (global-evil-surround-mode 1)?

Spacemacs sets it elsewhere by default.

[ninrod]

@braham-snyder, thanks. Now about that issue of yours, I have some ideas, but please open a separate issue for that.

[wbolster]

thanks for NOT mentioning me and blindly reverting my commits.

the "fix" which reverts a commit without knowing why that was done reintroduces another issue: https://github.com/timcharper/evil-surround/pull/81

[ninrod]

sorry @wbolster, I'm new to all these mantainance stuff.

can you re-submit a pull that does not reintroduces #24 as a reggression?

thanks.

[eeshugerman]

Anyone know the status of this? Repeat doesn't work for me (in Spacemacs. -- haven't tested in vanilla Emacs). It's unclear to me from the thread above if this is a known issue at this point? It looks like the commit that introduced this regression was reverted, so I guess it's regressed again?

[ninrod]

It is working fine, at least to my knowledge. You can confirm with a clean environment using make emacs (see instructions in the readme).

[eeshugerman]

Thanks @ninrod -- I'm seeing the same behavior with make emacs as with my normal install, but I now realize that repeat mostly works fine, but not for yst<some-char><other-char>. I'll look into it some more, might open another issue with my findings.

[ninrod]

Thanks @ninrod -- I'm seeing the same behavior with make emacs as with my normal install, but I now realize that repeat mostly works fine, but not for yst. I'll look into it some more, might open another issue with my findings.

Interesting. Keep me posted, please.