bug#35860: Delayed window positioning after buffer display

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

There are commands that set point while the buffer is not displayed,
and after displaying the buffer point remains at an old position,
not where it was moved while the buffer was undisplayed.

One solution is to generalize the variable 'help-window-point-marker'
currently described as:

  ;; `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.")

A general name could be e.g. 'window-point-marker'.  Its counterpart is
also necessary with the name 'window-start-marker'.

Beside the recently occurred need in bug#35624 to use a new buffer-local
variable for window-start, other modes require this feature as well,
e.g. vc-annotate takes a long time to finish and set window point, but
after switching forth and back, it doesn't set point to the right line
after the buffer becomes visible.

bug#35860: Delayed window positioning after buffer display

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Wed, 22 May 2019 23:32:13 +0300
> 
> There are commands that set point while the buffer is not displayed,
> and after displaying the buffer point remains at an old position,
> not where it was moved while the buffer was undisplayed.

That's a feature, isn't it?  See switch-to-buffer-preserve-window-point.

> One solution is to generalize the variable 'help-window-point-marker'
> currently described as:
> 
>   ;; `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.")

Why not bind switch-to-buffer-preserve-window-point to nil instead?

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

>> There are commands that set point while the buffer is not displayed,
>> and after displaying the buffer point remains at an old position,
>> not where it was moved while the buffer was undisplayed.
>
> That's a feature, isn't it?  See switch-to-buffer-preserve-window-point.

IIUC, it tries to preserve the previous position of window-point,
but the required feature should set point when the buffer
is displayed for the first time.

>> One solution is to generalize the variable 'help-window-point-marker'
>> currently described as:
>> 
>>   ;; `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.")
>
> Why not bind switch-to-buffer-preserve-window-point to nil instead?

I don't know.  I tried to replace

  (set-marker help-window-point-marker (point))

with

  (goto-char (point-max))
  (setq-local switch-to-buffer-preserve-window-point nil)

and it doesn't set point to the end of the Help buffer
when it's displayed for the first time.

bug#35860: Delayed window positioning after buffer display

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 35860@debbugs.gnu.org
> Date: Thu, 23 May 2019 23:56:17 +0300
> 
> >>   ;; `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.")
> >
> > Why not bind switch-to-buffer-preserve-window-point to nil instead?
> 
> I don't know.  I tried to replace
> 
>   (set-marker help-window-point-marker (point))
> 
> with
> 
>   (goto-char (point-max))
>   (setq-local switch-to-buffer-preserve-window-point nil)
> 
> and it doesn't set point to the end of the Help buffer
> when it's displayed for the first time.

Can you show a recipe starting from "emacs -Q" that exhibits this
behavior.  It sounds like I'm confused about what exactly happens.

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

