visual-line-mode still breaks evil-surround about evil-surround HOT 26 CLOSED

Fixed in #174.

from evil-surround.

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.

from evil-surround.

@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.

from evil-surround.

@MintSoup , thank you for your contribution.

Can you write a test for this scenario?

from evil-surround.

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

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

from evil-surround.

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

asd
fge
abc

select lines visually and press Sb:

(
asd
fge
abc
)

from evil-surround.

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?

from evil-surround.

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.

from evil-surround.

@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.

from evil-surround.

@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?

from evil-surround.

@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.

from evil-surround.

@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.

from evil-surround.

@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?

from evil-surround.

What am I doing wrong?

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

from evil-surround.

@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)"

from evil-surround.

@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
)

from evil-surround.

@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.

from evil-surround.

@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)"

from evil-surround.

@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.

from evil-surround.

@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)"

from evil-surround.

@ninrod Great. Ready to merge then I guess?

from evil-surround.

@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.

from evil-surround.

fixed in 3bd7379

from evil-surround.

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

from evil-surround.

@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.

from evil-surround.

@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).

from evil-surround.