bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Do-not-scroll-other-window-in-daemon-frame.patch --]
[-- Type: text/x-diff, Size: 1506 bytes --]

From 0471afc8da07d2dc397f2cf833d9f8bccfbe31c1 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Mon, 22 Jan 2018 02:57:08 +0000
Subject: [PATCH] Do not scroll other window in daemon frame

* src/window.c (Fother_window_for_scrolling): Ignore daemon frame
when searching other frames for a window.
---
 src/window.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/window.c b/src/window.c
index 08c3f32dff..c050b4e6c9 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5704,6 +5704,7 @@ specifies the window.  This takes precedence over
   (void)
 {
   Lisp_Object window;
+  struct frame *f;
 
   if (MINI_WINDOW_P (XWINDOW (selected_window))
       && !NILP (Vminibuf_scroll_window))
@@ -5725,11 +5726,14 @@ specifies the window.  This takes precedence over
 
       if (EQ (window, selected_window))
 	/* That didn't get us anywhere; look for a window on another
-           visible frame.  */
+           visible frame, ignoring the daemon's frame.  */
 	do
-	  window = Fnext_window (window, Qnil, Qt);
-	while (! FRAME_VISIBLE_P (XFRAME (WINDOW_FRAME (XWINDOW (window))))
-	       && ! EQ (window, selected_window));
+          {
+            window = Fnext_window (window, Qnil, Qt);
+            f = WINDOW_XFRAME (XWINDOW (window));
+          }
+        while (! ((FRAME_VISIBLE_P (f) && ! (IS_DAEMON && FRAME_INITIAL_P (f)))
+                  || EQ (window, selected_window)));
     }
 
   CHECK_LIVE_WINDOW (window);
-- 
2.15.1


[-- Attachment #2: Type: text/plain, Size: 1408 bytes --]


Consider the following:

1. emacs -Q --daemon

2. emacsclient --create-frame

3. (selected-window)
     => #<window 3 on *scratch*>

4. (other-window-for-scrolling)
     => #<window 1 on *scratch*>

5. (frame-terminal (window-frame (other-window-for-scrolling)))
     => #<terminal 0 on initial_terminal>

This can result in commands scroll-other-window and
scroll-other-window-down scrolling the "phantom" initial frame of the
daemon instead of signalling a "no other window" error.  I imagine this
behaviour is never desirable.

I believe this issue and its potential resolution share some ground with
bug#27210 in that the daemon frame is counterintuitively considered
visible in the following repeat-until condition in
Fother_window_for_scrolling:

  do
    window = Fnext_window (window, Qnil, Qt);
  while (! FRAME_VISIBLE_P (XFRAME (WINDOW_FRAME (XWINDOW (window))))
         && ! EQ (window, selected_window));

Until a more thorough review of the visibility of the initial daemon
frame is reached, I propose the attached patch to additionally ignore
the daemon frame in the aforementioned repeat-until condition.  I assume
the overhead of the additional loop logic and IS_DAEMON invariant is
negligible compared to the rest of the function.

An alternative approach to the attached patch would be to limit "other
window scrolling" to frames in the current terminal, as per the spirit
of bug#5616:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: diff.diff --]
[-- Type: text/x-diff, Size: 643 bytes --]

diff --git a/src/window.c b/src/window.c
index 08c3f32dff..328fcb3829 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5725,11 +5725,8 @@ specifies the window.  This takes precedence over
 
       if (EQ (window, selected_window))
 	/* That didn't get us anywhere; look for a window on another
-           visible frame.  */
-	do
-	  window = Fnext_window (window, Qnil, Qt);
-	while (! FRAME_VISIBLE_P (XFRAME (WINDOW_FRAME (XWINDOW (window))))
-	       && ! EQ (window, selected_window));
+           visible frame on the current terminal.  */
+        window = Fnext_window (window, Qnil, Qvisible);
     }
 
   CHECK_LIVE_WINDOW (window);

[-- Attachment #4: Type: text/plain, Size: 1016 bytes --]


or at least prioritise the current terminal's frames.  WDYT?

Thanks,

-- 
Basil

In GNU Emacs 27.0.50 (build 25, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2018-01-22 built on thunk
Repository revision: c42959cc206bcb52baffd45f892da1b767f0f8c1
Windowing system distributor 'The X.Org Foundation', version 11.0.11905000
System Description: Debian GNU/Linux testing (buster)

Configured using:
 'configure --config-cache --prefix=/home/blc/.local --with-mailutils
 --with-x-toolkit=lucid --with-modules --with-file-notification=yes
 --with-x 'CFLAGS=-flto -fomit-frame-pointer -march=native -maes -mavx
 -mcrc32 -mf16c -mfpmath=sse -mfsgsbase -mfxsr -minline-all-stringops
 -mmmx -mpclmul -mpopcnt -msahf -msse4.2 -mxsave -mxsaveopt -mvzeroupper
 -O2 -pipe' LDFLAGS=-flto'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS
NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS LUCID X11 MODULES THREADS LIBSYSTEMD JSON LCMS2

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[martin rudalics]

 > I believe this issue and its potential resolution share some ground with
 > bug#27210 in that the daemon frame is counterintuitively considered
 > visible in the following repeat-until condition in
 > Fother_window_for_scrolling:
 >
 >    do
 >      window = Fnext_window (window, Qnil, Qt);
 >    while (! FRAME_VISIBLE_P (XFRAME (WINDOW_FRAME (XWINDOW (window))))
 >           && ! EQ (window, selected_window));
 >
 > Until a more thorough review of the visibility of the initial daemon
 > frame is reached, I propose the attached patch to additionally ignore
 > the daemon frame in the aforementioned repeat-until condition.  I assume
 > the overhead of the additional loop logic and IS_DAEMON invariant is
 > negligible compared to the rest of the function.
 >
 > An alternative approach to the attached patch would be to limit "other
 > window scrolling" to frames in the current terminal, as per the spirit
 > of bug#5616:
 >
 >
 >
 >
 > or at least prioritise the current terminal's frames.  WDYT?

I think all your proposals are good and make sense.  Which one would
you like most?

martin

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Basil L. Contovounesios]

martin rudalics <rudalics@gmx.at> writes:

> I think all your proposals are good and make sense.  Which one would
> you like most?

I can't imagine a scenario where I would want to scroll a frame on a
different terminal, so I personally prefer the bug#56160-style
restriction to the current terminal for both its semantic and syntactic
simplicity.

OTOH, the other two approaches preserve established behaviour and also
have the following merits:

1. Ignoring the daemon frame acts as a reminder to review the daemon
   frame visibility issue discussed in bug#27210.

2. Prioritising frames on the current terminal before falling back to
   frames on all terminals, bar the daemon frame, combines the benefits
   of the other two approaches (to an extent) at the expense of greater
   code complexity.

In other words, I defer to others to chime in and/or make an executive
decision. :)