>> >>   ;; `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.")
>> >
>> > Why not bind switch-to-buffer-preserve-window-point to nil instead?
>>
>> I don't know.  I tried to replace
>>
>>   (set-marker help-window-point-marker (point))
>>
>> with
>>
>>   (goto-char (point-max))
>>   (setq-local switch-to-buffer-preserve-window-point nil)
>>
>> and it doesn't set point to the end of the Help buffer
>> when it's displayed for the first time.
>
> Can you show a recipe starting from "emacs -Q" that exhibits this
> behavior.  It sounds like I'm confused about what exactly happens.

0. emacs -Q
1. C-h l

Then point in the *Help* buffer should be at the bottom,
but when setting switch-to-buffer-preserve-window-point to nil
instead of using set-marker, point still is at the top of the buffer.

bug#35860: Delayed window positioning after buffer display

[martin rudalics]

 > 0. emacs -Q
 > 1. C-h l
 >
 > Then point in the *Help* buffer should be at the bottom,
 > but when setting switch-to-buffer-preserve-window-point to nil
 > instead of using set-marker, point still is at the top of the buffer.

I'm confused.  Here point in the *Help* buffer is always at bottom
regardless of how 'switch-to-buffer-preserve-window-point' is set.

martin

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

>> 0. emacs -Q
>> 1. C-h l
>>
>> Then point in the *Help* buffer should be at the bottom,
>> but when setting switch-to-buffer-preserve-window-point to nil
>> instead of using set-marker, point still is at the top of the buffer.
>
> I'm confused.  Here point in the *Help* buffer is always at bottom
> regardless of how 'switch-to-buffer-preserve-window-point' is set.

This effect is the result of this patch:

diff --git a/lisp/help.el b/lisp/help.el
index 42ff375565..0d76a45543 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -481,7 +481,7 @@ 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-local switch-to-buffer-preserve-window-point nil))))
 
 \f
 ;; Key bindings

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

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

>> 0. emacs -Q
>> 1. C-h l
>>
>> Then point in the *Help* buffer should be at the bottom,
>> but when setting switch-to-buffer-preserve-window-point to nil
>> instead of using set-marker, point still is at the top of the buffer.
>
> I'm confused.  Here point in the *Help* buffer is always at bottom
> regardless of how 'switch-to-buffer-preserve-window-point' is set.

Dmitry reverted my patch in bug#35624 because of no progress here,
so I needed to implement this quickly.  It generalizes
help-window-point-marker to a more general variable useful
not only in the Help buffer but everywhere, and all my tests show
that it works well:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: window-start-and-point.patch --]
[-- Type: text/x-diff, Size: 5371 bytes --]

diff --git a/lisp/help.el b/lisp/help.el
index 42ff375565..c046fe6a49 100644
--- a/lisp/help.el
+++ b/lisp/help.el
@@ -37,15 +37,7 @@
 (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.")
+(defvaralias 'help-window-point-marker 'window-point)
 
 (defvar help-window-old-frame nil
   "Frame selected at the time `with-help-window' is invoked.")
@@ -481,7 +473,8 @@ 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 window-point (point))
+      (setq window-start (save-excursion (forward-line (- 5 (window-height))) (point))))))
 
 \f
 ;; Key bindings
