spacemacs icon indicating copy to clipboard operation
spacemacs copied to clipboard

tracking issue for evil-surround

Open lebensterben opened this issue 2 years ago • 14 comments

I also think it makes sense not to turn on evil-surround globally.

Specifically, it can be turned on for all text-mode and prog-mode. Or in other words, specialm-mode won't be affected.

This should be fine in most use cases. Except for a few specfic special-mode such as various terminal, inferior emacs lisp, inferior python, etc. But then evil-surround can be turned on when needed.

Originally posted by @lebensterben in https://github.com/syl20bnr/spacemacs/issues/15448#issuecomment-1090810486

lebensterben avatar Apr 11 '22 18:04 lebensterben

The commit in #15462 does not seem to fix the original issue (#15448). I have created a PR upstream to fix the issue there.

B.t.w. they are looking for a new maintainer.

dalanicolai avatar Apr 16 '22 15:04 dalanicolai

my intent is not to modify that global minor mode, but simply add major mode hook to text-mode and prog-mode to enable evil-surround.

making the global minor mode disabled for special-mode works, but can be counterintuitive for some people.

lebensterben avatar Apr 16 '22 16:04 lebensterben

I think the :predicate option is nicer, you can set and exclude modes all from a single list, as you can find in the docs. This could be consistently used for all 'globalizes-minor-modes'. I would prefer to use that mechanism everywhere and document it well.

Well, the upstream fix is useful anyway, and will fix the issue in Spacemacs (as long as the globalized mode is active).

dalanicolai avatar Apr 16 '22 21:04 dalanicolai

you can also add comint-mode.

lebensterben avatar Apr 16 '22 22:04 lebensterben

The commit in #15462 does not seem to fix the original issue (#15448).

@dalanicolai Do you mean that you're still unable to stage line-wise in magit-status buffers using s because evil-surround intercepts the keystroke? If so, I'm surprised since that commit did fix things for me, but perhaps there's something esoteric about my .spacemacs config that might explain why it fixes things for me but not others.

Or do you mean that the commit in #15462 does not solve the ostensibly deeper issue/design choice of just how "global" evil-surround should be?

I'm glad that you've opened the PR upstream as I agree that what you've proposed is likely a more sensible behavior in order to get evil-surround to play nicer with other packages. I just want to make sure that my fix in #15462 indeed fixed the titular issue of #15448, i.e., that it was no longer possible to stage line-wise in magit-status using s. If your PR is later accepted, my fix could be reverted.

dankessler avatar Apr 18 '22 16:04 dankessler

@dankessler Unfortunately, it was indeed the case that the commit did not fix the issue of evil-surround intercepting the stage line-wise keystroke. As I did not modify a lot in my dotfile, I guess ineed it does not fix it for most users, but I did not try to understand why it did not fix it. Instead, I went looking for some 'default' mechanism to exclude certain modes from global(ized) minor modes. Then I found this solution, noted that it fixes the original issue https://github.com/syl20bnr/spacemacs/issues/15448 and so I created the PR upstream. Although your PR did not work for me, still thanks, of course, for trying to fix it...

dalanicolai avatar Apr 18 '22 18:04 dalanicolai

@dalanicolai Hmm, very odd indeed. Out of curiosity, I reset my spacemacs repo to the commit where I introduced the fix, and then I reverted my local .spacemacs to the template, uncommented the git layer, and then relaunched. The fix still works for me, i.e., I can move point to a line of the diff in magit-status, hit v, and then hit s and see just that line staged. Moreover, I can see that evil-surround-mode is nil in my magit-status buffer, but t in other buffers. So, perhaps there is something esoteric in your config that prevents the fix from working for you.

Either way, I agree that it's ultimately better to get it sorted out upstream (thanks again for doing the PR!), but this experiment leads me to suspect/hope that my somewhat hacky fix should restore the desired behavior for most users in the meantime.

dankessler avatar Apr 18 '22 18:04 dankessler

@dankessler I have tried a similar thing now as you did, and then indeed your fix seems to be working. But anyway, for some reason it does not work on my 'personal default' configuration (despite I'm using the git layer).

On the other hand, looking a little more into it, your fix does not look 'correct' to me, because evil-surround is already declared in the spacemacs-evil layer. I don't think it should be declared in the git layer also. Instead, I think you should have 'configured' the magit package (probably), using the :config keyword with a with-eval-after-load or something like that (I think; maybe the post-config trick is fine also, but I have never used that, and I don't know if it is possible to use it without declaring the package first).

Well, in the end, your fix is working for now (for you and probably most others). And in the end the upstream fix will also work for my personal configuration (I hope it will get merged at some point, the package is looking for a new maintainer, otherwise we might have to fork it).

dalanicolai avatar Apr 19 '22 05:04 dalanicolai

On the other hand, looking a little more into it, your fix does not look 'correct' to me, because evil-surround is already declared in the spacemacs-evil layer. I don't think it should be declared in the git layer also.

it appears in git-packages because there's post-init-evil-surround. the owner is still spacemacs-evil.

lebensterben avatar Apr 19 '22 06:04 lebensterben

Ah okay, well I don't fully understand these mechanisms. I read here that the list should be a list of packages that the layer needs. And I guess this package is not needed when users choose to use Emacs keybindings. Therefore, I would have chosen for the with-eval-after-load, but as I said, I don't fully understand these mechanisms. It would be great if the documentation explained a bit better why they are needed (e.g. with some examples), and when packages should/should not be declared in multiple layers...

dalanicolai avatar Apr 19 '22 06:04 dalanicolai

I did read again section 6 now, but I still find it somewhat confusing, as I think most/all of these things could be achieved with with-eval-after-load (at least for the post-init functions). Also, evil-surround has been added to two layers now, but SPC h SPC returns only one result for evil-surround. Well... as long as it works it's fine (although in this case the fix, for some reason, did not work for me)

dalanicolai avatar Apr 19 '22 06:04 dalanicolai

SPC h SPC returns only one result for evil-surround

that's not reliable at all. ivy and helm show different results.

lebensterben avatar Apr 19 '22 06:04 lebensterben

The upstream-fix has been merged...

dalanicolai avatar May 03 '22 11:05 dalanicolai

...and reverted, sadly. See PR for details.

tomdl89 avatar May 04 '22 08:05 tomdl89

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please let us know if this issue is still valid!

github-actions[bot] avatar May 04 '23 08:05 github-actions[bot]