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

["Basil L. Contovounesios" <contovob@tcd.ie>]

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