Maintain position in room list buffer

[phil-s]

Something I find frustrating is when I switch from the room list buffer to a room buffer, and then quit back to the room list, and find that point has been moved back to the start of the room list. This resetting seems to happen a lot.

(Edit: I see that ement-room-list-auto-update is relevant here; with that disabled, it looks like the issue doesn't occur. It would be nice to get the best of both worlds, though.)

I think the buffer is actually being erased and regenerated, so it's presumably not quite as trivial as a save-excursion; but to me it's an undesirable UX to have the apparent position forcibly changed this way. Even if the buffer is being rebuilt, I think it would be beneficial to try to match the previous position based on context. As it's a room list, "the line of the same room" would seem pretty easy and reliable in most cases.

[alphapapa]

There is already code to do this, see:

ement.el/ement-room-list.el

Lines 684 to 693 in 909abd4

	(if-let* ((section-ident)
	(section (magit-get-section section-ident)))
	(goto-char (oref section start))
	(goto-char pos))))
	(when display-buffer-action
	(when-let ((window (display-buffer buffer-name display-buffer-action)))
	(select-window window)))
	(when-let ((window (get-buffer-window buffer-name)))
	(set-window-start window window-start)
	(set-window-point window window-point))

Sometimes it doesn't work. I haven't been able to find what is moving point back to the beginning sometimes, despite a lot of Edebug stepping and so forth.

[phil-s]

WRT (elisp)Quitting Windows...