@@ -1249,12 +1242,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.
@@ -1327,7 +1314,7 @@ 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
+;; (4) A marker (`window-point') 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.
@@ -1339,9 +1326,9 @@ 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
+     ;; Make `window-point' point nowhere.  The only place
      ;; where this should be set to a buffer position is within BODY.
-     (set-marker help-window-point-marker nil)
+     (when window-point (setq window-point 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 69b1299dbd..e2796cecba 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 window-point nil
+  "Marker to override default `window-point' in help windows.
+`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 'window-point)
+(put 'window-point 'permanent-local t)
+
+(defvar window-start nil
+  "Marker to override default `window-start' in help windows.
+`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 'window-start)
+(put '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,15 @@ window--display-buffer
 	(when (consp preserve-size)
 	  (window-preserve-size window t (car preserve-size))
 	  (window-preserve-size window nil (cdr preserve-size)))))
+
+      (when window-start
+        (set-window-start window window-start)
+        (setq window-start nil))
+
+      (when window-point
+        (set-window-point window window-point)
+        (setq window-point nil))
+
       ;; 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..20a67dd6a7 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 "(Press 'd' here to see the diff against the current working revision)\n"
+                            'font-lock-face 'shadow))
+        (setq window-start (point))
+        (apply 'vc-git-command buffer
 	       'async files
 	       (append
 		'("log" "--no-color")

bug#35860: Delayed window positioning after buffer display

[martin rudalics]

 > Dmitry reverted my patch in bug#35624 because of no progress here,
 > so I needed to implement this quickly.  It generalizes
 > help-window-point-marker to a more general variable useful
 > not only in the Help buffer but everywhere, and all my tests show
 > that it works well:
 >

OK.  But

-;; (4) A marker (`help-window-point-marker') to move point in the help
+;; (4) A marker (`window-point') to move point in the help
  ;;     window to an arbitrary buffer position.

sounds irritating IMHO.  If it's used for help windows only, then why
rename it?  Otherwise, we should at least leave the '-marker' postfix
and provide some additional identification so maybe use something like
'temp-buffer-window-point-marker'

martin

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

> OK.  But
>
> -;; (4) A marker (`help-window-point-marker') to move point in the help
> +;; (4) A marker (`window-point') to move point in the help
>  ;;     window to an arbitrary buffer position.
>
> sounds irritating IMHO.  If it's used for help windows only, then why
> rename it?  Otherwise, we should at least leave the '-marker' postfix
> and provide some additional identification so maybe use something like
> 'temp-buffer-window-point-marker'

It's generalized now to be used not only for help windows.
The '-marker' postfix is not needed because it can be either
marker or integer, i.e. any value that set-window-point accepts
for its POSITION argument.

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

>> OK.  But
>>
>> -;; (4) A marker (`help-window-point-marker') to move point in the help
>> +;; (4) A marker (`window-point') to move point in the help
>>  ;;     window to an arbitrary buffer position.
>>
>> sounds irritating IMHO.  If it's used for help windows only, then why
>> rename it?  Otherwise, we should at least leave the '-marker' postfix
>> and provide some additional identification so maybe use something like
>> 'temp-buffer-window-point-marker'
>
> It's generalized now to be used not only for help windows.
> The '-marker' postfix is not needed because it can be either
> marker or integer, i.e. any value that set-window-point accepts
> for its POSITION argument.

What do you think about a more appropriate name
next-display-window-point?  This name says that its effect
will occur at the next display of the buffer in the window.

bug#35860: Delayed window positioning after buffer display

[martin rudalics]

 >> -;; (4) A marker (`help-window-point-marker') to move point in the help
 >> +;; (4) A marker (`window-point') to move point in the help
 >>   ;;     window to an arbitrary buffer position.
 >>
 >> sounds irritating IMHO.  If it's used for help windows only, then why
 >> rename it?  Otherwise, we should at least leave the '-marker' postfix
 >> and provide some additional identification so maybe use something like
 >> 'temp-buffer-window-point-marker'
 >
 > It's generalized now to be used not only for help windows.

Then the comment above is misleading.

 > The '-marker' postfix is not needed because it can be either
 > marker or integer, i.e. any value that set-window-point accepts
 > for its POSITION argument.

Hmmm.  "...-window-point-position" sounds silly.

martin

bug#35860: Delayed window positioning after buffer display

[martin rudalics]

 > What do you think about a more appropriate name
 > next-display-window-point?  This name says that its effect
 > will occur at the next display of the buffer in the window.

Generalizing 'help-window-point-marker' is problematic.  "at the next
display of the buffer in the window" says it already: What is the
"next display" (should it be "redisplay"?) which "buffer" and which
"window" are meant?

martin

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

>> What do you think about a more appropriate name
>> next-display-window-point?  This name says that its effect
>> will occur at the next display of the buffer in the window.
>
> Generalizing 'help-window-point-marker' is problematic.  "at the next
> display of the buffer in the window" says it already: What is the
> "next display" (should it be "redisplay"?) which "buffer" and which
> "window" are meant?

Generalized version does exactly what 'help-window-point-marker'
used to do, i.e.:

  ;; `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.")

but for all windows instead of only help windows, i.e.:

    "Marker to override default `window-point' in all windows."

So do you think about just removing the prefix `help-' from the name:
`window-point-marker'?

bug#35860: Delayed window positioning after buffer display

[martin rudalics]

 > Generalized version does exactly what 'help-window-point-marker'
 > used to do, i.e.:
 >
 >    ;; `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.")
 >
 > but for all windows instead of only help windows, i.e.:
 >
 >      "Marker to override default `window-point' in all windows."
 >
 > So do you think about just removing the prefix `help-' from the name:
 > `window-point-marker'?

Currently 'temp-buffer-window-show' has

       (goto-char (point-min))

and 'internal-temp-output-buffer-show' has

	  (set-window-start window (point-min) t)
	  ;; This should not be necessary.
	  (set-window-point window (point-min))

For help windows (windows showing *Help*) 'help-window-point-marker'
allows to override these assignments by a suitable assignment for that
variable.  It's hackish but still done in a pretty restricitve fashion
as follows.

First 'help-window-point-marker' is reset in 'with-help-window' via

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

- a security guard, because that marker should be nil at that time
anyway.

Then 'help-window-point-marker' may be set by the BODY of
'with-help-window' and 'help-window-setup' will pick that up guarded
as

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

thus (1) checking whether the marker buffer matches and (2)
immediately resetting that marker to nil.

Can you provide equivalent security guards when generalizing that
variable?  For example, you proposed

  (defun window--display-buffer (buffer window type &optional alist)
    "Display BUFFER in WINDOW.
[...]
+
+      (when window-start
+        (set-window-start window window-start)
+        (setq window-start nil))
+
+      (when window-point
+        (set-window-point window window-point)
+        (setq window-point nil))

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.

The *Help* buffer is special because it's used in a centralized way
for any help Emacs provides (that's also why it has a browsable
history).  Buffers displayed by 'display-buffer' OTOH don't obey any
such restricitions.  They don't even have a BODY as the only place
where markers as the ones you propose can be set.

Also 'temp-buffer-window-show' and 'internal-temp-output-buffer-show'
are special because they explicitly wants to set window point to BOB.
So there's some predefined mechanism we (1) are aware of and (2) want
to override deliberately.

So we'd carefully have to examine first how the mechanism you propose
could be abused, how to handle any errors in using and failing to
reset these markers and last but not least tell why we don't provide
'window-point' and 'window-start' action alist entries instead of such
global variables.

martin

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

> Currently 'temp-buffer-window-show' has
>
>       (goto-char (point-min))
>
> and 'internal-temp-output-buffer-show' has
>
> 	  (set-window-start window (point-min) t)
> 	  ;; This should not be necessary.
> 	  (set-window-point window (point-min))

BTW, there is a strange comment before the first line:

	  ;; Don't try this with NOFORCE non-nil!
	  (set-window-start window (point-min) t)

but its NOFORCE is non-nil here.  Is this a FIXME-like reminder?

> Then 'help-window-point-marker' may be set by the BODY of
> 'with-help-window' and 'help-window-setup' will pick that up guarded
> as
>
>       (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))
>
> thus (1) checking whether the marker buffer matches and (2)
> immediately resetting that marker to nil.
>
> Can you provide equivalent security guards when generalizing that
> variable?

In my previous patch there is no need to check for the buffer,
because a new general variable is declared as buffer-local.

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

> So we'd carefully have to examine first how the mechanism you propose
> could be abused, how to handle any errors in using and failing to
> reset these markers and last but not least tell why we don't provide
> 'window-point' and 'window-start' action alist entries instead of such
> global variables.

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.

bug#35860: Delayed window positioning after buffer display

[martin rudalics]

 > BTW, there is a strange comment before the first line:
 >
 > 	  ;; Don't try this with NOFORCE non-nil!
 > 	  (set-window-start window (point-min) t)
 >
 > but its NOFORCE is non-nil here.  Is this a FIXME-like reminder?

Rather a FIXME-like remainder.  I probably did try to fix something
here but don't recall any more if the comment applied to the version
before that fix or should apply to the present version (in which case
it is obviously wrong).

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

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

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

martin

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

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

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: window-start-and-point.2.patch --]
[-- Type: text/x-diff, Size: 6044 bytes --]

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))))))
 
 \f
 ;; 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")

