bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[Jay Kamat]

Hi,

One of the built-in erc modules is keep-place (in erc-goodies), which
keeps the place of point while new messages come into erc. The module
works as expected in Emacs 25.

However, in Emacs 26, enabling this module does nothing in an Emacs -Q
(ie: the point is not saved at all, point is always moved to the bottom
of the buffer).

After much searching, I found this is related to the new default value
(t) of `switch-to-buffer-preserve-window-point'. When setting this
variable back to nil, this issue goes away, and the keep-place module
works as normal on Emacs 26.

Since `switch-to-buffer-preserve-window-point' is defaulting to t, is it
possible to update keep-place to work regardless of this setting? If
that's not a possibility (since this seems to be a global variable),
documentation needs to be updated somehow to make it very clear that
this variable must be set to 'nil' for keep-place to work properly.


I tested this with the minimal init file:

(require 'erc)
(setq erc-modules
	  '(autojoin button completion fill irccontrols list match
				 menu move-to-prompt netsplit networks noncommands
				 readonly ring stamp track truncate keep-place))
(erc-update-modules)


Please let me know if you cannot reproduce.

-Jay

In GNU Emacs 26.0.90 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-10-22 built on laythe
Repository revision: 237e96bc5259e59ac5623a93a47f64abffab4e0b
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description:	Debian GNU/Linux 9.2 (stretch)

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[martin rudalics]

 > Since `switch-to-buffer-preserve-window-point' is defaulting to t, is it
 > possible to update keep-place to work regardless of this setting? If
 > that's not a possibility (since this seems to be a global variable),
 > documentation needs to be updated somehow to make it very clear that
 > this variable must be set to 'nil' for keep-place to work properly.

Can you try replacing all ‘switch-to-buffer’ calls in the erc directory
by ‘pop-to-buffer-same-window’ calls and test whether the problem goes
away?  I'm afraid I have no better proposal.

martin

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[Jay Kamat]

Hi Martin,

> Can you try replacing all ‘switch-to-buffer’ calls in the erc directory
> by ‘pop-to-buffer-same-window’ calls and test whether the problem goes
> away?  I'm afraid I have no better proposal.

I tried this, and it seems to restore keep-place functionality when
switching to buffers with M-x erc-track-switch-buffer, but not with C-x
b. I suspect the call to switch-to-buffer in C-x b isn't handled by erc,
so changing the function won't help. Obviously, I don't think that's an
acceptable solution.

I can think of two solutions right now:

1. Add a new configuration option which allows keep-place mode to
disable this setting dynamically, and on enabling keep-place, add erc
buffers to the exempted list. I took a look at `switch-to-buffer' and I
don't see any way this can be done without a new variable. I personally
would like to use the new default for non-erc buffers, and I would
suspect that others would like to turn it on selectively as well.

2. Make it impossible to miss that this variable must be turned off when
keep-place is enabled. If this isn't done, people's config will silently
break (which is pretty bad). I would think that keep-place should throw
an error when this variable is t, to be safe (since it doesn't work at
all when it is).

What do you think? I would be happy if keep-place would work out of the
box for new users of erc, but I'm not sure if that's possible...

-Jay

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[martin rudalics]

 > I tried this, and it seems to restore keep-place functionality when
 > switching to buffers with M-x erc-track-switch-buffer, but not with C-x
 > b. I suspect the call to switch-to-buffer in C-x b isn't handled by erc,
 > so changing the function won't help. Obviously, I don't think that's an
 > acceptable solution.

