visual-line-mode still breaks evil-surround

[MintSoup]

It seems that this issue was supposed to be fixed in #71, but I still have some problems.

yss will place the closing pair on the next line and also not ignore leading whitespaces if visual-line-mode is enabled. Demonstartion:

      test123

Now, we yss on this line, and get

(      test123
)       

Disabling visual-line

[MintSoup]

Fixed in #174.

[ninrod]

@MintSoup , thank you for your contribution.

Can you write a test for this scenario?

[ninrod]

@MintSoup, I'm having problems trying to reproduce your issue.

What do you mean exactly by if visual-line-mode is enabled?

[ninrod]

by the way, the case described in #71 is working fine:

asd
fge
abc

select lines visually and press Sb:

(
asd
fge
abc
)

[ninrod]

In you case:

test123

going in with yssb

(test123)

if visually selecting test123 and then pressing Sb:

(
test123
)

which is what you want. This is consistent with vim-surround. I don't see the problem, can you clarifly?

[ninrod]

By the way, I would like you to try to reproduce the problem using make emacs to exclude any issues that could arise from differences in custom emacs configurations.

[MintSoup]

@ninrod I'm sorry, my description wasn't clear enough. I did not mean visual selection, that works perfectly fine.

Visual line mode is a minor mode. It splits up lines too long to displayed into multiple visual lines. It does not insert real newlines into the file like fill-paragraph, only changes the rendering. Word wrap, basically.

Evil has an option called evil-respect-visual-line-mode, which makes various actions like movement with hjkl, $ and 0, among other things, work on the visual line as it is displayed on the screen, not the real line in the file.

The problem arises when visual-line-mode is enabled AND evil-respect-visual-line-mode is t.

Please enable visual line mode with M-x visual-line-mode RET, set evil-respect-visual-line-mode to t and use yssb on a multi-line file. The line doesn't need to be broken up by visual-line-mode, even regular, short lines suffer from this problem. You will see that the line doesn't get surrounded correctly.

From what I understand, when evil-respect-visual-line-mode is enabled, the argument type of the operator evil-surround-region (defined on line 365) will be screen-line , not line. This makes the cond fall back to the default case, which is why the line doesn't get surrounded properly. Other motions work fine, it's just yss that is broken.

My patch adds a new case to the cond for when type is screen-line, which surrounds the current visual line.

Also, I just noticed that my and condition checking for evil-respect-visual-line-mode is unnecessary, as type will be line anyway when that's disabled. I'll try to remove it today.

[ninrod]

@MintSoup , thank you. Understood. But now the problem is that I can't reproduce even this:

Evil has an option called evil-respect-visual-line-mode, which makes various actions like movement with hjkl, $ and 0, among other things, work on the visual line as it is displayed on the screen, not the real line in the file.

Here is my updated make emacs command which I'm using to try to reproduce your case:

emacs ?= emacs
bemacs = $(emacs) -batch -l test/elpa.el
LIBS = -L evil -L evil/lib -l evil.el -l goto-chg.el -l undo-tree.el -L . -l evil-surround.el -L test -l evil-surround-test.el

emacs:
    $(emacs) -Q $(LIBS) \
    --eval "(setq evil-respect-visual-line-mode t)" \
    --eval "(evil-mode 1)" \
    --eval "(global-evil-surround-mode 1)"
    --eval "(visual-line-mode)"

Emacs does open up with visual-line-mode enabled, the variable evil-respect-visual-line-mode is indeed set to t, confirmed by M-x describle-variable, but the behaviour of jkhl is still standard. This is using latest version of evil.

What am I doing wrong?

[MintSoup]

@ninrod I'm actually not sure. I can't even get make emacs to load evil correctly. Please try running make emacs normally, then enable visual-line-mode and evil-respect-visual-line-mode manually.

[ninrod]

@MintSoup , you have to issue make first. I can enable visual-line-mode from the load up, but evil-respect-visual-line-mode, even with set-variable does not get recognized by evil. I cannot make this work.

[MintSoup]

@ninrod In the docstring, it says that evil-respect-visual-line-mode needs to be set before evil is loaded. In my private config I do this in the :init clause of use-package, but I'm not sure how you can do that in our test environment.

After running make, make emacs doesn't enable evil by default, I have to do it manually, and I can't enable evil-surround at all. On emacs startup it tells me that it can't find undo-tree.el. What am I doing wrong?

[ninrod]

What am I doing wrong?

Hum, you should pull my last commit, it fixes a dependency bug related to undo-tree.el.

[MintSoup]

@ninrod Done, now everything works fine for me. I can reproduce your issue of evil-respect-visual-line-mode of not affecting normal evil commands. The problem is that we're loading evil before setting it, which is why it isn't working.

This modified Makefile, while not very pretty, should fix it.

emacs ?= emacs
bemacs = $(emacs) -batch -l test/elpa.el
LIBS = -L evil -L evil/lib

