"Marker points into wrong buffer" with helm-mark-ring and Evil #1891

Ambrevar · 2017-10-09T16:42:57Z

This issues has already been reported on Evil's issue tracker emacs-evil/evil#845 but no progress was made so far. It is not clear whether the issue comes from Helm, Evil, or an incompatibility between both. Maybe you can help.

Expected behavior

With follow-mode on, navigate through the different marks.

Actual behavior (from `emacs-helm.sh` if possible, see note at the bottom)

Some entries follow up to seemingly random locations and an error message is displayed:

Marker points into wrong buffer

Steps to reproduce (recipe)

Install latest Evil.
Start Emacs, require helm and evil. Update: No need to turn on evil mode.
~~Use some vim/Evil movements (e.g. { and } over a sequence of paragraphs) to populate the mark ring.~~ Update: Make two different marks with C-SPC.
Call helm-mark-ring and make sure you have at least 2 entries.
Enable helm-follow-mode with C-c C-f.
Move around the Helm entries up and down until you get the error.

Backtraces if any (`M-x toggle-debug-on-error`)

Debugger entered--Lisp error: (error "Marker points into wrong buffer" #<marker in no buffer>)
  helm-goto-char(#<marker in no buffer>)
  helm-mark-ring-default-action(#<marker in no buffer>)
  helm-execute-selection-action-1(#<marker in no buffer> (("Goto line" . helm-mark-ring-default-action)) t)
  helm-execute-persistent-action()
  #[0 "�\205��\301 \207" [helm-alive-p helm-execute-persistent-action] 1]()
  apply(#[0 "�\205��\301 \207" [helm-alive-p helm-execute-persistent-action] 1] nil)
  timer-event-handler([t 0 0 10000 nil #[0 "�\205��\301 \207" [helm-alive-p helm-execute-persistent-action] 1] nil idle 0])
  read-from-minibuffer("pattern: " <TRUNCATED>
  helm-read-pattern-maybe(nil nil nil noresume nil nil nil)
  helm-internal((helm-source-mark-ring helm-source-global-mark-ring) nil nil noresume nil "*helm mark ring*" nil nil nil)
  apply(helm-internal ((helm-source-mark-ring helm-source-global-mark-ring) nil nil noresume nil "*helm mark ring*" nil nil nil))
  helm((helm-source-mark-ring helm-source-global-mark-ring) nil nil noresume nil "*helm mark ring*" nil nil nil)
  apply(helm ((helm-source-mark-ring helm-source-global-mark-ring) nil nil noresume nil "*helm mark ring*" nil nil nil))
  helm(:sources (helm-source-mark-ring helm-source-global-mark-ring) :resume noresume :buffer "*helm mark ring*")
  helm-all-mark-rings()
  (if rectangle-mark-mode (rectangle-exchange-point-and-mark) (helm-all-mark-rings))
  helm-mark-or-exchange-rect()
  funcall-interactively(helm-mark-or-exchange-rect)
  call-interactively(helm-mark-or-exchange-rect nil nil)
  command-execute(helm-mark-or-exchange-rect)

Content of the mark-ring:

(#<marker at 190 in bar.c> #<marker at 143 in bar.c> #<marker at 190 in bar.c> #<marker at 143 in bar.c> #<marker at 190 in bar.c> #<marker at 23 in bar.c> #<marker at 190 in bar.c> #<marker at 143 in bar.c> #<marker at 190 in bar.c> #<marker at 143 in bar.c> #<marker at 190 in bar.c> #<marker at 23 in bar.c> #<marker at 190 in bar.c> #<marker at 143 in bar.c> #<marker at 245 in bar.c> #<marker at 245 in bar.c>)

Describe versions of Helm, Emacs, operating system, etc.

Helm: MELPA
Evil: MELPA
OS: Arch Linux, Gentoo, Ubuntu...

Are you using `emacs-helm.sh` to reproduce this bug (yes/no):

With Evil activated.

The text was updated successfully, but these errors were encountered:

thierryvolpiatto · 2017-10-10T04:40:25Z

Pierre Neidhardt <[email protected]> writes:

This issues has already been reported on Evil's issue tracker emacs-evil/evil#845 but no progress was made so far. It is not clear whether the issue comes from Helm, Evil, or an incompatibility between both. Maybe you can help. Expected behavior With follow-mode on, navigate through the different marks. Actual behavior (from emacs-helm.sh if possible, see note at the bottom) (if rectangle-mark-mode (rectangle-exchange-point-and-mark) (helm-all-mark-rings)) helm-mark-or-exchange-rect() funcall-interactively(helm-mark-or-exchange-rect)

What is `helm-mark-or-exchange-rect`?

…

-- Thierry

Ambrevar · 2017-10-10T09:07:36Z

Sorry, that was the trace from a non-vanilla session. Here is the vanilla trace:

(error "Marker points into wrong buffer" #<marker in no buffer>)
  helm-goto-char(#<marker in no buffer>)
  helm-mark-ring-default-action(#<marker in no buffer>)
  helm-execute-selection-action-1(#<marker in no buffer> (("Goto line" . helm-mark-ring-default-action)) t)
  helm-execute-persistent-action()
  #[0 "�\205�^@\301 \207" [helm-alive-p helm-execute-persistent-action] 1]()
  apply(#[0 "�\205�^@\301 \207" [helm-alive-p helm-execute-persistent-action] 1] nil)
  timer-event-handler([t 0 0 10000 nil #[0 "�\205�^@\301 \207" [helm-alive-p helm-execute-persistent-action] 1] nil idle 0])
  read-from-minibuffer("pattern: " nil (keymap (f13 lambda nil (interactive) (helm-select-nth-action 12)) (f12 lambda nil (interactive) (helm-select-nth-action 11)) (f11 lambda nil (interactive) (helm-select-nth-action 10)) (f10 lambda nil (interactive) (helm-select-nth-action 9)) (f9 lambda nil (interactive) (helm-select-nth-action 8)) (f8 lambda nil (interactive) (helm-select-nth-action 7)) (f7 lambda nil (interactive) (helm-select-nth-action 6)) (f6 lambda nil (interactive) (helm-select-nth-action 5)) (f5 lambda nil (interactive) (helm-select-nth-action 4)) (f4 lambda nil (interactive) (helm-select-nth-action 3)) (f3 lambda nil (interactive) (helm-select-nth-action 2)) (f2 lambda nil (interactive) (helm-select-nth-action 1)) (menu-bar keymap (help-menu keymap (describe keymap (describe-mode . helm-help)))) (help keymap (109 . helm-help)) (23 . #[0 "\306\307\310\311\312\305!\313\"\314$\216\315\302!\210\316\304!\317\211\211����\301=\203'^@\315\302!\210\320\202]^@\321�\303\"\211\262�\203:^@\315�A!\210\320\202]^@\312��!\262�\322\300�\"\262�\323�!\203S^@\315�!\210\202\\^@\324\325�\"��\244��\317\266\204\205e^@\202�^@)\207" [#0 23 helm-yank-text-at-point ((31 . helm-undo-yank-text-at-point)) nil nil make-byte-code 0 "\300\205�^@\300 \207" vconcat vector [] 1 call-interactively read-key nil t assoc lookup-key commandp mapcar identity last-command-event unread-command-events] 7 nil nil]) (f1 lambda nil (interactive) (helm-select-nth-action 0)) (8 keymap (109 . helm-help) (104 . undefined) (8 . undefined) (99 . helm-customize-group) (4 . helm-enable-or-switch-to-debug)) (20 . helm-toggle-resplit-and-swap-windows) (C-tab . undefined) (67108897 . helm-toggle-suspend-update) (3 keymap (57 . helm-execute-selection-action-at-nth-+9) (56 . helm-execute-selection-action-at-nth-+8) (55 . helm-execute-selection-action-at-nth-+7) (54 . helm-execute-selection-action-at-nth-+6) (53 . helm-execute-selection-action-at-nth-+5) (52 . helm-execute-selection-action-at-nth-+4) (51 . helm-execute-selection-action-at-nth-+3) (50 . helm-execute-selection-action-at-nth-+2) (49 . helm-execute-selection-action-at-nth-+1) (63 . helm-help) (110 . #[0 "\306\307\310\311\312\305!\313\"\314$\216\315\302!\210\316\304!\317\211\211����\301=\203'^@\315\302!\210\320\202]^@\321�\303\"\211\262�\203:^@\315�A!\210\320\202]^@\312��!\262�\322\300�\"\262�\323�!\203S^@\315�!\210\202\\^@\324\325�\"��\244��\317\266\204\205e^@\202�^@)\207" [#0 110 helm-run-cycle-resume nil nil nil make-byte-code 0 "\300\205�^@\300 \207" vconcat vector [] 1 call-interactively read-key nil t assoc lookup-key commandp mapcar identity last-command-event unread-command-events] 7 nil nil]) (62 . helm-toggle-truncate-line) (21 . helm-refresh) (6 . helm-follow-mode) (9 . helm-copy-to-buffer) (11 . helm-kill-selection-and-quit) (25 . helm-yank-selection) (45 . helm-swap-windows)) (67108987 . helm-enlarge-window) (67108989 . helm-narrow-window) (19 . undefined) (24 keymap (57 . helm-execute-selection-action-at-nth-+9) (56 . helm-execute-selection-action-at-nth-+8) (55 . helm-execute-selection-action-at-nth-+7) (54 . helm-execute-selection-action-at-nth-+6) (53 . helm-execute-selection-action-at-nth-+5) (52 . helm-execute-selection-action-at-nth-+4) (51 . helm-execute-selection-action-at-nth-+3) (50 . helm-execute-selection-action-at-nth-+2) (49 . helm-execute-selection-action-at-nth-+1) (2 . helm-resume-list-buffers-after-quit) (98 . helm-resume-previous-session-after-quit) (6 . helm-quit-and-find-file)) (11 . helm-delete-minibuffer-contents) (67108896 . helm-toggle-visible-mark) (0 . helm-toggle-visible-mark) (C-M-up . helm-scroll-other-window-down) (C-M-down . helm-scroll-other-window) (M-prior . helm-scroll-other-window-down) (M-next . helm-scroll-other-window) (12 . helm-recenter-top-bottom-other-window) (15 . helm-next-source) (10 . helm-execute-persistent-action) (26 . helm-execute-persistent-action) (9 . helm-select-action) (13 . helm-maybe-exit-minibuffer) (left . helm-previous-source) (right . helm-next-source) (7 . helm-keyboard-quit) (22 . helm-next-page) (27 keymap (110 . next-history-element) (112 . previous-history-element) (115 . undefined) (5 . helm-display-all-sources) (1 . helm-show-all-in-this-source-only) (85 . helm-unmark-all) (97 . helm-mark-all) (109 . helm-toggle-all-marks) (41 . helm-next-visible-mark) (40 . helm-prev-visible-mark) (91) (32 . helm-toggle-visible-mark) (33554454 . helm-scroll-other-window-down) (25 . helm-scroll-other-window-down) (22 . helm-scroll-other-window) (12 . helm-reposition-window-other-window) (111 . helm-previous-source) (62 . helm-end-of-buffer) (60 . helm-beginning-of-buffer) (118 . helm-previous-page)) (next . helm-next-page) (prior . helm-previous-page) (C-up . helm-follow-action-backward) (C-down . helm-follow-action-forward) (16 . helm-previous-line) (14 . helm-next-line) ...) nil nil nil t)
  helm-read-pattern-maybe(nil nil nil noresume nil nil nil)
  helm-internal((helm-source-mark-ring helm-source-global-mark-ring) nil nil noresume nil "*helm mark ring*" nil nil nil)
  apply(helm-internal ((helm-source-mark-ring helm-source-global-mark-ring) nil nil noresume nil "*helm mark ring*" nil nil nil))
  helm((helm-source-mark-ring helm-source-global-mark-ring) nil nil noresume nil "*helm mark ring*" nil nil nil)
  apply(helm ((helm-source-mark-ring helm-source-global-mark-ring) nil nil noresume nil "*helm mark ring*" nil nil nil))
  helm(:sources (helm-source-mark-ring helm-source-global-mark-ring) :resume noresume :buffer "*helm mark ring*")
  helm-all-mark-rings()
  funcall-interactively(helm-all-mark-rings)
  call-interactively(helm-all-mark-rings record nil)
  command-execute(helm-all-mark-rings record)
  execute-extended-command(nil "helm-all-mark-rings" nil)
  funcall-interactively(execute-extended-command nil "helm-all-mark-rings" nil)
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)

I realized something else: I don't need to turn Evil on to generate the issue, and I don't even need any other mark than a few marks created with C-SPC.

My init.el:

(require 'helm-config)
(require 'evil)

thierryvolpiatto · 2017-10-11T07:24:03Z

Pierre Neidhardt <[email protected]> writes:

I realized something else: I don't need to turn Evil on to generate the issue

But evil is loaded, right? Can you reproduce without evil loaded? Here without evil (I don't use this) I can't reproduce, seems evil is doing something wrong with marks to end up with #<marker in no buffer>?

…

-- Thierry

Ambrevar · 2017-10-11T08:02:52Z

Evil is loaded with require, but it's the evil mode is not on, i.e. the keybindings remain Emacs bndings.
I cannot reproduce without Evil either (which is why I opened the issue on Evil's bugtracker first).
The possibilities are narrowed down: it must come from some incompatibility between Helm and the code Evil runs/loads when it's required.

Ambrevar · 2017-10-23T08:19:27Z

I also noticed that after I see the error message, I see that the mark-ring gets corrupted when I check its value: some elements get replaced by other elements of the ring, so that in the end there are a lot of duplicates. Example:

(#<marker at 3736 in todo.org> #<marker at 3736 in todo.org> #<marker at 3736 in todo.org> #<marker at 3736 in todo.org> #<marker at 3736 in todo.org> #<marker at 3736 in todo.org> #<marker at 3736 in todo.org> #<marker at 3736 in todo.org> #<marker at 3736 in todo.org> #<marker at 3736 in todo.org> #<marker at 3696 in todo.org> #<marker at 338 in todo.org> #<marker at 3696 in todo.org> #<marker at 338 in todo.org> #<marker at 3696 in todo.org> #<marker at 3736 in todo.org>)

thierryvolpiatto · 2017-10-23T09:31:06Z

Pierre Neidhardt <[email protected]> writes:

I also noticed that the mark-ring gets corrupted: some elements get replaced by other elements of the ring, so that in the end there are a lot of duplicates.

Not here without using evil: (#<marker at 11803 in helm-ring.el> #<marker at 11240 in helm-ring.el> #<marker at 10979 in helm-ring.el> #<marker at 10717 in helm-ring.el> #<marker at 10337 in helm-ring.el> #<marker at 9324 in helm-ring.el> #<marker at 9678 in helm-ring.el> #<marker at 4472 in helm-ring.el>)

…

-- Thierry

Ambrevar · 2017-10-23T10:04:21Z

Of course, all issues in this report imply that Evil is required.

thierryvolpiatto · 2017-10-23T10:38:40Z

Pierre Neidhardt <[email protected]> writes:

Of course, all issues in this report imply that Evil is required.

Yes, but if helm without evil have no duplicates, what about evil (just loaded, not activated) without helm loaded ?

…

-- Thierry

Ambrevar · 2017-10-24T08:32:02Z

Some updates: emacs-evil/evil#845
Bug was reproduced.

helm-mark-ring is a sync source, so I'm not quite sure why a timer would be involved. Any clue on this?

I've investigated the state of the mark ring a little bit:

Without helm/evil, well, it only changes on C-SPC and the like. Mostly explicit commands.
With Helm only, starting helm-mark-ring, browsing with follow mode on, and then quitting with C-g does not modify the mark-ring. If however I confirm instead of quitting (RET), then a new mark is added to the ring. (The previous entries are left untouched.) I'm not sure why that is, considering it's a ring, it should only change it's starting position, shouldn't it? I think there is something wrong here.
With Evil only, I get a similar behaviour when I set some marks and I navigate them with C-o/C-i.
With Helm+Evil, it corrupts the entire ring: markers are duplicated, replaced, swapped, etc. There might be a pattern but I haven't figured it out yet.

thierryvolpiatto · 2017-10-24T11:59:02Z

Pierre Neidhardt <[email protected]> writes:

Some updates: emacs-evil/evil#845 Bug was reproduced. helm-mark-ring is a sync source, so I'm not quite sure why a timer would be involved. Any clue on this?

Helm is always using a timer, in any kind of sources.

I've investigated the state of the mark ring a little bit: * Without helm/evil, well, it only changes on C-SPC and the like. Mostly explicit commands. * With Helm only, starting helm-mark-ring, browsing with follow mode on, and then quitting with C-g does not modify the mark-ring. If however I confirm instead of quitting (RET), then a new mark is added to the ring. (The previous entries are left untouched.) I'm not sure why that is, considering it's a ring, it should only change it's starting position, shouldn't it? I think there is something wrong here. * With Evil only, I get a similar behaviour when I set some marks and I navigate them with C-o/C-i. * With Helm+Evil, it corrupts the entire ring: markers are duplicated, replaced, swapped, etc. There might be a pattern but I haven't figured it out yet.

Can you reproduce by reverting b518c23 (see issue #1770). Thanks.

…

-- Thierry

Ambrevar · 2017-10-24T13:45:09Z

I checked out 178a216, I can still reproduce.
Can you reproduce yourself?

thierryvolpiatto · 2017-10-25T07:26:51Z

Pierre Neidhardt <[email protected]> writes:

I checked out 178a216, I can still reproduce. Can you reproduce yourself?

I am not using evil.

…

-- Thierry

Ambrevar · 2017-10-25T08:21:02Z

I understand, but can you install it? It's on MELPA, there is nothing to configure but (require 'evil).

and remove obsolete prefix args for mark in grep/occur actions fns. * helm-grep.el (helm-grep-action): Do it, signature have changed. (helm-grep-persistent-action): Fix call to helm-grep-action. * helm-regexp.el (helm-moccur-action): Same as helm-grep-action. (helm-moccur-goto-line): Remove prefix arg usage. (helm-moccur-goto-line-ow): Remove prefix arg usage. (helm-moccur-goto-line-of): Remove prefix arg usage.

thierryvolpiatto · 2017-10-27T04:21:16Z

Pierre Neidhardt <[email protected]> writes:

I understand, but can you install it? It's on MELPA, there is nothing to configure but ``require 'evil)`.

I could reproduce duplicates with only helm, should be fixed now, I have no more duplicates. Then, I have installed evil and required it, and yes I have now duplicates, but I have no errors from helm, I can navigate tags with follow-mode enabled and jump to a tag with RET as expected.

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.*

-- Thierry

There is nothing to fix here as the bug with dups in mark-ring comes from `push-mark` itself, advising `push-mark` fixes the problem. * helm-ring.el (helm-mark-ring-get-candidates): Push current mark to mark-ring when it is empty, otherwise use mark-ring. * helm-utils.el (helm-save-current-pos-to-mark-ring): Revert previous.

thierryvolpiatto · 2017-10-27T05:15:29Z

After investigating I think the problem doesn't come from evil nor helm but from push-mark which doesn't care of dups, advicing it fixes the problem, see:
https://github.com/thierryvolpiatto/emacs-tv-config/blob/master/.emacs.el#L227

Ambrevar · 2017-10-27T07:10:25Z

With latest commit I still have the issue.
Advising push-mark seems to fix it for good.

I need to do more thorough testing.

Thank you so much for this, this issue was by far one of the biggest blockers.

Ambrevar · 2017-10-27T08:50:47Z

I've had a look at your patch: looks good to me, but I wonder if upstream like cl-loops...
The cl-lib is required in simple.el where push-mark is defined, but only for cl-assert.

Regardless, would you submit your patch upstream?
I can do it too.

Ambrevar · 2017-10-27T08:57:17Z

That being said, why do mark duplicates cause helm-mark-ring to raise errors?

thierryvolpiatto · 2017-10-27T09:45:49Z

Pierre Neidhardt <[email protected]> writes:

I've had a look at your patch: looks good to me, but I wonder if upstream like cl-loops...

They don't.

The cl-lib is require in simple.el where push-mark is defined, but only for cl-assert.

Sadly yes.

Regardless, would you submit your patch upstream?

No, I don't submit bugs nor patchs to emacs anymore.

I can do it too.

If you want, rewrite the patch without cl-loop and submit as yourself, no problems. But the fix if accepted will be available only for last emacs.

…

-- Thierry

Ambrevar · 2017-10-27T10:01:38Z

Before submitting, I'd need to provide an explanation as for why this fixes the issue (my last comment). Can you explain?

thierryvolpiatto · 2017-10-27T10:54:27Z

Pierre Neidhardt <[email protected]> writes:

Before submitting, I'd need to provide an explanation as for why this fixes the issue (my last comment). Can you explain?

This advice fix two things, the first our issue and the second update the global mark ring to most recent mark, it also remove old entries for same buffer. Perhaps you should report only the first issue and make a second report for global ring. So for our issue, the original code push the old mark to mark-ring, which cause dups when this same marker is already in mark-ring whereas the new code ensure to remove the marker before adding it on top: ;;old: (setq mark-ring (cons (copy-marker (mark-marker)) mark-ring)) ;;new: (setq mark-ring (cons marker (delete marker mark-ring)))

…

-- Thierry

Ambrevar · 2017-10-27T11:02:13Z

I understand, but why did the duplicates raise the "Marker points into wrong buffer" error in helm-mark-ring?

thierryvolpiatto · 2017-10-27T11:27:34Z

Pierre Neidhardt <[email protected]> writes:

I understand, but why did the duplicates raise the "Marker points into wrong buffer" error in helm-mark-ring?

Don't know, I never had this error (even with evil loaded), I only noticed duplicates. Apparently with no duplicates you don't have such errors anymore?

…

-- Thierry

Ambrevar · 2017-10-27T11:31:08Z

Apparently. So you could not reproduce? I'm not sure that will give much incentive to get any patch accepted upstream, but I'll try anyways.

thierryvolpiatto · 2017-10-27T13:51:06Z

Pierre Neidhardt <[email protected]> writes:

Apparently. So you could not reproduce? I'm not sure that will give much incentive to get any patch accepted upstream, but I'll try anyways.

To reproduce you have from emacs -Q to use C-SPC C-SPC, then move point somewhere and eval: (set-marker (mark-marker) (point)) (push-mark) mark-ring You should have now one duplicate.

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.*

-- Thierry

thierryvolpiatto · 2017-10-27T13:55:37Z

I think they never take care of this because AFAIK there is no ways to use mark-ring in emacs, only the head of the ring, so duplicates are not a problem.

Ambrevar · 2017-10-27T13:58:06Z

That's a good point.

Ambrevar · 2017-10-27T20:20:52Z

Let's keep in mind though that there probably remains a bug in Helm's code: it should not fail when the mark-ring has duplicates.

thierryvolpiatto · 2017-10-28T04:28:38Z

Pierre Neidhardt <[email protected]> writes:

Let's keep in mind though that there still is a bug in Helm's code: it should not fail when the mark-ring has duplicates.

It doesn't, at least here.

…

-- Thierry

thierryvolpiatto · 2017-10-28T06:15:09Z

What about providing the advice in helm?
It was provided before but only fixing the global ring, and was provided as a mode, I removed it because it was unrelated to helm (helm mark-ring was working with and without advice), but now that you say it is causing a real bug when evil is enabled, we could provide the advice unconditionally.
Anyway if upstream accepts the change, it will be only in months, will not be in next emacs release and will not cover other versions of emacs (olders).
WDYT?

Ambrevar · 2017-10-28T06:59:20Z

Sorry to insist, but I'm not sure I understood from your comments:

Can you reproduce the issue with my recipe (both helm and evil required) without the advice?

@wasamasa could reproduce, so if you can't that would be puzzling.

As for the advice: Yes, it could be a good thing, but let's do it cleanly and make sure turning Helm off removes the advice. It would also be a good test ground as there are hundreds users of Helm-unstable, so we would know probably quite soon if this breaks anything. So I suggest you don't publish a stable release for a little while (say 1 month) so we don't force-break everything.

thierryvolpiatto · 2017-10-28T08:05:26Z

Pierre Neidhardt <[email protected]> writes:

Sorry to insist, but I'm not sure I understood from your comments: Can you reproduce the issue with my recipe (both helm and evil required) without the advice?

I got duplicates but didn't have error when jumping to mark from helm. Perhaps I didn't insist enough? I will try to reinstall evil an try again soon.

@wasamasa could reproduce, so if you can't that would be puzzling. As for the advice: Yes, it could be a good thing, but let's do it cleanly and make sure turning Helm off removes the advice.

You can accumulate duplicates in mark-ring outside of helm with any code modifying mark and using push-mark. So removing the advice when helm is turned of will not prevent us to have dups in mark-ring. Otherwise it is what I generally do, removing advices after each helm session.

…

-- Thierry

even if mark-ring is not empty. * helm-ring.el (helm-mark-ring-get-candidates): Do it and don't copy mark to avoid dups.

thierryvolpiatto · 2017-10-28T08:56:12Z

Pierre Neidhardt <[email protected]> writes:

Can you reproduce the issue with my recipe (both helm and evil required) without the advice?

Could not reproduce the error after reinstalling evil and retrying.

@wasamasa could reproduce, so if you can't that would be puzzling.

Indeed.

As for the advice: Yes, it could be a good thing, but let's do it cleanly and make sure turning Helm off removes the advice.

I could get duplicates from emacs -Q just typing C-SPC C-SPC at different places in a buffer. So the advice should be always enabled IMO.

…

-- Thierry

Same as originally. * helm-ring.el (helm-mark-ring-get-candidates): Do it.

* helm-lib.el (helm--advice-push-mark): New advice enabled.

Ambrevar · 2017-11-01T22:23:58Z

Bug reported upstream: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29110

Ambrevar · 2017-11-02T06:45:46Z

And here is a suggestion from Noam Postavsky:

One thing to note is that
push-mark resets markers before discarding (so that they no longer point
to any buffer). Maybe Helm or Evil keep another reference to a
discarded marker, and try to use it without checking? If you have a way
to reproduce the problem, you can check if bumping up mark-ring-max to a
very large number has any effect.
(defun push-mark (&optional location nomsg activate)
   ...
    (when (> (length mark-ring) mark-ring-max)
      (move-marker (car (nthcdr mark-ring-max mark-ring)) nil)
      (setcdr (nthcdr (1- mark-ring-max) mark-ring) nil)))
   ...
    (when (> (length global-mark-ring) global-mark-ring-max)
      (move-marker (car (nthcdr global-mark-ring-max global-mark-ring)) nil)
      (setcdr (nthcdr (1- global-mark-ring-max) global-mark-ring) nil)))

The discussion suggests that have duplicates in the mark ring is both fine and intended.
The above suggestion might be a good lead to follow.

* helm-lib.el (helm-advice-push-mark): Allow using or not an advice for push-mark.

* helm-ring.el (helm-mark-ring-default-action): Check if marker point to a buffer, if so jump to it, calling hook etc.. otherwise remove this marker from mark-ring.

thierryvolpiatto · 2017-11-02T09:01:12Z

Pierre Neidhardt <[email protected]> writes:

The discussion suggests that have duplicates in the mark ring is both fine and intended.

As expected, no surprise.

The above suggestion might be a good lead to follow.

Already know this and that's fine as long as there is not a serie of dereferenced marker in queue, which doesn't happen with a regular use of markers, but happens if one starts for some reasons to dereference markers (what Evil does apparently IIUC the comment of its developer). I pushed a fix that check if marker is referenced to a buffer before jumping to it and otherwise delete this unreferenced marker from mark-ring. I made also a defcustom to disable push-mark advice for the fans of duplicates.

…

-- Thierry

* helm-ring.el (helm-mark-ring-default-action): Do it.

Ambrevar · 2017-11-02T22:22:20Z

When the advice is removed, the issue is still here.

I might be missing something with the new option, but it seems that turning it off via Custom or with (setq helm-advice-push-mark nil) does not remove the advice.

I think the code should be

(defcustom helm-advice-push-mark t
  "Override `push-mark' with a version avoiding duplicates when non nil."
  :group 'helm
  :type 'boolean
  :set (lambda (var val)
         (set var val)
         (if (symbol-value var)
             (advice-add 'push-mark :override #'helm--advice-push-mark)
           (advice-remove 'push-mark #'helm--advice-push-mark))))

* helm-lib.el (helm-advice-push-mark): Use value of symbol.

thierryvolpiatto · 2017-11-03T05:45:18Z

Pierre Neidhardt <[email protected]> writes:

When the advice is removed, the issue is still here.

Of course it is, and we can't do anything to fix it, the ball is now in Evil side.

I might be missing something with the new option, but it seems that turning it off via Custom or with (setq helm-advice-push-mark nil) does not remove the advice.

Thanks fixed.

…

-- Thierry

thierryvolpiatto · 2017-11-04T16:46:07Z

I think this can be closed as there is no much more to do on our side.

Ambrevar mentioned this issue Oct 23, 2017

helm-buffers input hangs in follow-mode with EXWM #1889

Closed

thierryvolpiatto pushed a commit that referenced this issue Oct 28, 2017

Ensure to always collect current mark (#1891)

2cfaea5

even if mark-ring is not empty. * helm-ring.el (helm-mark-ring-get-candidates): Do it and don't copy mark to avoid dups.

thierryvolpiatto pushed a commit that referenced this issue Oct 28, 2017

Don't push but cons current mark (#1891)

ece8972

Same as originally. * helm-ring.el (helm-mark-ring-get-candidates): Do it.

thierryvolpiatto pushed a commit that referenced this issue Oct 28, 2017

[Emacs bug] Advice push-mark (#1891)

ffd2abf

* helm-lib.el (helm--advice-push-mark): New advice enabled.

thierryvolpiatto added the emacsbug label Oct 28, 2017

thierryvolpiatto pushed a commit that referenced this issue Nov 2, 2017

Allow customisation of push-mark advice (#1891)

ff2c746

* helm-lib.el (helm-advice-push-mark): Allow using or not an advice for push-mark.

thierryvolpiatto pushed a commit that referenced this issue Nov 2, 2017

Delete dereferenced marker if some (#1891)

8889470

* helm-ring.el (helm-mark-ring-default-action): Check if marker point to a buffer, if so jump to it, calling hook etc.. otherwise remove this marker from mark-ring.

thierryvolpiatto pushed a commit that referenced this issue Nov 2, 2017

Add an error message for unreferenced markers (#1891)

b219217

* helm-ring.el (helm-mark-ring-default-action): Do it.

thierryvolpiatto pushed a commit that referenced this issue Nov 3, 2017

Fix defcustom function (#1891)

9395564

* helm-lib.el (helm-advice-push-mark): Use value of symbol.

thierryvolpiatto closed this as completed Nov 4, 2017

"Marker points into wrong buffer" with helm-mark-ring and Evil #1891

"Marker points into wrong buffer" with helm-mark-ring and Evil #1891

Comments

Ambrevar commented Oct 9, 2017 • edited Loading

Expected behavior

Actual behavior (from emacs-helm.sh if possible, see note at the bottom)

Steps to reproduce (recipe)

Backtraces if any (M-x toggle-debug-on-error)

Describe versions of Helm, Emacs, operating system, etc.

Are you using emacs-helm.sh to reproduce this bug (yes/no):

thierryvolpiatto commented Oct 10, 2017 via email

Ambrevar commented Oct 10, 2017 • edited Loading

thierryvolpiatto commented Oct 11, 2017 via email

Ambrevar commented Oct 11, 2017

Ambrevar commented Oct 23, 2017 • edited Loading

thierryvolpiatto commented Oct 23, 2017 via email

Ambrevar commented Oct 23, 2017

thierryvolpiatto commented Oct 23, 2017 via email

Ambrevar commented Oct 24, 2017

thierryvolpiatto commented Oct 24, 2017 via email

Ambrevar commented Oct 24, 2017

thierryvolpiatto commented Oct 25, 2017 via email

Ambrevar commented Oct 25, 2017 • edited Loading

thierryvolpiatto commented Oct 27, 2017 via email

thierryvolpiatto commented Oct 27, 2017

Ambrevar commented Oct 27, 2017

Ambrevar commented Oct 27, 2017 • edited Loading

Ambrevar commented Oct 27, 2017

thierryvolpiatto commented Oct 27, 2017 via email

Ambrevar commented Oct 27, 2017

thierryvolpiatto commented Oct 27, 2017 via email

Ambrevar commented Oct 27, 2017

thierryvolpiatto commented Oct 27, 2017 via email

Ambrevar commented Oct 27, 2017

thierryvolpiatto commented Oct 27, 2017 via email

thierryvolpiatto commented Oct 27, 2017

Ambrevar commented Oct 27, 2017

Ambrevar commented Oct 27, 2017 • edited Loading

thierryvolpiatto commented Oct 28, 2017 via email

thierryvolpiatto commented Oct 28, 2017

Ambrevar commented Oct 28, 2017

thierryvolpiatto commented Oct 28, 2017 via email

thierryvolpiatto commented Oct 28, 2017 via email

Ambrevar commented Nov 1, 2017

Ambrevar commented Nov 2, 2017

thierryvolpiatto commented Nov 2, 2017 via email

Ambrevar commented Nov 2, 2017 • edited Loading

thierryvolpiatto commented Nov 3, 2017 via email

thierryvolpiatto commented Nov 4, 2017

Ambrevar commented Oct 9, 2017 •

edited

Loading

Actual behavior (from `emacs-helm.sh` if possible, see note at the bottom)

Backtraces if any (`M-x toggle-debug-on-error`)

Are you using `emacs-helm.sh` to reproduce this bug (yes/no):

Ambrevar commented Oct 10, 2017 •

edited

Loading

Ambrevar commented Oct 23, 2017 •

edited

Loading

Ambrevar commented Oct 25, 2017 •

edited

Loading

Ambrevar commented Oct 27, 2017 •

edited

Loading

Ambrevar commented Oct 27, 2017 •

edited

Loading

Ambrevar commented Nov 2, 2017 •

edited

Loading