GNU bug report logs - #35464
[PATCH] Refactor update_window_begin and update_window_end hooks

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 35464 in the body.
You can then email your comments to 35464 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: Alex Gramiak <agrambot <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Refactor update_window_begin and update_window_end hooks
Date: Sat, 27 Apr 2019 15:28:49 -0600

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

I have two questions about this:

1) Is it okay to not use reset_mouse_highlight in the generic version
(ns_update_window_end still uses it)?  See commit 60ae3d09932f for why
reset_mouse_highlight was removed in the x/w32 versions.

2) Should the #if 0 section be removed in w32_update_window_begin?

[0001-Refactor-update_window_begin-and-update_window_end-h.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35464 <at> debbugs.gnu.org
Subject: Re: bug#35464: [PATCH] Refactor update_window_begin and
 update_window_end hooks
Date: Sun, 28 Apr 2019 19:45:22 +0300

> From: Alex Gramiak <agrambot <at> gmail.com>
> Date: Sat, 27 Apr 2019 15:28:49 -0600
> 
> 1) Is it okay to not use reset_mouse_highlight in the generic version
> (ns_update_window_end still uses it)?  See commit 60ae3d09932f for why
> reset_mouse_highlight was removed in the x/w32 versions.

Don't know, the problem was specific to X.  I think we should leave
the code there, the NS port has enough redisplay problems already.

> 2) Should the #if 0 section be removed in w32_update_window_begin?

Yes, I think we can remove that.

> @@ -13145,8 +13069,8 @@ static struct redisplay_interface x_redisplay_interface =
>      gui_clear_end_of_line,
>      x_scroll_run,
>      x_after_update_window_line,
> -    x_update_window_begin,
> -    x_update_window_end,
> +    gui_update_window_begin,
> +    gui_update_window_end,
>      x_flip_and_flush,
>      gui_clear_window_mouse_face,
>      gui_get_glyph_overhangs,

This looks like a step in the wrong direction to me: the different
implementations are all almost completely identical, except that w32
has a small quirk there.  So I'd say make a single function
window_update_begin, that will be called directly (not via a hook
pointer), and make the w32 part be an optional hook called only if
non-NULL.

Also, please don't add gui_* functions extracted from the *term.c
files in xdisp.c, as that file is already too large.  Renaming
existing functions in xdisp.c is OK, as well as adding static utility
functions.  But for new gui_* functions that were originally in
xterm.c etc., I'd prefer a new file, let's call it gui_term.c.

Thanks.

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

From: Alex Gramiak <agrambot <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35464 <at> debbugs.gnu.org
Subject: Re: bug#35464: [PATCH] Refactor update_window_begin and
 update_window_end hooks
Date: Sun, 28 Apr 2019 12:37:28 -0600

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

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

> Don't know, the problem was specific to X.  I think we should leave
> the code there, the NS port has enough redisplay problems already.

It seems that it's needed (see commit 96e78d1fb) for both X and W32, so
I wouldn't be surprised if NS needs the change as well, or, at least, if
it doesn't matter for that port.

>> @@ -13145,8 +13069,8 @@ static struct redisplay_interface x_redisplay_interface =
>>      gui_clear_end_of_line,
>>      x_scroll_run,
>>      x_after_update_window_line,
>> -    x_update_window_begin,
>> -    x_update_window_end,
>> +    gui_update_window_begin,
>> +    gui_update_window_end,
>>      x_flip_and_flush,
>>      gui_clear_window_mouse_face,
>>      gui_get_glyph_overhangs,
>
> This looks like a step in the wrong direction to me: the different
> implementations are all almost completely identical, except that w32
> has a small quirk there.  So I'd say make a single function
> window_update_begin, that will be called directly (not via a hook
> pointer), and make the w32 part be an optional hook called only if
> non-NULL.

Done.

> Also, please don't add gui_* functions extracted from the *term.c
> files in xdisp.c, as that file is already too large.  Renaming
> existing functions in xdisp.c is OK, as well as adding static utility
> functions.  But for new gui_* functions that were originally in
> xterm.c etc., I'd prefer a new file, let's call it gui_term.c.

A gui_term.c might be beneficial in the future, but I'm not sure it
should be the place for RIF procedures like these. I attached an updated
patch that currently puts them in terminal.c (beside update_begin and
update_end), but I think a better place would be to put them beside
update_window in dispnew.c. WDYT?

[0001-Refactor-update_window_begin-and-update_window_end-h.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35464 <at> debbugs.gnu.org
Subject: Re: bug#35464: [PATCH] Refactor update_window_begin and
 update_window_end hooks
Date: Sun, 28 Apr 2019 21:45:37 +0300

> From: Alex Gramiak <agrambot <at> gmail.com>
> Cc: 35464 <at> debbugs.gnu.org
> Date: Sun, 28 Apr 2019 12:37:28 -0600
> 
> > Also, please don't add gui_* functions extracted from the *term.c
> > files in xdisp.c, as that file is already too large.  Renaming
> > existing functions in xdisp.c is OK, as well as adding static utility
> > functions.  But for new gui_* functions that were originally in
> > xterm.c etc., I'd prefer a new file, let's call it gui_term.c.
> 
> A gui_term.c might be beneficial in the future, but I'm not sure it
> should be the place for RIF procedures like these.

Bu these functions are not RIF procedures, they are just regular
functions.

> I attached an updated patch that currently puts them in terminal.c
> (beside update_begin and update_end), but I think a better place
> would be to put them beside update_window in dispnew.c. WDYT?

It isn't worth arguing about 2 short functions, so OK.  But those in
your other patch should definitely start a new file.

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

From: Alex Gramiak <agrambot <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 35464 <at> debbugs.gnu.org
Subject: Re: bug#35464: [PATCH] Refactor update_window_begin and
 update_window_end hooks
Date: Thu, 02 May 2019 23:10:41 -0600

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

close 35464
quit

I pushed the change as 9ae94ebdf.

I have another change that I don't think is worth a new bug report,
which is refactoring scroll_run_hook in a similar way. Is it okay to
apply it?

[0001-Refactor-scroll_run_hook.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Alex Gramiak <agrambot <at> gmail.com>
Cc: 35464 <at> debbugs.gnu.org
Subject: Re: bug#35464: [PATCH] Refactor update_window_begin and
 update_window_end hooks
Date: Sat, 04 May 2019 11:45:30 +0300

> From: Alex Gramiak <agrambot <at> gmail.com>
> Cc: 35464 <at> debbugs.gnu.org
> Date: Thu, 02 May 2019 23:10:41 -0600
> 
> I have another change that I don't think is worth a new bug report,
> which is refactoring scroll_run_hook in a similar way. Is it okay to
> apply it?

I'm not sure this is a change for the best.  scroll_run is already a
frame-specific method; just because its 3 implementations are similar
in some of their parts doesn't yet mean _all_ possible implementations
will be that way.  Suppose the NS implementation needs to change for
some reason, or the TTY case needs to use this -- what do we do then?
move the code back to *term.[cm]?

The change also splits the code's logic into 2 parts in 2 different
places, another minus in my book.

This also adds an ifdef where currently there is none, which is an
additional small disadvantage.

Bottom line: I think this does not have enough significant advantages
to change the current code.

Thanks.

Previous Next

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