bug#31988: scroll-other-window broken on master

bug#31988: scroll-other-window broken on master

[Daniel Colascione]

scroll-other-window scrolls the current window _and_ the other window. To
repro, visit a big file, C-x 2, and mash C-M-v. Only the other window
should scroll. Now, both windows scroll.

bug#31988: scroll-other-window broken on master

[Daniel Colascione]

Basil, it looks like you recently rewrote a big chunk of the window
scrolling code. Can you please take a look?

Thanks.

> scroll-other-window scrolls the current window _and_ the other window. To
> repro, visit a big file, C-x 2, and mash C-M-v. Only the other window
> should scroll. Now, both windows scroll.

bug#31988: scroll-other-window broken on master

[Basil L. Contovounesios]

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

tags 31988 patch
quit

"Daniel Colascione" <dancol@dancol.org> writes:

> Basil, it looks like you recently rewrote a big chunk of the window
> scrolling code. Can you please take a look?
>
> Thanks.
>
>> scroll-other-window scrolls the current window _and_ the other window. To
>> repro, visit a big file, C-x 2, and mash C-M-v. Only the other window
>> should scroll. Now, both windows scroll.

This bug was indeed introduced by my recent refactor.
Does the following patch fix it?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-other-window-scroll-when-showing-same-buffer.patch --]
[-- Type: text/x-diff, Size: 986 bytes --]

From 026b6befc40795d8597e036f930f634b2a17e4f3 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Thu, 28 Jun 2018 00:13:07 +0300
Subject: [PATCH] Fix other window scroll when showing same buffer