Thanks,

-- 
Basil

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Noam Postavsky]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> I can't imagine a scenario where I would want to scroll a frame on a
> different terminal, so I personally prefer the bug#56160-style
> restriction to the current terminal for both its semantic and syntactic
> simplicity.

I believe some people use Emacs in a single-window-per-frame style and
manage the frames via the window manager, so scrolling in other
terminals sounds useful for that kind of scenario.

> OTOH, the other two approaches preserve established behaviour and also
> have the following merits:
>
> 1. Ignoring the daemon frame acts as a reminder to review the daemon
>    frame visibility issue discussed in bug#27210.

Thanks for reminding us about it.  :)

I'm still of the opinion that the daemon frame should be marked
invisible (what with it being not visible and all).

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Basil L. Contovounesios]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> I believe some people use Emacs in a single-window-per-frame style and
> manage the frames via the window manager, so scrolling in other
> terminals sounds useful for that kind of scenario.

I am one such user, but all frames managed by my window manager live in
the same terminal and I would be surprised if this weren't the case in
most setups.  Nevertheless, I suppose it is conceivable that someone may
have such an unusual setup one day...

> I'm still of the opinion that the daemon frame should be marked
> invisible (what with it being not visible and all).

I'm only beginning to look into the relevant code, but this seems
intuitively right to me too.

-- 
Basil

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Noam Postavsky]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Noam Postavsky <npostavs@users.sourceforge.net> writes:
>
>> I believe some people use Emacs in a single-window-per-frame style and
>> manage the frames via the window manager, so scrolling in other
>> terminals sounds useful for that kind of scenario.
>
> I am one such user, but all frames managed by my window manager live in
> the same terminal and I would be surprised if this weren't the case in
> most setups.  Nevertheless, I suppose it is conceivable that someone may
> have such an unusual setup one day...

Oops, I was confused between Emacs 'frame' and 'terminal'.  I agree that
scrolling in another terminal is not very likely to be useful.

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Basil L. Contovounesios]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> I can't imagine a scenario where I would want to scroll a frame on a
> different terminal, so I personally prefer the bug#56160-style
> restriction to the current terminal for both its semantic and syntactic
> simplicity.

In the initial report I wrote bug#5616 and here I misspelt it as
bug#56160, when in fact I have been thinking of bug#3442 throughout.

Sorry about the confusion.

-- 
Basil

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Limit "other window" scrolling to current terminal --]
[-- Type: text/x-diff, Size: 1598 bytes --]

From da3f96bac102e731b5d7d99d26a53921d431c0ad Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Mon, 30 Apr 2018 18:02:15 +0100
Subject: [PATCH 1/4] Limit "other window" scrolling to current terminal

* src/window.c (Fother_window_for_scrolling): Limit next-window
search to visible frames on the current terminal. (bug#30207)
---
 src/window.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/window.c b/src/window.c
index e6d0280d9b..59c9422029 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5709,8 +5709,7 @@ specifies the window.  This takes precedence over
       && !NILP (Vminibuf_scroll_window))
     window = Vminibuf_scroll_window;
   /* If buffer is specified and live, scroll that buffer.  */
-  else if (!NILP (Vother_window_scroll_buffer)
-	   && BUFFERP (Vother_window_scroll_buffer)
+  else if (BUFFERP (Vother_window_scroll_buffer)
 	   && BUFFER_LIVE_P (XBUFFER (Vother_window_scroll_buffer)))
     {
       window = Fget_buffer_window (Vother_window_scroll_buffer, Qnil);
@@ -5725,11 +5724,8 @@ specifies the window.  This takes precedence over
 
       if (EQ (window, selected_window))
 	/* That didn't get us anywhere; look for a window on another
-           visible frame.  */
-	do
-	  window = Fnext_window (window, Qnil, Qt);
-	while (! FRAME_VISIBLE_P (XFRAME (WINDOW_FRAME (XWINDOW (window))))
-	       && ! EQ (window, selected_window));
+           visible frame on the current terminal.  */
+        window = Fnext_window (window, Qnil, Qvisible);
     }
 
   CHECK_LIVE_WINDOW (window);
-- 
2.17.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Improve documentation for "other window" scrolling --]
[-- Type: text/x-diff, Size: 6181 bytes --]

From 5d47174ea4205742094e3e8f8bba38f3fc9bef8a Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Mon, 30 Apr 2018 18:06:59 +0100
Subject: [PATCH 2/4] Improve documentation for "other window" scrolling

* doc/emacs/windows.texi (Other Window):
* doc/lispref/windows.texi (Textual Scrolling):
Document scroll-other-window-down.
* doc/lispref/minibuf.texi (Minibuffer Misc):
Cross-reference minibuffer-scroll-window with Textual Scrolling.
* src/window.c (Fother_window_for_scrolling):
Clarify how "other window" is determined in docstring.
(Fscroll_other_window):
Simplify docstring, pointing to that of Fother_window_for_scrolling.
---
 doc/emacs/windows.texi   | 18 ++++++++++++------
 doc/lispref/minibuf.texi |  2 +-
 doc/lispref/windows.texi |  7 +++++++
 src/window.c             | 25 ++++++++++++-------------
 4 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/doc/emacs/windows.texi b/doc/emacs/windows.texi
index 809363305f..cbcf8f645f 100644
--- a/doc/emacs/windows.texi
+++ b/doc/emacs/windows.texi
@@ -157,7 +157,9 @@ Other Window
 @item C-x o
 Select another window (@code{other-window}).
 @item C-M-v
-Scroll the next window (@code{scroll-other-window}).
+Scroll the next window upward (@code{scroll-other-window}).
+@item C-M-S-v
+Scroll the next window downward (@code{scroll-other-window-down}).
 @item mouse-1
 @kbd{mouse-1}, in the text area of a window, selects the window and
 moves point to the position clicked.  Clicking in the mode line
@@ -181,13 +183,17 @@ Other Window
 
 @kindex C-M-v
 @findex scroll-other-window
+@kindex C-M-S-v
+@findex scroll-other-window-down
   The usual scrolling commands (@pxref{Display}) apply to the selected
-window only, but there is one command to scroll the next window.
+window only, but there are two commands to scroll the next window.
 @kbd{C-M-v} (@code{scroll-other-window}) scrolls the window that