bug#35860: Delayed window positioning after buffer display

[martin rudalics]

 > +(defvar delayed-window-point nil

Name space conventions would recommend 'window-delayed-point'.

 > +  "Marker to override default `window-point' in all windows.

Why "marker" and why "all windows"?  As for the former, I think it
makes sense because then one can specify an insertion type as well.
But IIRC you earlier wanted to allow plain integers here too?

 > +`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.")

IIUC there is no such "mechanism" except for buffers just created.
And what about 'switch-to-buffer-preserve-window-point'?  "window ???
displaying functions" implicitly hints at the fact that the mechanism
works only when the buffer is shown by 'display-buffer'.  But are we
sure that this restriction always applies?

Otherwise the patch looks good.

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

Until Lars comes up with a solution that detects any changed macros,
finds their users, and orderly recompiles the respective files.  But
I'm afraid he's turning back to his books for now.

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

Macros are useful in as much as they allow to specify the evaluation
order of their arguments.  Action alists are dumb in this regard.

martin

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

>> +(defvar delayed-window-point nil
>
> Name space conventions would recommend 'window-delayed-point'.

Then 'window-delayed-start' would be too ambiguous.  Maybe then
'window-start-delayed' and 'window-point-delayed'.

>> +  "Marker to override default `window-point' in all windows.
>
> Why "marker" and why "all windows"?  As for the former, I think it
> makes sense because then one can specify an insertion type as well.
> But IIRC you earlier wanted to allow plain integers here too?

Yes, plain integers should be allowed as well.

>> +`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.")
>
> IIUC there is no such "mechanism" except for buffers just created.
> And what about 'switch-to-buffer-preserve-window-point'?  "window ???
> displaying functions" implicitly hints at the fact that the mechanism
> works only when the buffer is shown by 'display-buffer'.  But are we
> sure that this restriction always applies?

Maybe the same should be allowed for 'switch-to-buffer' as well,
and even for buffers displayed by set-window-configuration.

>> 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.
>
> Macros are useful in as much as they allow to specify the evaluation
> order of their arguments.  Action alists are dumb in this regard.

The problem is that currently the whole cycle of buffer displaying
doesn't allow executing arbitrary code at different stages:
there are no pre-display-buffer and post-display-buffer hooks.

Adding such hooks would allow to get rid of macros easily.
For example, instead of using two variables 'window-point-delayed'
and 'window-start-delayed', a pre-display hook in 'view-lossage'
will be able just to set a buffer-local run-once post-display hook
that will adjust the window positions after the buffer is displayed
in the window.

Another candidate that can be placed into such post-display hook is
fit-window-to-buffer for cases when adding an action alist
as an arg of display-buffer call is not possible.

And vice versa, pre/post-display actions could be specified
in an action alist of display-buffer, when using hooks is not possible.

bug#35860: Delayed window positioning after buffer display

[martin rudalics]

 > Adding such hooks would allow to get rid of macros easily.
 > For example, instead of using two variables 'window-point-delayed'
 > and 'window-start-delayed', a pre-display hook in 'view-lossage'
 > will be able just to set a buffer-local run-once post-display hook
 > that will adjust the window positions after the buffer is displayed
 > in the window.

The problem is to properly clear these hooks to avoid that they affect
future, unrelated calls of 'display-buffer'.  In most cases they are
supposed to be one-shots so a 'condition-case' in 'display-buffer'
should suffice, but it's a bit abnormal for hooks to get auto-cleared.

martin

bug#35860: Delayed window positioning after buffer display

[Juri Linkov]

>> +(defvar delayed-window-point nil
>
> Name space conventions would recommend 'window-delayed-point'.

BTW, we had plans to rename 'window--display-buffer'.
But shouldn't it rather use 'display-buffer-' prefix?
Maybe, 'display-buffer-in-window'?

bug#35860: Delayed window positioning after buffer display

[martin rudalics]

 > BTW, we had plans to rename 'window--display-buffer'.
 > But shouldn't it rather use 'display-buffer-' prefix?
 > Maybe, 'display-buffer-in-window'?

Back in January I wrote ...

 > The cleanup has been done.  The final message of that subthread says:
 >
 >   Anyone interested in renaming 'window--display-buffer' - please go
 >   ahead.  I think a 'display-buffer-' prefix should then be appropriate.
 >
 > What I meant there is that calling it say 'display-buffer-in-window'
 > would be appropriate but the final name is up to its clients.

... so feel free to do that.

BTW, doesn't your earlier remark ...

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

... miss one aspect: 'display-buffer-overriding-action' can always be
used to set and/or override any 'window-point' or 'window-start' alist
entries.

martin