src/window.c (scroll_command): Make other window's buffer current
before scrolling, even when displaying the same buffer. (bug#31988)
---
 src/window.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/window.c b/src/window.c
index 81fd7f2b47..42b9522862 100644
--- a/src/window.c
+++ b/src/window.c
@@ -5654,7 +5654,7 @@ scroll_command (Lisp_Object window, Lisp_Object n, int direction)
 
   /* 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 (w->contents) != current_buffer)
+  if (other_window || XBUFFER (w->contents) != current_buffer)
     {
       record_unwind_protect_excursion ();
       Fset_buffer (w->contents);
-- 
2.18.0


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


Thanks,

-- 
Basil

bug#31988: scroll-other-window broken on master

[Daniel Colascione]

> "Daniel Colascione" <dancol@dancol.org> writes:
>
>> Basil, it looks like you recently rewrote a big chunk of the window
>> scrolling code. Can you please take a look?
>>
>> Thanks.
>>
>>> scroll-other-window scrolls the current window _and_ the other window.
>>> To
>>> repro, visit a big file, C-x 2, and mash C-M-v. Only the other window
>>> should scroll. Now, both windows scroll.
>
> This bug was indeed introduced by my recent refactor.
> Does the following patch fix it?

That works. Thanks! Do you want to apply the patch?

bug#31988: scroll-other-window broken on master

[Basil L. Contovounesios]

"Daniel Colascione" <dancol@dancol.org> writes:

>> "Daniel Colascione" <dancol@dancol.org> writes:
>>
>>> Basil, it looks like you recently rewrote a big chunk of the window
>>> scrolling code. Can you please take a look?
>>>
>>> Thanks.
>>>
>>>> scroll-other-window scrolls the current window _and_ the other window.
>>>> To
>>>> repro, visit a big file, C-x 2, and mash C-M-v. Only the other window
>>>> should scroll. Now, both windows scroll.
>>
>> This bug was indeed introduced by my recent refactor.
>> Does the following patch fix it?
>
> That works. Thanks! Do you want to apply the patch?

I don't have push access, so someone else will have to apply the patch
for me if there are no objections to it.

Thanks,

-- 
Basil

bug#31988: scroll-other-window broken on master

[Daniel Colascione]

close 31988
quit

> "Daniel Colascione" <dancol@dancol.org> writes:
>
>>> "Daniel Colascione" <dancol@dancol.org> writes:
>>>
>>>> Basil, it looks like you recently rewrote a big chunk of the window
>>>> scrolling code. Can you please take a look?
>>>>
>>>> Thanks.
>>>>
>>>>> scroll-other-window scrolls the current window _and_ the other
>>>>> window.
>>>>> To
>>>>> repro, visit a big file, C-x 2, and mash C-M-v. Only the other window
>>>>> should scroll. Now, both windows scroll.
>>>
>>> This bug was indeed introduced by my recent refactor.
>>> Does the following patch fix it?
>>
>> That works. Thanks! Do you want to apply the patch?
>
> I don't have push access, so someone else will have to apply the patch
> for me if there are no objections to it.

I just applied it. Thanks!

bug#31988: scroll-other-window broken on master

[Eli Zaretskii]

> Date: Fri, 6 Jul 2018 16:31:24 -0700
> From: "Daniel Colascione" <dancol@dancol.org>
> Cc: 31988@debbugs.gnu.org
> 
> > I don't have push access, so someone else will have to apply the patch
> > for me if there are no objections to it.
> 
> I just applied it. Thanks!

Thanks, but why didn't you use the commit log Basil provided?  (It was
somewhat inaccurate, but more informative than what you used, IMO.)

bug#31988: scroll-other-window broken on master

[Basil L. Contovounesios]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 6 Jul 2018 16:31:24 -0700
>> From: "Daniel Colascione" <dancol@dancol.org>
>> Cc: 31988@debbugs.gnu.org
>> 
>> > I don't have push access, so someone else will have to apply the patch
>> > for me if there are no objections to it.
>> 
>> I just applied it. Thanks!
>
> Thanks, but why didn't you use the commit log Basil provided?  (It was
> somewhat inaccurate, but more informative than what you used, IMO.)

Thanks Daniel for applying the patch and Eli for your subsequent
efficiency and documentation fix.

I don't yet feel particularly good at or confident with writing
changelog entries, let alone Emacs C code, so any guidance is more than
welcome now and in the future.

I'm not sure whether Daniel's addition of the 'close' tag had the
intended effect, so I'm taking the liberty of CCing
<31988-done@debbugs.gnu.org>; if this was the wrong thing to do, please
let me know what I should have done instead.

Thanks again,

-- 
Basil

bug#31988: scroll-other-window broken on master

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Cc: Daniel Colascione <dancol@dancol.org>, <31988@debbugs.gnu.org>, <31988-done@debbugs.gnu.org>
> Date: Sat, 07 Jul 2018 16:40:17 +0300
> 
> I don't yet feel particularly good at or confident with writing
> changelog entries, let alone Emacs C code, so any guidance is more than
> welcome now and in the future.

I think you are doing just fine, thank you.  There's always some
learning curve, of course.  CONTRIBUTE should tell you enough, and you
can study the log messages by veterans to pick up the spirit.

The "inaccurate" part in my message related not to your wording (which
was fine, AFAICT), but to the fact that the important part of your fix
was to call save-excursion in the described case, not the part that
set the buffer (which was actually a no-op).

> I'm not sure whether Daniel's addition of the 'close' tag had the
> intended effect

It should have, but it looks like it didn't for some reason.

> so I'm taking the liberty of CCing <31988-done@debbugs.gnu.org>; if
> this was the wrong thing to do, please let me know what I should
> have done instead.

Doing that for a bug that should be closed can never do any harm, even
if it was already closed.  (You also don't need to CC both NNN@ and
NNN-done@, as the message gets recorded by the tracker even if you
only use the latter.)

Thanks.

bug#31988: scroll-other-window broken on master

[Basil L. Contovounesios]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: "Basil L. Contovounesios" <contovob@tcd.ie>
>> Cc: Daniel Colascione <dancol@dancol.org>, <31988@debbugs.gnu.org>,
>> <31988-done@debbugs.gnu.org>
>> Date: Sat, 07 Jul 2018 16:40:17 +0300
>> 
>> I don't yet feel particularly good at or confident with writing
>> changelog entries, let alone Emacs C code, so any guidance is more than
>> welcome now and in the future.
>
> I think you are doing just fine, thank you.  There's always some
> learning curve, of course.  CONTRIBUTE should tell you enough, and you
> can study the log messages by veterans to pick up the spirit.
>
> The "inaccurate" part in my message related not to your wording (which
> was fine, AFAICT), but to the fact that the important part of your fix
> was to call save-excursion in the described case, not the part that
> set the buffer (which was actually a no-op).
>
>> I'm not sure whether Daniel's addition of the 'close' tag had the
>> intended effect
>
> It should have, but it looks like it didn't for some reason.
>
>> so I'm taking the liberty of CCing <31988-done@debbugs.gnu.org>; if
>> this was the wrong thing to do, please let me know what I should
>> have done instead.
>
> Doing that for a bug that should be closed can never do any harm, even
> if it was already closed.  (You also don't need to CC both NNN@ and
> NNN-done@, as the message gets recorded by the tracker even if you
> only use the latter.)

Thanks for explaining everything, it all makes sense.

-- 
Basil