"keep place" means "preserve point" so it's somewhat surprising that
this is a problem in the first place.  Can you explain why and how the
keep place mechanism and the ‘switch-to-buffer-preserve-window-point’
mechanisms differ?  Is ‘window-point-insertion-type’ involved somehow?

 > I can think of two solutions right now:
 >
 > 1. Add a new configuration option which allows keep-place mode to
 > disable this setting dynamically, and on enabling keep-place, add erc
 > buffers to the exempted list. I took a look at `switch-to-buffer' and I
 > don't see any way this can be done without a new variable. I personally
 > would like to use the new default for non-erc buffers, and I would
 > suspect that others would like to turn it on selectively as well.

Alternatively, we could allow ‘switch-to-buffer-preserve-window-point’
have a buffer local value which would override the customizable value
and add a new value, say 'always', which would allow the user to
override a local 'nil' while the default 't' would have the local 'nil'
prevail.  Maybe we'd then need a 'never' too ...

 > 2. Make it impossible to miss that this variable must be turned off when
 > keep-place is enabled. If this isn't done, people's config will silently
 > break (which is pretty bad). I would think that keep-place should throw
 > an error when this variable is t, to be safe (since it doesn't work at
 > all when it is).
 >
 > What do you think? I would be happy if keep-place would work out of the
 > box for new users of erc, but I'm not sure if that's possible...

Since ‘keep-place’ is some sort of a minor mode, enabling it should warn
the user about the ‘switch-to-buffer-preserve-window-point’
incompatibility.  But I'm not familiar with ‘define-erc-module’ so we'd
probably need someone knowledgeable to do that.

martin

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[Eli Zaretskii]

> Date: Fri, 03 Nov 2017 08:58:48 +0100
> From: martin rudalics <rudalics@gmx.at>
> Cc: 29111@debbugs.gnu.org
> 
> Alternatively, we could allow ‘switch-to-buffer-preserve-window-point’
> have a buffer local value which would override the customizable value
> and add a new value, say 'always', which would allow the user to
> override a local 'nil' while the default 't' would have the local 'nil'
> prevail.  Maybe we'd then need a 'never' too ...

That's a possibility, but it would be too radical to go into 26.1, so
I think we should explore the less drastic alternatives first.

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[Jay Kamat]

Hi,

> "keep place" means "preserve point" so it's somewhat surprising that
> this is a problem in the first place.  Can you explain why and how the
> keep place mechanism and the ‘switch-to-buffer-preserve-window-point’
> mechanisms differ?  Is ‘window-point-insertion-type’ involved somehow?

I'm not very familiar with emacs internals, so all of this could be
wrong, but here's my best shot of explaining why this is a problem.

By default, point stays at the bottom of erc buffers as new messages
come in. If point is not at the bottom, point stays where it is (so even
when new messages come in, point wont move).

Keep place adds a hook so that, when a new message comes in on a
non-visible buffer *and* point is at the bottom, point is moved up by
one line. Doing this means that point stays on the last 'read'
message. Here's the relevant code from keep-place:

> (defun erc-keep-place (ignored)
> "Move point away from the last line in a non-selected ERC buffer."
> (when (and (not (eq (window-buffer (selected-window))
>                     (current-buffer)))
>            (>= (point) erc-insert-marker))
>   (deactivate-mark)
>   (goto-char (erc-beg-of-input-line))
>   (forward-line -1)))

Previously, this worked fine, since moving point on non-visible buffers
meant the movement persisted till the buffer was next 'shown'. However,
the new default of `switch-to-buffer-preserve-window-point' ensures that
point stays wherever it was 'last seen'. I'm not sure how it does it,
but I think it's saving point when a buffer is hidden and restoring it
when it's seen again