-@kbd{C-x o} would select.  It takes arguments, positive and negative,
-like @kbd{C-v}.  (In the minibuffer, @kbd{C-M-v} scrolls the help
-window associated with the minibuffer, if any, rather than the next
-window in the standard cyclic order; @pxref{Minibuffer Edit}.)
+@kbd{C-x o} would select upward.  It takes arguments, positive and
+negative, like @kbd{C-v}.  (In the minibuffer, @kbd{C-M-v} scrolls the
+help window associated with the minibuffer, if any, rather than the
+next window in the standard cyclic order; @pxref{Minibuffer Edit}.)
+@kbd{C-M-S-v} (@code{scroll-other-window-down}) scrolls the next
+window downward in a similar way.
 
 @vindex mouse-autoselect-window
   If you set @code{mouse-autoselect-window} to a non-@code{nil} value,
diff --git a/doc/lispref/minibuf.texi b/doc/lispref/minibuf.texi
index 338e03ef74..889b64af8a 100644
--- a/doc/lispref/minibuf.texi
+++ b/doc/lispref/minibuf.texi
@@ -2462,7 +2462,7 @@ Minibuffer Misc
 @anchor{Definition of minibuffer-scroll-window}
 If the value of this variable is non-@code{nil}, it should be a window
 object.  When the function @code{scroll-other-window} is called in the
-minibuffer, it scrolls this window.
+minibuffer, it scrolls this window (@pxref{Textual Scrolling}).
 @end defvar
 
 @defun minibuffer-selected-window
diff --git a/doc/lispref/windows.texi b/doc/lispref/windows.texi
index f5de2fc90b..315ffd4f48 100644
--- a/doc/lispref/windows.texi
+++ b/doc/lispref/windows.texi
@@ -4028,6 +4028,13 @@ Textual Scrolling
 @samp{End of buffer}.
 @end deffn
 
+@deffn Command scroll-other-window-down &optional count
+This function scrolls the text in another window downward @var{count}
+lines.  Negative values of @var{count}, or @code{nil}, are handled as
+in @code{scroll-down}.  In other respects, it behaves the same way as
+@code{scroll-other-window} does.
+@end deffn
+
 @defvar other-window-scroll-buffer
 If this variable is non-@code{nil}, it tells @code{scroll-other-window}
 which buffer's window to scroll.
diff --git a/src/window.c b/src/window.c
index 59c9422029..af317674bb 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5696,11 +5696,12 @@ When calling from a program, supply as argument a number, nil, or `-'.  */)
 \f
 DEFUN ("other-window-for-scrolling", Fother_window_for_scrolling, Sother_window_for_scrolling, 0, 0, 0,
        doc: /* Return the other window for \"other window scroll\" commands.
-If `other-window-scroll-buffer' is non-nil, a window
-showing that buffer is used.
 If in the minibuffer, `minibuffer-scroll-window' if non-nil
-specifies the window.  This takes precedence over
-`other-window-scroll-buffer'.  */)
+specifies the window.
+Otherwise, if `other-window-scroll-buffer' is non-nil, a window
+showing that buffer is used, popping the buffer up if necessary.
+Finally, look for a neighboring window on the selected frame,
+followed by all visible frames on the current terminal.  */)
   (void)
 {
   Lisp_Object window;
@@ -5739,16 +5740,14 @@ specifies the window.  This takes precedence over
 DEFUN ("scroll-other-window", Fscroll_other_window, Sscroll_other_window, 0, 1, "P",
        doc: /* Scroll next window upward ARG lines; or near full screen if no ARG.
 A near full screen is `next-screen-context-lines' less than a full screen.
-The next window is the one below the current one; or the one at the top
-if the current one is at the bottom.  Negative ARG means scroll downward.
-If ARG is the atom `-', scroll downward by nearly full screen.
-When calling from a program, supply as argument a number, nil, or `-'.
+Negative ARG means scroll downward.  If ARG is the atom `-', scroll
+downward by nearly full screen.  When calling from a program, supply
+as argument a number, nil, or `-'.
 
-If `other-window-scroll-buffer' is non-nil, scroll the window
-showing that buffer, popping the buffer up if necessary.
-If in the minibuffer, `minibuffer-scroll-window' if non-nil
-specifies the window to scroll.  This takes precedence over
-`other-window-scroll-buffer'.  */)
+The next window is usually the one below the current one;
+or the one at the top if the current one is at the bottom.
+It is determined by the function `other-window-for-scrolling',
+which see.  */)
   (Lisp_Object arg)
 {
   Lisp_Object window;
-- 
2.17.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Simplify "other window" bob/eob motion commands --]
[-- Type: text/x-diff, Size: 2215 bytes --]

From 8e465eb0a766c01901618a6308c9c8920698edf1 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 1 May 2018 16:05:52 +0100
Subject: [PATCH 3/4] Simplify "other window" bob/eob motion commands

* lisp/window.el (beginning-of-buffer-other-window)
(end-of-buffer-other-window): Simplify via with-selected-window.
---
 lisp/window.el | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/lisp/window.el b/lisp/window.el
index 8055e5babc..bc2008e1d9 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -8939,19 +8939,12 @@ beginning-of-buffer-other-window
 Leave mark at previous position.
 With arg N, put point N/10 of the way from the true beginning."
   (interactive "P")
