Subject: Re: bug#35860: Delayed window positioning after buffer display

From: Juri Linkov <[email-masked]>
Date: Mon, 17 Jun 2019 23:33:44 +0300

>> In my previous patch there is no need to check for the buffer,
>> because a new general variable is declared as buffer-local.
>
> But the BUFFER argument of 'window--display-buffer' is not necessarily
> the current buffer.  Or do I miss something?

Thanks for pointing out, fixed in a new patch.

>>> What happens with these markers when 'display-buffer-no-window' gets
>>> called?  Or some user provided routine provokes an unhandled error?
>>> You don't even check the marker buffer of these variables so some old,
>>> completely unrelated marker could get reused here.
>>
>> There is no global effect because this feature is localized
>> to a single buffer by its buffer-local scope.
>
> We would always have to assure that the marker's buffer and the
> current buffer are the same.  And we would have to make sure the
> marker gets reset when an unhandled error occurs.

Assured in a new patch.

>> We could support an action alist too in addition to buffer-local variables,
>> but we should leave an ability to set buffer-local variables, because
>> neither view-lossage nor vc-git-print-log (two primary cases to test
>> this feature) have no access to the 'action' arg of display-buffer.
>
> Maybe we could provide a new macro which encapsulates the use of these
> variables in a more disciplied fashion (providing the necessary action
> arguments for `display-buffer').

Frankly speaking, macros like with-help-window, with-temp-buffer-window
are terribly ugly and look like a kludge.  For example, after applying
the following patch, all source files that use the changed macro,
need to be recompiled, so everyone will need to do bootstrap after
pushing it, and many other problems.  I see no reason to use macros here.
We need to replace these macros with the elegant solution that we already
have of using action alists.

diff --git a/lisp/help.el b/lisp/help.el
index 42ff375565..24b5a26ed1 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -37,15 +37,8 @@
 (add-hook 'temp-buffer-setup-hook 'help-mode-setup)
 (add-hook 'temp-buffer-show-hook 'help-mode-finish)
 
-;; `help-window-point-marker' is a marker you can move to a valid
-;; position of the buffer shown in the help window in order to override
-;; the standard positioning mechanism (`point-min') chosen by
-;; `with-output-to-temp-buffer' and `with-temp-buffer-window'.
-;; `with-help-window' has this point nowhere before exiting.  Currently
-;; used by `view-lossage' to assert that the last keystrokes are always
-;; visible.
-(defvar help-window-point-marker (make-marker)
-  "Marker to override default `window-point' in help windows.")
+(define-obsolete-variable-alias 'help-window-point-marker
+  'delayed-window-point "27.1")
 
 (defvar help-window-old-frame nil
   "Frame selected at the time `with-help-window' is invoked.")
@@ -481,7 +474,10 @@ view-lossage
           (comment-indent)
 	  (forward-line 1)))
       ;; Show point near the end of "lossage", as we did in Emacs 24.
-      (set-marker help-window-point-marker (point)))))
+      (setq delayed-window-point (point))
+      (setq delayed-window-start (save-excursion
+                                   (forward-line (- 5 (window-height)))
+                                   (point))))))
 
 
 ;; Key bindings
@@ -1249,12 +1245,6 @@ help-window-setup
 	 (frame (window-frame window)))
 
     (when help-buffer
-      ;; Handle `help-window-point-marker'.
-      (when (eq (marker-buffer help-window-point-marker) help-buffer)
-	(set-window-point window help-window-point-marker)
-	;; Reset `help-window-point-marker'.
-	(set-marker help-window-point-marker nil))
-
       ;; If the help window appears on another frame, select it if
       ;; `help-window-select' is non-nil and give that frame input focus
       ;; too.  See also Bug#19012.
@@ -1326,9 +1316,6 @@ help-window-setup
 
 ;; (3) An option (customizable via `help-window-select') to select the
 ;;     help window automatically.
-
-;; (4) A marker (`help-window-point-marker') to move point in the help
-;;     window to an arbitrary buffer position.
 (defmacro with-help-window (buffer-or-name &rest body)
   "Evaluate BODY, send output to BUFFER-OR-NAME and show in a help window.
 This construct is like `with-temp-buffer-window' but unlike that
@@ -1339,9 +1326,6 @@ with-help-window
 Most of this is done by `help-window-setup', which see."
   (declare (indent 1) (debug t))
   `(progn
-     ;; Make `help-window-point-marker' point nowhere.  The only place
-     ;; where this should be set to a buffer position is within BODY.
-     (set-marker help-window-point-marker nil)
      (let ((temp-buffer-window-setup-hook
 	    (cons 'help-mode-setup temp-buffer-window-setup-hook))
 	   (temp-buffer-window-show-hook
diff --git a/lisp/window.el b/lisp/window.el
index a2335a6798..15a3665cec 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -6795,6 +6795,22 @@ window--even-window-sizes
            (/ (- (window-total-height window) (window-total-height)) 2))
         (error nil))))))
 
+(defvar delayed-window-point nil
+  "Marker to override default `window-point' in all windows.
+`delayed-window-point' is a buffer-local marker you can move to a valid
+position of the buffer shown in the window in order to override the standard
+positioning mechanism (`point-min') chosen by window displaying functions.")
+(make-variable-buffer-local 'delayed-window-point)
+(put 'delayed-window-point 'permanent-local t)
+
+(defvar delayed-window-start nil
+  "Marker to override default `delayed-window-start' in all windows.
+`delayed-window-start' is a buffer-local marker you can move to a valid
+position of the buffer shown in the window in order to override the standard
+positioning mechanism (`point-min') chosen by window displaying functions.")
+(make-variable-buffer-local 'delayed-window-start)
+(put 'delayed-window-start 'permanent-local t)
+
 (defun window--display-buffer (buffer window type &optional alist)
   "Display BUFFER in WINDOW.
 WINDOW must be a live window chosen by a buffer display action
@@ -6913,6 +6929,23 @@ window--display-buffer
 	(when (consp preserve-size)
 	  (window-preserve-size window t (car preserve-size))
 	  (window-preserve-size window nil (cdr preserve-size)))))
+
+      (when (local-variable-p 'delayed-window-start buffer)
+        (let ((window-start (buffer-local-value 'delayed-window-start buffer)))
+          (when (or (not (markerp window-start))
+                    (eq (marker-buffer window-start) buffer))
+            (set-window-start window window-start)))
+        (with-current-buffer buffer
+          (kill-local-variable 'delayed-window-start)))
+
+      (when (local-variable-p 'delayed-window-point buffer)
+        (let ((window-point (buffer-local-value 'delayed-window-point buffer)))
+          (when (or (not (markerp window-point))
+                    (eq (marker-buffer window-point) buffer))
+            (set-window-point window window-point)))
+        (with-current-buffer buffer
+          (kill-local-variable 'delayed-window-point)))
+
       ;; Assign any window parameters specified.
       (let ((parameters (cdr (assq 'window-parameters alist))))
         (dolist (parameter parameters)
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 61c13026cc..5070e4b310 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1017,9 +1017,11 @@ vc-git-print-log
     ;; If the buffer exists from a previous invocation it might be
     ;; read-only.
     (let ((inhibit-read-only t))
-      (with-current-buffer
-          buffer
-	(apply 'vc-git-command buffer
+      (with-current-buffer buffer
+	(insert (propertize "(Type 'd' here to show diffs with working version)\n"
+                            'font-lock-face 'shadow))
+        (setq delayed-window-start (point))
+        (apply 'vc-git-command buffer
 	       'async files
 	       (append
 		'("log" "--no-color")