> (when (and entry
> 		   (or (eq switch-to-buffer-preserve-window-point t)
> 		       displayed))
> 	  ;; Try to restore start and point of buffer in the selected
> 	  ;; window (Bug#4041).
> 	  (set-window-start (selected-window) (nth 1 entry) t)
> 	  (set-window-point nil (nth 2 entry)))


>> Alternatively, we could allow ‘switch-to-buffer-preserve-window-point’
>> have a buffer local value which would override the customizable value
>> and add a new value, say 'always', which would allow the user to
>> override a local 'nil' while the default 't' would have the local 'nil'
>> prevail.  Maybe we'd then need a 'never' too ...
>
>That's a possibility, but it would be too radical to go into 26.1, so
>I think we should explore the less drastic alternatives first.

I agree with Eli that this is too big of a change at this point, but I
think this is the best long term solution. Perhaps we could
`make-variable-buffer-local' on `switch-to-buffer-preserve-window-point'
when enabling keep-place (later, of course).

> Since ‘keep-place’ is some sort of a minor mode, enabling it should warn
> the user about the ‘switch-to-buffer-preserve-window-point’
> incompatibility.  But I'm not familiar with ‘define-erc-module’ so we'd
> probably need someone knowledgeable to do that.

I think this is probably the best idea before the emacs 26 release. The
definition of 'keep-place' is currently

> (define-erc-module keep-place nil
>   "Leave point above un-viewed text in other channels."
>   ((add-hook 'erc-insert-pre-hook  'erc-keep-place))
>   ((remove-hook 'erc-insert-pre-hook  'erc-keep-place)))

I think we should be able to simply do something like

> (define-erc-module keep-place nil
>   "Leave point above un-viewed text in other channels."
>   ((add-hook 'erc-insert-pre-hook  'erc-keep-place)
>    ;; poor name, but just an example
>    (erc-check-if-switch-to-buffer-preserve-and-warn))
>   ((remove-hook 'erc-insert-pre-hook  'erc-keep-place)))

I would be happy to submit a patch for this, if the Erc maintainer is
busy. Does this seem like a good solution, or does anyone see a better
way around this?

Thanks for the responses so far!

-Jay

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[martin rudalics]

 > Keep place adds a hook so that, when a new message comes in on a
 > non-visible buffer *and* point is at the bottom, point is moved up by
 > one line. Doing this means that point stays on the last 'read'
 > message. Here's the relevant code from keep-place:
 >
 >> (defun erc-keep-place (ignored)
 >> "Move point away from the last line in a non-selected ERC buffer."
 >> (when (and (not (eq (window-buffer (selected-window))
 >>                      (current-buffer)))
 >>             (>= (point) erc-insert-marker))
 >>    (deactivate-mark)
 >>    (goto-char (erc-beg-of-input-line))
 >>    (forward-line -1)))

You could try to pretend that the position at which the buffer was
previously shown in a window is the position calculated here.  Add after
the

(forward-line -1)

something like

;; For all non-minibuffer windows on all frames check whether the
;; current buffer was shown in that window before.  If so, update the
;; window point positions stored for the buffer to the position we just
;; calculated.
(dolist (frame (frame-list))
   (walk-window-tree
    (lambda (window)
      (let ((prev (assq (current-buffer) (window-prev-buffers window)))
	   (next (assq (current-buffer) (window-next-buffers window))))
        (when prev
	 (setf (nth 2 prev) (point-marker)))
        (when next
	 (setf (nth 2 next) (point-marker)))))
    frame nil 'nominibuf))

Completely _untested_ so you may have to tweak it appropriately!

 > Previously, this worked fine, since moving point on non-visible buffers
 > meant the movement persisted till the buffer was next 'shown'. However,
 > the new default of `switch-to-buffer-preserve-window-point' ensures that
 > point stays wherever it was 'last seen'. I'm not sure how it does it,
 > but I think it's saving point when a buffer is hidden and restoring it
 > when it's seen again
 >
 >> (when (and entry
 >> 		   (or (eq switch-to-buffer-preserve-window-point t)
 >> 		       displayed))
 >> 	  ;; Try to restore start and point of buffer in the selected
 >> 	  ;; window (Bug#4041).
 >> 	  (set-window-start (selected-window) (nth 1 entry) t)
 >> 	  (set-window-point nil (nth 2 entry)))

Right.  So with the hack above you would set (nth 2 entry) to the value
keep-place calculated.

 >> That's a possibility, but it would be too radical to go into 26.1, so
 >> I think we should explore the less drastic alternatives first.
 >
 > I agree with Eli that this is too big of a change at this point, but I
 > think this is the best long term solution. Perhaps we could
 > `make-variable-buffer-local' on `switch-to-buffer-preserve-window-point'
 > when enabling keep-place (later, of course).

I can't yet assess all implications of such a solution so I'd certainly
defer it until all other measures have been exhausted.

 >> Since ‘keep-place’ is some sort of a minor mode, enabling it should warn
 >> the user about the ‘switch-to-buffer-preserve-window-point’
 >> incompatibility.  But I'm not familiar with ‘define-erc-module’ so we'd
 >> probably need someone knowledgeable to do that.
 >
 > I think this is probably the best idea before the emacs 26 release. The
 > definition of 'keep-place' is currently
 >
 >> (define-erc-module keep-place nil
 >>    "Leave point above un-viewed text in other channels."
 >>    ((add-hook 'erc-insert-pre-hook  'erc-keep-place))
 >>    ((remove-hook 'erc-insert-pre-hook  'erc-keep-place)))
 >
 > I think we should be able to simply do something like
 >
 >> (define-erc-module keep-place nil
 >>    "Leave point above un-viewed text in other channels."
 >>    ((add-hook 'erc-insert-pre-hook  'erc-keep-place)
 >>     ;; poor name, but just an example
 >>     (erc-check-if-switch-to-buffer-preserve-and-warn))
 >>    ((remove-hook 'erc-insert-pre-hook  'erc-keep-place)))
 >
 > I would be happy to submit a patch for this, if the Erc maintainer is
 > busy. Does this seem like a good solution, or does anyone see a better
 > way around this?

If all else fails let's do that.  First please try the proposal above.

martin

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[Jay Kamat]

[-- Attachment #1: Type: text/plain, Size: 524 bytes --]

Hi,

Sorry for the delay, I've been a bit busy for the last couple of
weeks. The code that Martin provided worked perfectly, and solves the
issue! I removed a few unneeded lines (`window-next-buffers' need not be
modified, as it is not used when setting point) and wrapped it so it
only runs when `switch-to-buffer-preserve-window-point' is set, and
created a patch, which is attached. 

Let me know if you think another approach is better or if there are
other issues with the patch that I can help resolve.

Thanks,
-Jay


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-erc-keep-place-module-with-new-defaults-bug-2911.patch --]
[-- Type: text/x-diff, Size: 1386 bytes --]

From 1ccb3110be45a9492a53bc149ff7f824dc132479 Mon Sep 17 00:00:00 2001
From: Jay Kamat <jaygkamat@gmail.com>
Date: Sat, 18 Nov 2017 16:41:34 -0500
Subject: [PATCH] Fix erc keep-place module with new defaults (bug#29111)

Allows erc keep-place to continue working with
switch-to-buffer-preserve-window-point set to t, the new default.
---
 lisp/erc/erc-goodies.el | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
index a655d48a6a..8906da1e47 100644
--- a/lisp/erc/erc-goodies.el
+++ b/lisp/erc/erc-goodies.el
@@ -147,7 +147,19 @@ erc-keep-place
              (>= (point) erc-insert-marker))
     (deactivate-mark)
     (goto-char (erc-beg-of-input-line))
-    (forward-line -1)))
+    (forward-line -1)
+    ;; if `switch-to-buffer-preserve-window-point' is set,
+    ;; we cannot rely on point being saved, and must commit
+    ;; it to window-prev-buffers.
+    (when switch-to-buffer-preserve-window-point
+      (dolist (frame (frame-list))
+        (walk-window-tree
+         (lambda (window)
+           (let ((prev (assq (current-buffer)
+                             (window-prev-buffers window))))
+             (when prev
+	       (setf (nth 2 prev) (point-marker)))))
+         frame nil 'nominibuf)))))
 
 ;;; Distinguish non-commands
 (defvar erc-noncommands-list '(erc-cmd-ME
-- 
2.11.0


[-- Attachment #3: Type: text/plain, Size: 4233 bytes --]


martin rudalics <rudalics@gmx.at> writes:

>> Keep place adds a hook so that, when a new message comes in on a
>> non-visible buffer *and* point is at the bottom, point is moved up by
>> one line. Doing this means that point stays on the last 'read'
>> message. Here's the relevant code from keep-place:
>>
>>> (defun erc-keep-place (ignored)
>>> "Move point away from the last line in a non-selected ERC buffer."
>>> (when (and (not (eq (window-buffer (selected-window))
>>>                      (current-buffer)))
>>>             (>= (point) erc-insert-marker))
>>>    (deactivate-mark)
>>>    (goto-char (erc-beg-of-input-line))
>>>    (forward-line -1)))
>
> You could try to pretend that the position at which the buffer was
> previously shown in a window is the position calculated here.  Add after
> the
>
> (forward-line -1)
>
> something like
>
> ;; For all non-minibuffer windows on all frames check whether the
> ;; current buffer was shown in that window before.  If so, update the
> ;; window point positions stored for the buffer to the position we just
> ;; calculated.
> (dolist (frame (frame-list))
>   (walk-window-tree
>    (lambda (window)
>      (let ((prev (assq (current-buffer) (window-prev-buffers window)))
> 	   (next (assq (current-buffer) (window-next-buffers window))))
>        (when prev
> 	 (setf (nth 2 prev) (point-marker)))
>        (when next
> 	 (setf (nth 2 next) (point-marker)))))
>    frame nil 'nominibuf))
>
> Completely _untested_ so you may have to tweak it appropriately!
>
>> Previously, this worked fine, since moving point on non-visible buffers
>> meant the movement persisted till the buffer was next 'shown'. However,
>> the new default of `switch-to-buffer-preserve-window-point' ensures that
>> point stays wherever it was 'last seen'. I'm not sure how it does it,
>> but I think it's saving point when a buffer is hidden and restoring it
>> when it's seen again
>>
>>> (when (and entry
>>> 		   (or (eq switch-to-buffer-preserve-window-point t)
>>> 		       displayed))
>>> 	  ;; Try to restore start and point of buffer in the selected
>>> 	  ;; window (Bug#4041).
>>> 	  (set-window-start (selected-window) (nth 1 entry) t)
>>> 	  (set-window-point nil (nth 2 entry)))
>
> Right.  So with the hack above you would set (nth 2 entry) to the value
> keep-place calculated.
>
>>> That's a possibility, but it would be too radical to go into 26.1, so
>>> I think we should explore the less drastic alternatives first.
>>
>> I agree with Eli that this is too big of a change at this point, but I
>> think this is the best long term solution. Perhaps we could
>> `make-variable-buffer-local' on `switch-to-buffer-preserve-window-point'
>> when enabling keep-place (later, of course).
>
> I can't yet assess all implications of such a solution so I'd certainly
> defer it until all other measures have been exhausted.
>
>>> Since ‘keep-place’ is some sort of a minor mode, enabling it should warn
>>> the user about the ‘switch-to-buffer-preserve-window-point’
>>> incompatibility.  But I'm not familiar with ‘define-erc-module’ so we'd
>>> probably need someone knowledgeable to do that.
>>
>> I think this is probably the best idea before the emacs 26 release. The
>> definition of 'keep-place' is currently
>>
>>> (define-erc-module keep-place nil
>>>    "Leave point above un-viewed text in other channels."
>>>    ((add-hook 'erc-insert-pre-hook  'erc-keep-place))
>>>    ((remove-hook 'erc-insert-pre-hook  'erc-keep-place)))
>>
>> I think we should be able to simply do something like
>>
>>> (define-erc-module keep-place nil
>>>    "Leave point above un-viewed text in other channels."
>>>    ((add-hook 'erc-insert-pre-hook  'erc-keep-place)
>>>     ;; poor name, but just an example
>>>     (erc-check-if-switch-to-buffer-preserve-and-warn))
>>>    ((remove-hook 'erc-insert-pre-hook  'erc-keep-place)))
>>
>> I would be happy to submit a patch for this, if the Erc maintainer is
>> busy. Does this seem like a good solution, or does anyone see a better
>> way around this?
>
> If all else fails let's do that.  First please try the proposal above.
>
> martin

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[martin rudalics]

 > Let me know if you think another approach is better or if there are
 > other issues with the patch that I can help resolve.

Thanks.  Eli, is it OK to install this on the release branch?

martin

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[Eli Zaretskii]

> Date: Mon, 20 Nov 2017 09:26:41 +0100
> From: martin rudalics <rudalics@gmx.at>
> Cc: 29111@debbugs.gnu.org
> 
>  > Let me know if you think another approach is better or if there are
>  > other issues with the patch that I can help resolve.
> 
> Thanks.  Eli, is it OK to install this on the release branch?

Yes, please.

Thanks.

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[martin rudalics]

 >> Thanks.  Eli, is it OK to install this on the release branch?
 >
 > Yes, please.

Committed as tiny change since I nowhere found any prior contribution by
the author.  Jay, if you already signed papers, please tell me so I can
eventually remove the remark.  Otherwise, please consider signing them
so we can process future patches by you.

And if there are no more issues please close the bug.

Thank you, martin

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[Eli Zaretskii]

> Date: Tue, 21 Nov 2017 10:23:25 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: jaygkamat@gmail.com, 29111@debbugs.gnu.org
> 
>  >> Thanks.  Eli, is it OK to install this on the release branch?
>  >
>  > Yes, please.
> 
> Committed as tiny change since I nowhere found any prior contribution by
> the author.  Jay, if you already signed papers, please tell me so I can
> eventually remove the remark.

Jay has an assignment on file (you can always ask me before pushing,
if you aren't sure about that).

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[Jay Kamat]

Hi,

As Eli said, I should have assignment on file. Let me know if there are
any other issues with the patch and I'll follow up. I'll (attempt) to
close this bug now.

Thanks again to Martin for all your help, and working to get this bug fixed!

-Jay

bug#29111: 26.0.90; Erc keep-place module broken with new default of switch-to-buffer-preserve-window-point

[martin rudalics]

 > Jay has an assignment on file (you can always ask me before pushing,
 > if you aren't sure about that).

My bad.  I should have asked Jay in the first place.

martin