-  (let ((orig-window (selected-window))
-	(window (other-window-for-scrolling)))
-    ;; We use unwind-protect rather than save-window-excursion
-    ;; because the latter would preserve the things we want to change.
-    (unwind-protect
-	(progn
-	  (select-window window)
-	  ;; Set point and mark in that window's buffer.
-	  (with-no-warnings
-	   (beginning-of-buffer arg))
-	  ;; Set point accordingly.
-	  (recenter '(t)))
-      (select-window orig-window))))
+  (with-selected-window (other-window-for-scrolling)
+    ;; Set point and mark in that window's buffer.
+    (with-no-warnings
+      (beginning-of-buffer arg))
+    ;; Set point accordingly.
+    (recenter '(t))))
 
 (defun end-of-buffer-other-window (arg)
   "Move point to the end of the buffer in the other window.
@@ -8959,15 +8952,10 @@ end-of-buffer-other-window
 With arg N, put point N/10 of the way from the true end."
   (interactive "P")
   ;; See beginning-of-buffer-other-window for comments.
-  (let ((orig-window (selected-window))
-	(window (other-window-for-scrolling)))
-    (unwind-protect
-	(progn
-	  (select-window window)
-	  (with-no-warnings
-	   (end-of-buffer arg))
-	  (recenter '(t)))
-      (select-window orig-window))))
+  (with-selected-window (other-window-for-scrolling)
+    (with-no-warnings
+      (end-of-buffer arg))
+    (recenter '(t))))
 \f
 (defvar mouse-autoselect-window-timer nil
   "Timer used by delayed window autoselection.")
-- 
2.17.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Rewrite scroll-other-window-down in C --]
[-- Type: text/x-diff, Size: 6553 bytes --]

From 877b5a5690bdfc4bd34516189dd858f66275b75b Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 1 May 2018 13:48:30 +0100
Subject: [PATCH 4/4] Rewrite scroll-other-window-down in C (bug#30207)

* lisp/window.el (scroll-other-window-down):
Move to src/window.c as Fscroll_other_window_down.
* src/window.c (scroll_command): Generalise for arbitrary windows.
(Fscroll_up, Fscroll_down): Use scroll_command with selected_window.
(Fscroll_other_window, Fscroll_other_window_down):
Rewrite in terms of scroll_command.
(syms_of_window): Add Sscroll_other_window_down.
---
 lisp/window.el | 11 -------
 src/window.c   | 85 ++++++++++++++++++++++++++------------------------
 2 files changed, 45 insertions(+), 51 deletions(-)

diff --git a/lisp/window.el b/lisp/window.el
index bc2008e1d9..085e51646f 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -8923,17 +8923,6 @@ scroll-down-line
 (put 'scroll-down-line 'scroll-command t)
 
 \f
-(defun scroll-other-window-down (&optional lines)
-  "Scroll the \"other window\" down.
-For more details, see the documentation for `scroll-other-window'."
-  (interactive "P")
-  (scroll-other-window
-   ;; Just invert the argument's meaning.
-   ;; We can do that without knowing which window it will be.
-   (if (eq lines '-) nil
-     (if (null lines) '-
-       (- (prefix-numeric-value lines))))))
-
 (defun beginning-of-buffer-other-window (arg)
   "Move point to the beginning of the buffer in the other window.
 Leave mark at previous position.
diff --git a/src/window.c b/src/window.c
index af317674bb..f654d87e14 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5634,35 +5634,54 @@ window_scroll_line_based (Lisp_Object window, int n, bool whole, bool noerror)
 }
 
 
-/* Scroll selected_window up or down.  If N is nil, scroll a
+/* Scroll WINDOW up or down.  If N is nil, scroll upward by a
    screen-full which is defined as the height of the window minus
-   next_screen_context_lines.  If N is the symbol `-', scroll.
-   DIRECTION may be 1 meaning to scroll down, or -1 meaning to scroll
-   up.  This is the guts of Fscroll_up and Fscroll_down.  */
+   next_screen_context_lines.  If N is the symbol `-', scroll downward
+   by a screen-full.  DIRECTION may be 1 meaning to scroll down, or -1
+   meaning to scroll up.  */
 
 static void
-scroll_command (Lisp_Object n, int direction)
+scroll_command (Lisp_Object window, Lisp_Object n, int direction)
 {
+  struct window *w;
+  bool other_window;
   ptrdiff_t count = SPECPDL_INDEX ();
 
   eassert (eabs (direction) == 1);
 
-  /* If selected window's buffer isn't current, make it current for
+  w = XWINDOW (window);
+  other_window = ! EQ (window, selected_window);
+
+  /* If given window's buffer isn't current, make it current for
      the moment.  But don't screw up if window_scroll gets an error.  */
-  if (XBUFFER (XWINDOW (selected_window)->contents) != current_buffer)
+  if (XBUFFER (w->contents) != current_buffer)
     {
       record_unwind_protect (save_excursion_restore, save_excursion_save ());
-      Fset_buffer (XWINDOW (selected_window)->contents);
+      Fset_buffer (w->contents);
+    }
+
+  if (other_window)
+    {
+      SET_PT_BOTH (marker_position (w->pointm),
+                   marker_byte_position (w->pointm));
+      SET_PT_BOTH (marker_position (w->old_pointm),
+                   marker_byte_position (w->old_pointm));
     }
 
   if (NILP (n))
-    window_scroll (selected_window, direction, true, false);
+    window_scroll (window, direction, true, false);
   else if (EQ (n, Qminus))
-    window_scroll (selected_window, -direction, true, false);
+    window_scroll (window, -direction, true, false);
   else
     {
       n = Fprefix_numeric_value (n);
-      window_scroll (selected_window, XINT (n) * direction, false, false);
+      window_scroll (window, XINT (n) * direction, false, false);
+    }
+
+  if (other_window)
+    {
+      set_marker_both (w->pointm, Qnil, PT, PT_BYTE);
+      set_marker_both (w->old_pointm, Qnil, PT, PT_BYTE);
     }
 
   unbind_to (count, Qnil);
@@ -5677,7 +5696,7 @@ If ARG is the atom `-', scroll downward by nearly full screen.
 When calling from a program, supply as argument a number, nil, or `-'.  */)
   (Lisp_Object arg)
 {
-  scroll_command (arg, 1);
+  scroll_command (selected_window, arg, 1);
   return Qnil;
 }
 
@@ -5690,7 +5709,7 @@ If ARG is the atom `-', scroll upward by nearly full screen.
 When calling from a program, supply as argument a number, nil, or `-'.  */)
   (Lisp_Object arg)
 {
-  scroll_command (arg, -1);
+  scroll_command (selected_window, arg, -1);
   return Qnil;
 }
 \f
@@ -5750,36 +5769,21 @@ It is determined by the function `other-window-for-scrolling',
 which see.  */)
   (Lisp_Object arg)
 {
-  Lisp_Object window;
-  struct window *w;
   ptrdiff_t count = SPECPDL_INDEX ();
-
-  window = Fother_window_for_scrolling ();
-  w = XWINDOW (window);
-
-  /* Don't screw up if window_scroll gets an error.  */
-  record_unwind_protect (save_excursion_restore, save_excursion_save ());
-
-  Fset_buffer (w->contents);
-  SET_PT_BOTH (marker_position (w->pointm), marker_byte_position (w->pointm));
-  SET_PT_BOTH (marker_position (w->old_pointm), marker_byte_position (w->old_pointm));
-
-  if (NILP (arg))
-    window_scroll (window, 1, true, true);
-  else if (EQ (arg, Qminus))
-    window_scroll (window, -1, true, true);
-  else
-    {
-      if (CONSP (arg))
-	arg = XCAR (arg);
-      CHECK_NUMBER (arg);
-      window_scroll (window, XINT (arg), false, true);
-    }
-
-  set_marker_both (w->pointm, Qnil, PT, PT_BYTE);
-  set_marker_both (w->old_pointm, Qnil, PT, PT_BYTE);
+  scroll_command (Fother_window_for_scrolling (), arg, 1);
   unbind_to (count, Qnil);
+  return Qnil;
+}
 
+DEFUN ("scroll-other-window-down", Fscroll_other_window_down,
+       Sscroll_other_window_down, 0, 1, "P",
+       doc: /* Scroll next window downward ARG lines; or near full screen if no ARG.
+For more details, see the documentation for `scroll-other-window'.  */)
+  (Lisp_Object arg)
+{
+  ptrdiff_t count = SPECPDL_INDEX ();
+  scroll_command (Fother_window_for_scrolling (), arg, -1);
+  unbind_to (count, Qnil);
   return Qnil;
 }
 \f
@@ -7831,6 +7835,7 @@ displayed after a scrolling operation to be somewhat inaccurate.  */);
   defsubr (&Sscroll_right);
   defsubr (&Sother_window_for_scrolling);
   defsubr (&Sscroll_other_window);
+  defsubr (&Sscroll_other_window_down);
   defsubr (&Sminibuffer_selected_window);
   defsubr (&Srecenter);
   defsubr (&Swindow_text_width);
-- 
2.17.0


[-- Attachment #5: Type: text/plain, Size: 1506 bytes --]


I'm sorry about the long delay in getting back to this (SSD with
unpushed changes was TRIMmed by Windows partition updates a few months
ago; I got distracted by other things...).  Attached are four patches.

The first avoids scrolling the phantom daemon's window by limiting the
next-window search in Fother_window_for_scrolling to visible frames on
the current terminal, as per the solution to bug#3442.  So far, everyone
seems to agree this is the reasonable thing to do here as well.

The second extends the Emacs and Elisp manuals to document
scroll-other-window-down and tries to make the docstrings of
Fother_window_for_scrolling and Fscroll_other_window clearer and more
accurate.

The third tries to simplify beginning-of-buffer-other-window and
end-of-buffer-other-window by using the macro with-selected-window.

The fourth tries to reduce code duplication in src/window.c by
generalising scroll_command to suit all three commands Fscroll_up,
Fscroll_down, and Fscroll_other_window.  Doing this made it clear that
scroll-other-window-down could also be written in terms of
scroll_command for simplicity, so this patch moves the former from
lisp/window.el to src/window.c.  The patch does not move the command's
corresponding binding from lisp/bindings.el to keys_of_window in
src/window.c because it doesn't look like any of the keys_of_* functions
can (or do) make use of the shift modifier.

Let me know how the patches can be improved,
should these proposals be accepted.

Thanks,

-- 
Basil

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Wed, 02 May 2018 19:57:59 +0100
> Cc: Noam Postavsky <npostavs@users.sourceforge.net>
> 
> +@kindex C-M-S-v
> +@findex scroll-other-window-down
>    The usual scrolling commands (@pxref{Display}) apply to the selected
> -window only, but there is one command to scroll the next window.
> +window only, but there are two commands to scroll the next window.
                    ^^^^^^^^^^^^^^^^^^^^^^
I'd just say "there are commands".

>  @kbd{C-M-v} (@code{scroll-other-window}) scrolls the window that
> -@kbd{C-x o} would select.  It takes arguments, positive and negative,
> -like @kbd{C-v}.  (In the minibuffer, @kbd{C-M-v} scrolls the help
> -window associated with the minibuffer, if any, rather than the next
> -window in the standard cyclic order; @pxref{Minibuffer Edit}.)
> +@kbd{C-x o} would select upward.
                     ^^^^^^^^^^^^^
This makes the sentence ambiguous: it is not clear whether "upward"
goes with "scroll" or with "select".  Either say "scrolls upward" at
the beginning of the sentence, or maybe explain the "upward" part in a
separate sentence (and what does "upward" mean here anyway -- does the
text or the viewport move upward?).

Thanks.

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Basil L. Contovounesios]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: "Basil L. Contovounesios" <contovob@tcd.ie>
>> Date: Wed, 02 May 2018 19:57:59 +0100
>> Cc: Noam Postavsky <npostavs@users.sourceforge.net>
>> 
>> +@kindex C-M-S-v
>> +@findex scroll-other-window-down
>>    The usual scrolling commands (@pxref{Display}) apply to the selected
>> -window only, but there is one command to scroll the next window.
>> +window only, but there are two commands to scroll the next window.
>                     ^^^^^^^^^^^^^^^^^^^^^^
> I'd just say "there are commands".

That's much better, but the sentence still seems a bit abrupt to me.
How about "there are also commands"?

>>  @kbd{C-M-v} (@code{scroll-other-window}) scrolls the window that
>> -@kbd{C-x o} would select.  It takes arguments, positive and negative,
>> -like @kbd{C-v}.  (In the minibuffer, @kbd{C-M-v} scrolls the help
>> -window associated with the minibuffer, if any, rather than the next
>> -window in the standard cyclic order; @pxref{Minibuffer Edit}.)
>> +@kbd{C-x o} would select upward.
>                      ^^^^^^^^^^^^^
> This makes the sentence ambiguous: it is not clear whether "upward"
> goes with "scroll" or with "select".  Either say "scrolls upward" at
> the beginning of the sentence, or maybe explain the "upward" part in a
> separate sentence (and what does "upward" mean here anyway -- does the
> text or the viewport move upward?).

Good point.  Given that (a) forward/upward and backward/downward
scrolling is described in more detail under the indirectly referenced
"(emacs) Scrolling", and (b) half of this paragraph is already
parenthetical, could we get away with just minor disambiguation?
For example:


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

diff --git a/doc/emacs/windows.texi b/doc/emacs/windows.texi
index 59b3b65f65..17ce4ad04d 100644
--- a/doc/emacs/windows.texi
+++ b/doc/emacs/windows.texi
@@ -183,13 +183,18 @@ Other Window
 
 @kindex C-M-v
 @findex scroll-other-window
+@kindex C-M-S-v
+@findex scroll-other-window-down
   The usual scrolling commands (@pxref{Display}) apply to the selected
-window only, but there is one command to scroll the next window.
+window only, but there are also commands to scroll the next window.
 @kbd{C-M-v} (@code{scroll-other-window}) scrolls the window that
-@kbd{C-x o} would select.  It takes arguments, positive and negative,
-like @kbd{C-v}.  (In the minibuffer, @kbd{C-M-v} scrolls the help
-window associated with the minibuffer, if any, rather than the next
-window in the standard cyclic order; @pxref{Minibuffer Edit}.)
+@kbd{C-x o} would select.  In other respects, the command behaves like
+@kbd{C-v}; both move the buffer text upward relative to the window, and
+take positive and negative arguments.  (In the minibuffer, @kbd{C-M-v}
+scrolls the help window associated with the minibuffer, if any, rather
+than the next window in the standard cyclic order; @pxref{Minibuffer
+Edit}.)  @kbd{C-M-S-v} (@code{scroll-other-window-down}) scrolls the
+next window downward in a similar way.
 
 @vindex mouse-autoselect-window
   If you set @code{mouse-autoselect-window} to a non-@code{nil} value,

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


[As you can see, I'm not particularly confident of my taste in technical
 prose.]

Thanks,

-- 
Basil

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Cc: <30207@debbugs.gnu.org>,  <rudalics@gmx.at>,  <npostavs@users.sourceforge.net>
> Date: Thu, 03 May 2018 14:03:52 +0100
> 
> Good point.  Given that (a) forward/upward and backward/downward
> scrolling is described in more detail under the indirectly referenced
> "(emacs) Scrolling", and (b) half of this paragraph is already
> parenthetical, could we get away with just minor disambiguation?
> For example:

Thanks, the updated text LGTM.

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Basil L. Contovounesios]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Limit "other window" scrolling to current terminal --]
[-- Type: text/x-diff, Size: 1598 bytes --]

From 12d53f5a7b24307b550716aa4f6875c0f8eebb2b Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Mon, 30 Apr 2018 18:02:15 +0100
Subject: [PATCH 1/4] Limit "other window" scrolling to current terminal

* src/window.c (Fother_window_for_scrolling): Limit next-window
search to visible frames on the current terminal. (bug#30207)
---
 src/window.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/window.c b/src/window.c
index e6d0280d9b..59c9422029 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5709,8 +5709,7 @@ specifies the window.  This takes precedence over
       && !NILP (Vminibuf_scroll_window))
     window = Vminibuf_scroll_window;
   /* If buffer is specified and live, scroll that buffer.  */
-  else if (!NILP (Vother_window_scroll_buffer)
-	   && BUFFERP (Vother_window_scroll_buffer)
+  else if (BUFFERP (Vother_window_scroll_buffer)
 	   && BUFFER_LIVE_P (XBUFFER (Vother_window_scroll_buffer)))
     {
       window = Fget_buffer_window (Vother_window_scroll_buffer, Qnil);
@@ -5725,11 +5724,8 @@ specifies the window.  This takes precedence over
 
       if (EQ (window, selected_window))
 	/* That didn't get us anywhere; look for a window on another
-           visible frame.  */
-	do
-	  window = Fnext_window (window, Qnil, Qt);
-	while (! FRAME_VISIBLE_P (XFRAME (WINDOW_FRAME (XWINDOW (window))))
-	       && ! EQ (window, selected_window));
+           visible frame on the current terminal.  */
+        window = Fnext_window (window, Qnil, Qvisible);
     }
 
   CHECK_LIVE_WINDOW (window);
-- 
2.17.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Improve documentation for "other window" scrolling --]
[-- Type: text/x-diff, Size: 6284 bytes --]

From a9b3a7cee8cec61d1b2256a57ad0e6a22f1f51b8 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Thu, 3 May 2018 13:52:20 +0100
Subject: [PATCH 2/4] Improve documentation for "other window" scrolling

* doc/emacs/windows.texi (Other Window):
* doc/lispref/windows.texi (Textual Scrolling):
Document scroll-other-window-down.
* doc/lispref/minibuf.texi (Minibuffer Misc):
Cross-reference minibuffer-scroll-window with Textual Scrolling.
* src/window.c (Fother_window_for_scrolling):
Clarify how "other window" is determined in docstring.
(Fscroll_other_window): Simplify docstring, pointing to that of
Fother_window_for_scrolling. (bug#30207)
---
 doc/emacs/windows.texi   | 19 +++++++++++++------
 doc/lispref/minibuf.texi |  2 +-
 doc/lispref/windows.texi |  7 +++++++
 src/window.c             | 25 ++++++++++++-------------
 4 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/doc/emacs/windows.texi b/doc/emacs/windows.texi
index 809363305f..17ce4ad04d 100644
--- a/doc/emacs/windows.texi
+++ b/doc/emacs/windows.texi
@@ -157,7 +157,9 @@ Other Window
 @item C-x o
 Select another window (@code{other-window}).
 @item C-M-v
-Scroll the next window (@code{scroll-other-window}).
+Scroll the next window upward (@code{scroll-other-window}).
+@item C-M-S-v
+Scroll the next window downward (@code{scroll-other-window-down}).
 @item mouse-1
 @kbd{mouse-1}, in the text area of a window, selects the window and
 moves point to the position clicked.  Clicking in the mode line
@@ -181,13 +183,18 @@ Other Window
 
 @kindex C-M-v
 @findex scroll-other-window
+@kindex C-M-S-v
+@findex scroll-other-window-down
   The usual scrolling commands (@pxref{Display}) apply to the selected
-window only, but there is one command to scroll the next window.
+window only, but there are also commands to scroll the next window.
 @kbd{C-M-v} (@code{scroll-other-window}) scrolls the window that
-@kbd{C-x o} would select.  It takes arguments, positive and negative,
-like @kbd{C-v}.  (In the minibuffer, @kbd{C-M-v} scrolls the help
-window associated with the minibuffer, if any, rather than the next
-window in the standard cyclic order; @pxref{Minibuffer Edit}.)
+@kbd{C-x o} would select.  In other respects, the command behaves like
+@kbd{C-v}; both move the buffer text upward relative to the window, and
+take positive and negative arguments.  (In the minibuffer, @kbd{C-M-v}
+scrolls the help window associated with the minibuffer, if any, rather
+than the next window in the standard cyclic order; @pxref{Minibuffer
+Edit}.)  @kbd{C-M-S-v} (@code{scroll-other-window-down}) scrolls the
+next window downward in a similar way.
 
 @vindex mouse-autoselect-window
   If you set @code{mouse-autoselect-window} to a non-@code{nil} value,
diff --git a/doc/lispref/minibuf.texi b/doc/lispref/minibuf.texi
index 338e03ef74..889b64af8a 100644
--- a/doc/lispref/minibuf.texi
+++ b/doc/lispref/minibuf.texi
@@ -2462,7 +2462,7 @@ Minibuffer Misc
 @anchor{Definition of minibuffer-scroll-window}
 If the value of this variable is non-@code{nil}, it should be a window
 object.  When the function @code{scroll-other-window} is called in the
-minibuffer, it scrolls this window.
+minibuffer, it scrolls this window (@pxref{Textual Scrolling}).
 @end defvar
 
 @defun minibuffer-selected-window
diff --git a/doc/lispref/windows.texi b/doc/lispref/windows.texi
index f5de2fc90b..315ffd4f48 100644
--- a/doc/lispref/windows.texi
+++ b/doc/lispref/windows.texi
@@ -4028,6 +4028,13 @@ Textual Scrolling
 @samp{End of buffer}.
 @end deffn
 
+@deffn Command scroll-other-window-down &optional count
+This function scrolls the text in another window downward @var{count}
+lines.  Negative values of @var{count}, or @code{nil}, are handled as
+in @code{scroll-down}.  In other respects, it behaves the same way as
+@code{scroll-other-window} does.
+@end deffn
+
 @defvar other-window-scroll-buffer
 If this variable is non-@code{nil}, it tells @code{scroll-other-window}
 which buffer's window to scroll.
diff --git a/src/window.c b/src/window.c
index 59c9422029..af317674bb 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5696,11 +5696,12 @@ When calling from a program, supply as argument a number, nil, or `-'.  */)
 \f
 DEFUN ("other-window-for-scrolling", Fother_window_for_scrolling, Sother_window_for_scrolling, 0, 0, 0,
        doc: /* Return the other window for \"other window scroll\" commands.
-If `other-window-scroll-buffer' is non-nil, a window
-showing that buffer is used.
 If in the minibuffer, `minibuffer-scroll-window' if non-nil
-specifies the window.  This takes precedence over
-`other-window-scroll-buffer'.  */)
+specifies the window.
+Otherwise, if `other-window-scroll-buffer' is non-nil, a window
+showing that buffer is used, popping the buffer up if necessary.
+Finally, look for a neighboring window on the selected frame,
+followed by all visible frames on the current terminal.  */)
   (void)
 {
   Lisp_Object window;
@@ -5739,16 +5740,14 @@ specifies the window.  This takes precedence over
 DEFUN ("scroll-other-window", Fscroll_other_window, Sscroll_other_window, 0, 1, "P",
        doc: /* Scroll next window upward ARG lines; or near full screen if no ARG.
 A near full screen is `next-screen-context-lines' less than a full screen.
-The next window is the one below the current one; or the one at the top
-if the current one is at the bottom.  Negative ARG means scroll downward.
-If ARG is the atom `-', scroll downward by nearly full screen.
-When calling from a program, supply as argument a number, nil, or `-'.
+Negative ARG means scroll downward.  If ARG is the atom `-', scroll
+downward by nearly full screen.  When calling from a program, supply
+as argument a number, nil, or `-'.
 
-If `other-window-scroll-buffer' is non-nil, scroll the window
-showing that buffer, popping the buffer up if necessary.
-If in the minibuffer, `minibuffer-scroll-window' if non-nil
-specifies the window to scroll.  This takes precedence over
-`other-window-scroll-buffer'.  */)
+The next window is usually the one below the current one;
+or the one at the top if the current one is at the bottom.
+It is determined by the function `other-window-for-scrolling',
+which see.  */)
   (Lisp_Object arg)
 {
   Lisp_Object window;
-- 
2.17.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Simplify "other window" bob/eob motion commands --]
[-- Type: text/x-diff, Size: 2227 bytes --]

From c9cd3a1e7b732f48906820c86fc35f2c517cb496 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 1 May 2018 16:05:52 +0100
Subject: [PATCH 3/4] Simplify "other window" bob/eob motion commands

* lisp/window.el (beginning-of-buffer-other-window)
(end-of-buffer-other-window):
Simplify via with-selected-window. (bug#30207)
---
 lisp/window.el | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/lisp/window.el b/lisp/window.el
index 8055e5babc..bc2008e1d9 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -8939,19 +8939,12 @@ beginning-of-buffer-other-window
 Leave mark at previous position.
 With arg N, put point N/10 of the way from the true beginning."
   (interactive "P")
-  (let ((orig-window (selected-window))
-	(window (other-window-for-scrolling)))
-    ;; We use unwind-protect rather than save-window-excursion
-    ;; because the latter would preserve the things we want to change.
-    (unwind-protect
-	(progn
-	  (select-window window)
-	  ;; Set point and mark in that window's buffer.
-	  (with-no-warnings
-	   (beginning-of-buffer arg))
-	  ;; Set point accordingly.
-	  (recenter '(t)))
-      (select-window orig-window))))
+  (with-selected-window (other-window-for-scrolling)
+    ;; Set point and mark in that window's buffer.
+    (with-no-warnings
+      (beginning-of-buffer arg))
+    ;; Set point accordingly.
+    (recenter '(t))))
 
 (defun end-of-buffer-other-window (arg)
   "Move point to the end of the buffer in the other window.
@@ -8959,15 +8952,10 @@ end-of-buffer-other-window
 With arg N, put point N/10 of the way from the true end."
   (interactive "P")
   ;; See beginning-of-buffer-other-window for comments.
-  (let ((orig-window (selected-window))
-	(window (other-window-for-scrolling)))
-    (unwind-protect
-	(progn
-	  (select-window window)
-	  (with-no-warnings
-	   (end-of-buffer arg))
-	  (recenter '(t)))
-      (select-window orig-window))))
+  (with-selected-window (other-window-for-scrolling)
+    (with-no-warnings
+      (end-of-buffer arg))
+    (recenter '(t))))
 \f
 (defvar mouse-autoselect-window-timer nil
   "Timer used by delayed window autoselection.")
-- 
2.17.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Rewrite scroll-other-window-down in C --]
[-- Type: text/x-diff, Size: 6553 bytes --]

From cc8278bb3ca56c729b2b72cb4eaf8c60702af0eb Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 1 May 2018 13:48:30 +0100
Subject: [PATCH 4/4] Rewrite scroll-other-window-down in C (bug#30207)

* lisp/window.el (scroll-other-window-down):
Move to src/window.c as Fscroll_other_window_down.
* src/window.c (scroll_command): Generalise for arbitrary windows.
(Fscroll_up, Fscroll_down): Use scroll_command with selected_window.
(Fscroll_other_window, Fscroll_other_window_down):
Rewrite in terms of scroll_command.
(syms_of_window): Add Sscroll_other_window_down.
---
 lisp/window.el | 11 -------
 src/window.c   | 85 ++++++++++++++++++++++++++------------------------
 2 files changed, 45 insertions(+), 51 deletions(-)

diff --git a/lisp/window.el b/lisp/window.el
index bc2008e1d9..085e51646f 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -8923,17 +8923,6 @@ scroll-down-line
 (put 'scroll-down-line 'scroll-command t)
 
 \f
-(defun scroll-other-window-down (&optional lines)
-  "Scroll the \"other window\" down.
-For more details, see the documentation for `scroll-other-window'."
-  (interactive "P")
-  (scroll-other-window
-   ;; Just invert the argument's meaning.
-   ;; We can do that without knowing which window it will be.
-   (if (eq lines '-) nil
-     (if (null lines) '-
-       (- (prefix-numeric-value lines))))))
-
 (defun beginning-of-buffer-other-window (arg)
   "Move point to the beginning of the buffer in the other window.
 Leave mark at previous position.
diff --git a/src/window.c b/src/window.c
index af317674bb..f654d87e14 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5634,35 +5634,54 @@ window_scroll_line_based (Lisp_Object window, int n, bool whole, bool noerror)
 }
 
 
-/* Scroll selected_window up or down.  If N is nil, scroll a
+/* Scroll WINDOW up or down.  If N is nil, scroll upward by a
    screen-full which is defined as the height of the window minus
-   next_screen_context_lines.  If N is the symbol `-', scroll.
-   DIRECTION may be 1 meaning to scroll down, or -1 meaning to scroll
-   up.  This is the guts of Fscroll_up and Fscroll_down.  */
+   next_screen_context_lines.  If N is the symbol `-', scroll downward
+   by a screen-full.  DIRECTION may be 1 meaning to scroll down, or -1
+   meaning to scroll up.  */
 
 static void
-scroll_command (Lisp_Object n, int direction)
+scroll_command (Lisp_Object window, Lisp_Object n, int direction)
 {
+  struct window *w;
+  bool other_window;
   ptrdiff_t count = SPECPDL_INDEX ();
 
   eassert (eabs (direction) == 1);
 
-  /* If selected window's buffer isn't current, make it current for
+  w = XWINDOW (window);
+  other_window = ! EQ (window, selected_window);
+
+  /* If given window's buffer isn't current, make it current for
      the moment.  But don't screw up if window_scroll gets an error.  */
-  if (XBUFFER (XWINDOW (selected_window)->contents) != current_buffer)
+  if (XBUFFER (w->contents) != current_buffer)
     {
       record_unwind_protect (save_excursion_restore, save_excursion_save ());
-      Fset_buffer (XWINDOW (selected_window)->contents);
+      Fset_buffer (w->contents);
+    }
+
+  if (other_window)
+    {
+      SET_PT_BOTH (marker_position (w->pointm),
+                   marker_byte_position (w->pointm));
+      SET_PT_BOTH (marker_position (w->old_pointm),
+                   marker_byte_position (w->old_pointm));
     }
 
   if (NILP (n))
-    window_scroll (selected_window, direction, true, false);
+    window_scroll (window, direction, true, false);
   else if (EQ (n, Qminus))
-    window_scroll (selected_window, -direction, true, false);
+    window_scroll (window, -direction, true, false);
   else
     {
       n = Fprefix_numeric_value (n);
-      window_scroll (selected_window, XINT (n) * direction, false, false);
+      window_scroll (window, XINT (n) * direction, false, false);
+    }
+
+  if (other_window)
+    {
+      set_marker_both (w->pointm, Qnil, PT, PT_BYTE);
+      set_marker_both (w->old_pointm, Qnil, PT, PT_BYTE);
     }
 
   unbind_to (count, Qnil);
@@ -5677,7 +5696,7 @@ If ARG is the atom `-', scroll downward by nearly full screen.
 When calling from a program, supply as argument a number, nil, or `-'.  */)
   (Lisp_Object arg)
 {
-  scroll_command (arg, 1);
+  scroll_command (selected_window, arg, 1);
   return Qnil;
 }
 
@@ -5690,7 +5709,7 @@ If ARG is the atom `-', scroll upward by nearly full screen.
 When calling from a program, supply as argument a number, nil, or `-'.  */)
   (Lisp_Object arg)
 {
-  scroll_command (arg, -1);
+  scroll_command (selected_window, arg, -1);
   return Qnil;
 }
 \f
@@ -5750,36 +5769,21 @@ It is determined by the function `other-window-for-scrolling',
 which see.  */)
   (Lisp_Object arg)
 {
-  Lisp_Object window;
-  struct window *w;
   ptrdiff_t count = SPECPDL_INDEX ();
-
-  window = Fother_window_for_scrolling ();
-  w = XWINDOW (window);
-
-  /* Don't screw up if window_scroll gets an error.  */
-  record_unwind_protect (save_excursion_restore, save_excursion_save ());
-
-  Fset_buffer (w->contents);
-  SET_PT_BOTH (marker_position (w->pointm), marker_byte_position (w->pointm));
-  SET_PT_BOTH (marker_position (w->old_pointm), marker_byte_position (w->old_pointm));
-
-  if (NILP (arg))
-    window_scroll (window, 1, true, true);
-  else if (EQ (arg, Qminus))
-    window_scroll (window, -1, true, true);
-  else
-    {
-      if (CONSP (arg))
-	arg = XCAR (arg);
-      CHECK_NUMBER (arg);
-      window_scroll (window, XINT (arg), false, true);
-    }
-
-  set_marker_both (w->pointm, Qnil, PT, PT_BYTE);
-  set_marker_both (w->old_pointm, Qnil, PT, PT_BYTE);
+  scroll_command (Fother_window_for_scrolling (), arg, 1);
   unbind_to (count, Qnil);
+  return Qnil;
+}
 
+DEFUN ("scroll-other-window-down", Fscroll_other_window_down,
+       Sscroll_other_window_down, 0, 1, "P",
+       doc: /* Scroll next window downward ARG lines; or near full screen if no ARG.
+For more details, see the documentation for `scroll-other-window'.  */)
+  (Lisp_Object arg)
+{
+  ptrdiff_t count = SPECPDL_INDEX ();
+  scroll_command (Fother_window_for_scrolling (), arg, -1);
+  unbind_to (count, Qnil);
   return Qnil;
 }
 \f
@@ -7831,6 +7835,7 @@ displayed after a scrolling operation to be somewhat inaccurate.  */);
   defsubr (&Sscroll_right);
   defsubr (&Sother_window_for_scrolling);
   defsubr (&Sscroll_other_window);
+  defsubr (&Sscroll_other_window_down);
   defsubr (&Sminibuffer_selected_window);
   defsubr (&Srecenter);
   defsubr (&Swindow_text_width);
-- 
2.17.0


[-- Attachment #5: Type: text/plain, Size: 220 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, the updated text LGTM.

Thanks, I reattach the updated patches.  I've incorporated the
documentation feedback in the second one; the other three are as before.

-- 
Basil

bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns window on daemon frame

[Noam Postavsky]

tags 30207 fixed
close 30207 27.1
quit

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Thanks, I reattach the updated patches.  I've incorporated the
> documentation feedback in the second one; the other three are as before.

Pushed to master.

[1: dc9188ada5]: 2018-05-10 19:04:11 -0400
  Limit "other window" scrolling to current terminal
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=dc9188ada522743dd9c9a6658570d9c4973be432

[2: d5cf1b3160]: 2018-05-10 19:04:11 -0400
  Improve documentation for "other window" scrolling
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=d5cf1b3160a5510198fc5dd4e28b3eca7aadf71b

[3: ae92f52c75]: 2018-05-10 19:04:11 -0400
  Simplify "other window" bob/eob motion commands
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=ae92f52c75383cf8f332db184d91f10fa8fb67cb

[4: eabb6f6c3e]: 2018-05-10 19:04:11 -0400
  Rewrite scroll-other-window-down in C (bug#30207)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=eabb6f6c3ee75dac1a7510e80bdd3c2fcfbbbcb5