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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 30207 in the body.
You can then email your comments to 30207 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Mon, 22 Jan 2018 15:57:17 +0000

[0001-Do-not-scroll-other-window-in-daemon-frame.patch (text/x-diff, attachment)]

[Message part 2 (text/plain, inline)]

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:

[diff.diff (text/x-diff, inline)]

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

[Message part 4 (text/plain, inline)]

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

Message #8 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>, 30207 <at> debbugs.gnu.org
Subject: Re: bug#30207: 27.0.50; [PATCH] other-window-for-scrolling returns
 window on daemon frame
Date: Mon, 22 Jan 2018 19:59:47 +0100

> 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

Message #11 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 30207 <at> debbugs.gnu.org
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Tue, 23 Jan 2018 00:07:19 +0000

martin rudalics <rudalics <at> 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

Message #14 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: martin rudalics <rudalics <at> gmx.at>, 30207 <at> debbugs.gnu.org
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Mon, 22 Jan 2018 19:29:12 -0500

"Basil L. Contovounesios" <contovob <at> 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).

Message #17 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: martin rudalics <rudalics <at> gmx.at>, 30207 <at> debbugs.gnu.org
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Tue, 23 Jan 2018 01:06:21 +0000

Noam Postavsky <npostavs <at> 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

Message #20 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: martin rudalics <rudalics <at> gmx.at>, 30207 <at> debbugs.gnu.org
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Mon, 22 Jan 2018 20:29:08 -0500

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

> Noam Postavsky <npostavs <at> 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.

Message #23 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: <30207 <at> debbugs.gnu.org>
Cc: martin rudalics <rudalics <at> gmx.at>,
 Noam Postavsky <npostavs <at> users.sourceforge.net>
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Tue, 23 Jan 2018 13:28:41 +0000

"Basil L. Contovounesios" <contovob <at> 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

Message #26 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: <30207 <at> debbugs.gnu.org>
Cc: martin rudalics <rudalics <at> gmx.at>,
 Noam Postavsky <npostavs <at> users.sourceforge.net>
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Wed, 02 May 2018 19:57:59 +0100

[0001-Limit-other-window-scrolling-to-current-terminal.patch (text/x-diff, attachment)]

[0002-Improve-documentation-for-other-window-scrolling.patch (text/x-diff, attachment)]

[0003-Simplify-other-window-bob-eob-motion-commands.patch (text/x-diff, attachment)]

[0004-Rewrite-scroll-other-window-down-in-C-bug-30207.patch (text/x-diff, attachment)]

[Message part 5 (text/plain, inline)]

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

Message #29 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 30207 <at> debbugs.gnu.org, npostavs <at> users.sourceforge.net, rudalics <at> gmx.at
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Wed, 02 May 2018 22:44:03 +0300

> From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
> Date: Wed, 02 May 2018 19:57:59 +0100
> Cc: Noam Postavsky <npostavs <at> 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.

Message #32 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 30207 <at> debbugs.gnu.org, npostavs <at> users.sourceforge.net, rudalics <at> gmx.at
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Thu, 03 May 2018 14:03:52 +0100

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
>> Date: Wed, 02 May 2018 19:57:59 +0100
>> Cc: Noam Postavsky <npostavs <at> 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:

[other-window.patch (text/x-diff, inline)]

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,

[Message part 3 (text/plain, inline)]

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

Thanks,

-- 
Basil

Message #35 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: 30207 <at> debbugs.gnu.org, npostavs <at> users.sourceforge.net, rudalics <at> gmx.at
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Thu, 03 May 2018 22:00:27 +0300

> From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
> Cc: <30207 <at> debbugs.gnu.org>,  <rudalics <at> gmx.at>,  <npostavs <at> 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.

Message #38 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 30207 <at> debbugs.gnu.org, npostavs <at> users.sourceforge.net, rudalics <at> gmx.at
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Thu, 03 May 2018 23:44:00 +0100

[0001-Limit-other-window-scrolling-to-current-terminal.patch (text/x-diff, attachment)]

[0002-Improve-documentation-for-other-window-scrolling.patch (text/x-diff, attachment)]

[0003-Simplify-other-window-bob-eob-motion-commands.patch (text/x-diff, attachment)]

[0004-Rewrite-scroll-other-window-down-in-C-bug-30207.patch (text/x-diff, attachment)]

[Message part 5 (text/plain, inline)]

Eli Zaretskii <eliz <at> 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

Message #41 received at 30207 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 30207 <at> debbugs.gnu.org, rudalics <at> gmx.at
Subject: Re: bug#30207: 27.0.50;
 [PATCH] other-window-for-scrolling returns window on daemon frame
Date: Thu, 10 May 2018 19:48:32 -0400

tags 30207 fixed
close 30207 27.1
quit

"Basil L. Contovounesios" <contovob <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.