If I start in the room list buffer (in my test (point) is 690), and select a room with RET (replacing the room list buffer in its window), and then evaluate (window-parameter nil 'quit-restore) in the room buffer I see:

(other (#<buffer *Ement Room List*> 1 #<marker at 1 in *Ement Room List*> 23) #<window 99 on *Ement Room: #emacs*> #<buffer *Ement Room: #emacs*>)

Which is: (METHOD (PREV-BUFFER PREV-WINDOW-START PREV-WINDOW-POINT HEIGHT) OWINDOW THIS-BUFFER)

So #<marker at 1 in *Ement Room List*> is PREV-WINDOW-START and when I quit the room buffer it takes me back to position 1.

When a region containing a marker is deleted the marker gets moved to the position before the deleted region. Hence if the whole buffer contents are erased, the marker will be moved to position 1. This ties in with my observation about ement-room-list-auto-update (and I can confirm that a call to that function happens when I select a room; so by the time I'd checked the window property, the marker position had already been changed).

The call to set-window-point won't help with that -- you would need to set the position of the specific marker used in the quit-restore property.

I believe display-buffer-record-window is responsible, in which I see:

      ;; WINDOW shows another buffer.
      (with-current-buffer (window-buffer window)
	(set-window-parameter
	 window 'quit-restore
	 (list 'other
	       ;; A quadruple of WINDOW's buffer, start, point and height.
	       (list (current-buffer) (window-start window)
		     ;; Preserve window-point-insertion-type (Bug#12855).
		     (copy-marker
		      (window-point window) window-point-insertion-type)
		     (if (window-combined-p window)
                         (window-total-height window)
                       (window-total-width window)))
	       (selected-window) buffer)))))

So the marker is specific to the window property, so I guess that will need to be captured so that it can be manipulated at the same time as the set-window-point call. I suppose it might be necessary to maintain a list of them (one for each of the relevant windows).

Or probably simpler and maybe better: clobber each of these markers with a simple buffer position integer (its own marker-position). Hopefully the quit-restore handler doesn't assume it's definitely a marker.

Or maintain your own single marker for the most recent position in the room list, and share that around -- injecting it as a replacement marker into each of the window property lists (this won't work out if the quit-restore handler explicitly cleans up the marker, but if it leaves it to the GC then it should be fine).

Or before erasing the buffer, walk all of the windows looking for quit-restore properties for the room list, build a list mapping windows to marker positions and update them all again after rebuilding the buffer. Or even map windows to the room for the line the marker was on, and then find the (possibly-new) positions for each of those rooms in the rebuilt buffer when updating the window properties.

[phil-s]

Another option may be for ement-room-list to not erase-buffer and to use replace-buffer-contents to try to maintain the markers.

[phil-s]

I decided to implement a proof of concept as an interim measure. This "works" insofar as it maintains the original position but, as the room list sequence may have changed in the interim, that may or may not be where you want to go back to. Sometimes it might be, though? I guess I'll run with this for now and see whether the behaviour is what I want most of the time.

modified   ement-room-list.el
@@ -674,6 +674,19 @@ DISPLAY-BUFFER-ACTION is nil, the buffer is not displayed."
             (when ement-room-list-visibility-cache
               (setf magit-section-visibility-cache ement-room-list-visibility-cache))
             (add-hook 'kill-buffer-hook #'ement-room-list--cache-visibility nil 'local)
+            ;; Update relevant `quit-restore' markers before they all get reset to 1.
+            (let ((roomlist (current-buffer)))
+              (cl-flet ((process
+                         (win)
+                         (when-let ((qr (window-parameter win 'quit-restore)))
+                           (and (eq (car qr) 'other)
+                                (eq (caadr qr) roomlist)
+                                (markerp (nth 2 (nth 1 qr)))
+                                ;; (message "Setting marker in %s to %s" win
+                                ;;          (marker-position (nth 2 (nth 1 qr))))
+                                (setf (nth 2 (nth 1 qr))
+                                      (marker-position (nth 2 (nth 1 qr))))))))
+                (walk-windows #'process t t)))
             ;; Before this point, no changes have been made to the buffer's contents.
             (delete-all-overlays)
             (erase-buffer)

[phil-s]

I see that this case can still occur:

FIXME: Despite all this code to save and restore point and window point and window start, when I send a message from the minibuffer, or when I abort sending a message from the minibuffer, point is moved to the beginning of the buffer.

Or at least I can reproduce that when sending a message (i.e. not when aborting with C-g) -- but only in conjunction with another weird bug, which is that ement sometimes creates a new frame for the room buffer when I send a message.

I think its happening when every other window in the original frame is 'dedicated' to its buffer; but given that there's a perfectly good window and buffer in that frame for the room I'm sending the message to, it doesn't make any logical sense for a new frame to be created.

Anyhow, if a new frame is created at that stage, then point moves back to the beginning in the room list.

(I can confirm that simply using C-x 5 2 has no such effect.)

[phil-s]

Ok, this is the reason for the new frame when all the other windows are dedicated:

1 -> (display-buffer #<buffer *Org HTML Export*> t)

I think we want to get the display-buffer-no-window action handling that one.

[phil-s]

Oh, crazy... simply calling (display-buffer SOMEBUF) can trigger the movement back to the start of the room list.

Not consistently, though, so maybe not directly on that account? How fun...

[phil-s]

I believe I have this resolved. I'll raise a PR later today.

The remaining bug was the same issue of the window point being set using an internal marker, with that internal marker being reset to 1 every time the buffer is erased.

In this case it's on account of the buffer-local switch-to-buffer-preserve-window-point variable, as switch-to-buffer obtains an entry value from (window-prev-buffers) and that contains a marker which was established before the room list buffer was regenerated.

I believe setting that to nil buffer-locally resolves the issues I was still seeing.

[phil-s]

In case it's useful, here's what I came up with to locate that:

;;(advice-remove 'set-window-point 'set-window-point@my-debug)
(define-advice set-window-point (:before (window pos) my-debug)
  (and (markerp pos)
       (eql (marker-position pos) 1)
       (message "set-window-point(1): %S"
                (memq 'set-window-point@my-debug
                      (mapcar (lambda (fun) (or (and (symbolp fun) fun) 'SKIP))
                              (mapcar #'backtrace-frame-fun (backtrace-get-frames)))))))

(debug-on-entry was a bit unwieldy in this case!)

I used this in a couple of places, and the SKIP part is because the first time was in ement-room-list, and printing the (big) non-symbol function objects to the messages buffer every time this code triggered almost killed Emacs :)

[alphapapa]

As I said on #193, thanks very much, Phil, for digging into this. Not a fun problem, and very non-obvious behavior.