emacs:
    $(emacs) -Q $(LIBS) \
    --eval "(setq evil-respect-visual-line-mode t)" \
    --eval "(load-file \"evil/evil.el\")" \
    --eval "(load-file \"evil/lib/goto-chg.el\")" \
    --eval "(load-file \"evil-surround.el\")" \
    --eval "(visual-line-mode)" \
    --eval "(evil-mode 1)" \
    --eval "(global-evil-surround-mode 1)"

[ninrod]

This modified Makefile, while not very pretty, should fix it.

Awesome! Thank you for this. Now we have "common ground". I'll proceed with testing this week, hopefully.

[ninrod]

@MintSoup

I got some minutes now to reproduce the problem with your modified makefile:

I'm using evil-surround's master.

So back to the first post on this thread, [cursor is here]:

   [t]est123

yssb turns this into:

[(]   test123)

This is different than what you are getting, which you said was this:

(      test123
)

[MintSoup]

@ninrod That's what I was getting too, if its the only line in the buffer. If you have a few lines around test123, you should get the second result.

[ninrod]

@ninrod That's what I was getting too, if its the only line in the buffer. If you have a few lines around test123, you should get the second result.

Ok, will try that later.

[ninrod]

@MintSoup I think I've found something odd here.

Are you using lisp-interaction-mode? because if I issue M-x text-mode the problem disappears in master.

Even more odd: if you go to text-mode and back to lisp-interaction-mode, the problem vanishes.

what gives?

edit:

in fact, if you modify your Makefile to this, using master, you will not be able to reproduce your problem: (can you confirm?)

emacs ?= emacs
bemacs = $(emacs) -batch -l test/elpa.el
LIBS = -L evil -L evil/lib

emacs:
    $(emacs) -Q $(LIBS) \
    --eval "(setq evil-respect-visual-line-mode t)" \
    --eval "(load-file \"evil/evil.el\")" \
    --eval "(load-file \"evil/lib/goto-chg.el\")" \
    --eval "(load-file \"evil-surround.el\")" \
    --eval "(visual-line-mode)" \
    --eval "(evil-mode 1)" \
    --eval "(global-evil-surround-mode 1)" \
    --eval "(text-mode)"

[MintSoup]

@ninrod, yes, with your Makefile the issue vanishes. However, visual-line-mode doesn't seem to actually get enabled this way (please confirm), for some reason. If I enable it with M-x visual-line-mode, the issue still persists. I have never touched lisp-interaction-mode. It is very unlikely that it has anything to do with the major mode, because this isn't really a bug per se, but rather an incomplete cond expression, missing the block for screen-line.

[ninrod]

@ninrod, yes, with your Makefile the issue vanishes. However, visual-line-mode doesn't seem to actually get enabled this way (please confirm), for some reason. If I enable it with M-x visual-line-mode, the issue still persists. I have never touched lisp-interaction-mode. It is very unlikely that it has anything to do with the major mode, because this isn't really a bug per se, but rather an incomplete cond expression, missing the block for screen-line.

Confirmed:

with this makefile, the issue persists even with text-mode enabled. I think switching modes disables visual-line-mode.

emacs ?= emacs
bemacs = $(emacs) -batch -l test/elpa.el
LIBS = -L evil -L evil/lib

emacs:
    $(emacs) -Q $(LIBS) \
    --eval "(setq evil-respect-visual-line-mode t)" \
    --eval "(load-file \"evil/evil.el\")" \
    --eval "(load-file \"evil/lib/goto-chg.el\")" \
    --eval "(load-file \"evil-surround.el\")" \
    --eval "(text-mode)" \
    --eval "(visual-line-mode)" \
    --eval "(evil-mode 1)" \
    --eval "(global-evil-surround-mode 1)"

[MintSoup]

@ninrod Great. Ready to merge then I guess?

[ninrod]

@ninrod Great. Ready to merge then I guess?

yes. All the tests are running ok and I did not find any odd behaviour in my exploratory tests using your branch.

[ninrod]

fixed in https://github.com/emacs-evil/evil-surround/commit/3bd73794ee5a760118042584ef74e2b6fb2a1e06

[ninrod]

@MintSoup , can you check if everything is in order?

[MintSoup]

@ninrod Pulling a fresh one from master I get the version with my changes, looks like everything is fine. Although there seems to be some weirdness around the git/PR stuff - on my repo it says it's 2 commits ahead and 2 behind master. Isn't it supposed to say it's even? Did it merge correctly? Sorry, this is my first time contributing upstream on github, not sure how it's supposed to work.

[ninrod]

@MintSoup, that's fine. If you check your branch against master, you'll see that I've pushed a commit to master and also squashed your 2 commits into one and pushed that into master too. That's why you are 2 behind and two ahead (these 2 ahead equals to the one I created squashing into one commit).