emacs-evil / evil-surround

you will be surrounded (surround.vim for evil, the extensible vi layer)
Other
631 stars 61 forks source link

Fix visible narrowing when surrounding within a field #135

Closed lionel- closed 6 years ago

lionel- commented 6 years ago

In operator state the buffer is narrowed to the current Emacs field. This narrowing becomes visible when waiting for user input which is quite disruptive of the workflow. This patch widens the buffer before reading a character to avoid this.

For some context see https://github.com/emacs-evil/evil/issues/294 and https://gitorious.org/evil/cofis-evil/commit/bcf0bac?p=evil:cofis-evil.git;a=blobdiff;f=evil-types.el;h=3d963fb271013319566e923cde210af616369e0c;hp=b850569f868e994cb04236fb0f1ce614d230a722;hb=bcf0bac;hpb=ef4f5b8a74199731b08e0737027d7cb92c9e2a77.

ninrod commented 6 years ago

Hi, thanks

Couple of issues:

lionel- commented 6 years ago

this seems to me like a problem in evil itself and the first link points to an issue fixed in 2014 in evil,

I don't think so because not narrowing would be a bug in evil. The issue is in any operator client code that waits for input without widening, as in evil-surround.

The second link is giving me 404.

That's weird, I just tried it again and it works for me.

I'm not able to reproduce the core problem of seeing a narrowed buffer while waiting for input in operator pending state.

Try M-x shell and make sure that comint-use-prompt-regexp-instead-of-fields is nil. Now fill the shell buffer with some output, and try to use evil-surround at the prompt. You should see the narrowing that this patch fixes.

lionel- commented 6 years ago

That's weird, I just tried it again and it works for me.

Oh maybe that's from the trailing .? Github excludes it from the clickable link but you might have included it in a copy/paste?

ninrod commented 6 years ago

Ok, very interesting. Now, why does this only happen in M-x shell and not in a "normal" buffer? I'm using M-x multi-term and this does not happen there. What is exactly an emacs Field?

ninrod commented 6 years ago

The tests failed because Travis couldn't install emacs on the container.

lionel- commented 6 years ago

Fields are a mechanism to give hard boundaries to the Emacs cursor (point). For instance that's what makes it possible to use evil line commands without affecting the prompt in comint-derived modes. See https://www.gnu.org/software/emacs/manual/html_node/elisp/Fields.html

ninrod commented 6 years ago

@lionel-, so you are saying that if the current mode derives from comint mode, then this problem appears? Is that right?

Very interesting. I was using multi-term and to edit the line in zsh's bindkey -v mode I had to resort to an emacs state hack. In M-x shell I can edit the line as if it was a normal buffer, but I lose auto-completion.

lionel- commented 6 years ago

so you are saying that if the current mode derives from comint mode, then this problem appears?

yup and that's a major bother for me because I use R for a living and evil-surround has kept glitching the REPL for years ;) Quite happy to finally have found the source of this bug!

ninrod commented 6 years ago

Humm.. I've just tried out your patch and it does not solve the problem. The buffer still narrows:

evil-surround ❯ pwd
/var/workbench/evil-surround
evil-surround ❯ grep -i evil-surround < ~/.emacs.d/boot.org 
*** evil-surround
  (use-package evil-surround
    :load-path "/var/workbench/evil-surround"
    (with-eval-after-load 'evil-surround
       'evil-surround-pairs-alist ;; use non-spaced pairs when surrounding with 
an opening brace evil-surround/issues/86                                       
    :config (global-evil-surround-mode 1))
    (evil-embrace-enable-evil-surround-integration))
evil-surround ❯ git status -sb
## fix-operator-narrowing...lionel/fix-operator-narrowing
evil-surround ❯ git remote show lionel| head -n2
* remote lionel
  Fetch URL: https://github.com/lionel-/evil-surround.git
evil-surround ❯ 
ninrod commented 6 years ago

Do you think you could write a test for this? I imagine that you could write a test which inspected the buffer after issuing typing ys.

for me, with your patch, after M-x shell, type foo, <ESC>ys the buffer narrows.

ninrod commented 6 years ago

hi @lionel-

are we still moving forward with this patch?

lionel- commented 6 years ago

Yup sorry I'll try to find some time... I'm using my local branch without issues so I tend to forget about it but it'd be nice to eventually merge the patch here indeed.

ninrod commented 6 years ago

Ok, do you think you could write some tests for this?

ninrod commented 6 years ago

@lionel- are you still interested in the patch?

lionel- commented 6 years ago

yeah, sorry, I've been really busy at work and with ESS lately. I'll try to come up with some tests.

ninrod commented 6 years ago

@lionel- awesome. The tests are already passing so your patch is ready to merge. I'm just trying to capture the problem in a test so that we protect against possible regressions.

Now if you find that this specific problem is too troublesome to test, just post the rationale and we'll merge anyway.

lionel- commented 6 years ago

Sounds good, I'll take a look this weekend. I've been using this branch ever since so I know it works well, but a test would be of course better.

lionel- commented 6 years ago

I added some tests.

Could there be something wrong in your test suite? The following test is broken locally and on Travis: "function surrounding with dot repeat", but it does not cause Travis to fail.

It is unrelated to the PR in any case.

ninrod commented 6 years ago

didn't notice that, I'll take a look.

ninrod commented 6 years ago

funny. when you run the test inside emacs, all is good. when you run the tests through make test, this test you mention breaks.

ninrod commented 6 years ago

Ok, so I've found the culprit: commit 5a20c97. I've also opened #146 because the test machinery should have warned about the test test failure introduced on commit 5a20c97

ninrod commented 6 years ago

@lionel-, can you rebase your commits on top of a92151d and see if it solves the issue?

lionel- commented 6 years ago

Now rebased. It looks like all tests now pass normally!

ninrod commented 6 years ago

Looks good to me!