Subject: Re: bug#35464: [PATCH] Refactor update_window_begin and update_window_end hooks

From: Alex Gramiak <[email-masked]>
Date: Sun, 28 Apr 2019 12:37:28 -0600

Eli Zaretskii <[email-masked]> 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?

>From 6b6bae2370a378e08e50acad99007f10b541fe93 Mon Sep 17 00:00:00 2001
From: Alexander Gramiak <[email-masked]>
Date: Sat, 27 Apr 2019 15:00:13 -0600
Subject: [PATCH] Refactor update_window_begin and update_window_end hooks

* src/terminal.c (gui_update_window_begin, gui_update_window_end): New
procedures implementing common functionality.

* src/nsterm.m: (ns_update_window_begin, ns_update_window_end):
* src/xterm.c: (x_update_window_begin, x_update_window_end): Remove in
favor of only using the new generic versions.

* src/w32term.c: (w32_update_window_begin, w32_update_window_end):
Remove duplicated code.
---
 src/dispextern.h |  4 +++
 src/dispnew.c    |  4 +--
 src/nsterm.m     | 78 +++----------------------------------------
 src/terminal.c   | 86 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/w32term.c    | 83 ++--------------------------------------------
 src/xdisp.c      | 12 +++----
 src/xterm.c      | 84 +++-------------------------------------------
 7 files changed, 109 insertions(+), 242 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 4d6d0371d3..dede7830ec 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3533,6 +3533,10 @@ extern void fill_up_frame_row_with_spaces (struct glyph_row *, int);
 extern void ring_bell (struct frame *);
 extern void update_begin (struct frame *);
 extern void update_end (struct frame *);
+#ifdef HAVE_WINDOW_SYSTEM
+extern void gui_update_window_begin (struct window *);
+extern void gui_update_window_end (struct window *, bool, bool);
+#endif
 extern void set_terminal_window (struct frame *, int);
 extern void cursor_to (struct frame *, int, int);
 extern void raw_cursor_to (struct frame *, int, int);
diff --git a/src/dispnew.c b/src/dispnew.c
index 25a2d1cd38..7577760483 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -3411,7 +3411,7 @@ update_window (struct window *w, bool force_p)
       bool changed_p = 0, mouse_face_overwritten_p = 0;
       int n_updated = 0;
 
-      rif->update_window_begin_hook (w);
+      gui_update_window_begin (w);
       yb = window_text_bottom_y (w);
       row = MATRIX_ROW (desired_matrix, 0);
       end = MATRIX_MODE_LINE_ROW (desired_matrix);
@@ -3539,7 +3539,7 @@ update_window (struct window *w, bool force_p)
          paused updating the display because in this case,
          set_window_cursor_after_update hasn't been called, and
          W->output_cursor doesn't contain the cursor location.  */
-      rif->update_window_end_hook (w, !paused_p, mouse_face_overwritten_p);
+      gui_update_window_end (w, !paused_p, mouse_face_overwritten_p);
     }
   else
     paused_p = 1;
diff --git a/src/nsterm.m b/src/nsterm.m
index cdf1916e71..ffb7b7692b 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -1106,7 +1106,7 @@ static NSRect constrain_frame_rect(NSRect frameRect, bool isFullscreen)
 ns_update_begin (struct frame *f)
 /* --------------------------------------------------------------------------
    Prepare for a grouped sequence of drawing calls
-   external (RIF) call; whole frame, called before update_window_begin
+   external (RIF) call; whole frame, called before gui_update_window_begin
    -------------------------------------------------------------------------- */
 {
 #ifdef NS_IMPL_COCOA
@@ -1128,81 +1128,11 @@ static NSRect constrain_frame_rect(NSRect frameRect, bool isFullscreen)
 }

-static void
-ns_update_window_begin (struct window *w)
-/* --------------------------------------------------------------------------
-   Prepare for a grouped sequence of drawing calls
-   external (RIF) call; for one window, called after update_begin
-   -------------------------------------------------------------------------- */
-{
-  struct frame *f = XFRAME (WINDOW_FRAME (w));
-  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
-
-  NSTRACE_WHEN (NSTRACE_GROUP_UPDATES, "ns_update_window_begin");
-  w->output_cursor = w->cursor;
-
-  block_input ();
-
-  if (f == hlinfo->mouse_face_mouse_frame)
-    {
-      /* Don't do highlighting for mouse motion during the update.  */
-      hlinfo->mouse_face_defer = 1;
-
-        /* If the frame needs to be redrawn,
-           simply forget about any prior mouse highlighting.  */
-      if (FRAME_GARBAGED_P (f))
-        hlinfo->mouse_face_window = Qnil;
-
-      /* (further code for mouse faces ifdef'd out in other terms elided) */
-    }
-
-  unblock_input ();
-}
-
-
-static void
-ns_update_window_end (struct window *w, bool cursor_on_p,
-                      bool mouse_face_overwritten_p)
-/* --------------------------------------------------------------------------
-   Finished a grouped sequence of drawing calls
-   external (RIF) call; for one window called before update_end
-   -------------------------------------------------------------------------- */
-{
-  NSTRACE_WHEN (NSTRACE_GROUP_UPDATES, "ns_update_window_end");
-
-  /* note: this fn is nearly identical in all terms */
-  if (!w->pseudo_window_p)
-    {
-      block_input ();
-
-      if (cursor_on_p)
-	display_and_set_cursor (w, 1,
-				w->output_cursor.hpos, w->output_cursor.vpos,
-				w->output_cursor.x, w->output_cursor.y);
-
-      if (draw_window_fringes (w, 1))
-	{
-	  if (WINDOW_RIGHT_DIVIDER_WIDTH (w))
-	    gui_draw_right_divider (w);
-	  else
-	    gui_draw_vertical_border (w);
-	}
-
-      unblock_input ();
-    }
-
-  /* If a row with mouse-face was overwritten, arrange for
-     frame_up_to_date to redisplay the mouse highlight.  */
-  if (mouse_face_overwritten_p)
-    reset_mouse_highlight (MOUSE_HL_INFO (XFRAME (w->frame)));
-}
-
-
 static void
 ns_update_end (struct frame *f)
 /* --------------------------------------------------------------------------
    Finished a grouped sequence of drawing calls
-   external (RIF) call; for whole frame, called after update_window_end
+   external (RIF) call; for whole frame, called after gui_update_window_end
    -------------------------------------------------------------------------- */
 {
   NSTRACE_WHEN (NSTRACE_GROUP_UPDATES, "ns_update_end");
@@ -5166,8 +5096,8 @@ static Lisp_Object ns_string_to_lispmod (const char *s)
   gui_clear_end_of_line,
   ns_scroll_run,
   ns_after_update_window_line,
-  ns_update_window_begin,
-  ns_update_window_end,
+  NULL, /* update_window_begin */
+  NULL, /* update_window_end   */
   0, /* flush_display */
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
diff --git a/src/terminal.c b/src/terminal.c
index 0ee0121e35..5486905ff2 100644
--- a/src/terminal.c
+++ b/src/terminal.c
@@ -26,6 +26,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include "termchar.h"
 #include "termhooks.h"
 #include "keyboard.h"
+#include "blockinput.h"
 
 #if HAVE_STRUCT_UNIPAIR_UNICODE
 # include <errno.h>
@@ -93,6 +94,91 @@ update_end (struct frame *f)
     (*FRAME_TERMINAL (f)->update_end_hook) (f);
 }
 
+#ifdef HAVE_WINDOW_SYSTEM
+
+/* Start update of window W.  */
+
+void
+gui_update_window_begin (struct window *w)
+{
+  struct frame *f = XFRAME (WINDOW_FRAME (w));
+  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+
+  block_input ();
+
+  if (FRAME_RIF (f)->update_window_begin_hook)
+    FRAME_RIF (f)->update_window_begin_hook (w);
+
+  w->output_cursor = w->cursor;
+
+  if (f == hlinfo->mouse_face_mouse_frame)
+    {
+      /* Don't do highlighting for mouse motion during the update.  */
+      hlinfo->mouse_face_defer = true;
+
+      /* If the frame needs to be redrawn, simply forget about any
+	 prior mouse highlighting.  */
+      if (FRAME_GARBAGED_P (f))
+	hlinfo->mouse_face_window = Qnil;
+    }
+
+  unblock_input ();
+}
+
+/* End update of window W.
+
+   Draw vertical borders between horizontally adjacent windows, and
+   display W's cursor if CURSOR_ON_P is non-zero.
+
+   MOUSE_FACE_OVERWRITTEN_P non-zero means that some row containing
+   glyphs in mouse-face were overwritten.  In that case we have to
+   make sure that the mouse-highlight is properly redrawn.  */
+void
+gui_update_window_end (struct window *w, bool cursor_on_p,
+                       bool mouse_face_overwritten_p)
+{
+  struct frame *f = XFRAME (WINDOW_FRAME (w));
+
+  block_input ();
+
+  /* Pseudo windows don't have cursors, so don't display them here.  */
+  if (!w->pseudo_window_p)
+    {
+
+      if (cursor_on_p)
+	display_and_set_cursor (w, true,
+				w->output_cursor.hpos, w->output_cursor.vpos,
+				w->output_cursor.x, w->output_cursor.y);
+
+      if (draw_window_fringes (w, true))
+	{
+	  if (WINDOW_RIGHT_DIVIDER_WIDTH (w))
+	    gui_draw_right_divider (w);
+	  else
+	    gui_draw_vertical_border (w);
+	}
+    }
+
+  /* If a row with mouse-face was overwritten, arrange for
+     frame_up_to_date_hook to redisplay the mouse highlight.  */
+  if (mouse_face_overwritten_p)
+    {
+      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
+
+      hlinfo->mouse_face_beg_row = hlinfo->mouse_face_beg_col = -1;
+      hlinfo->mouse_face_end_row = hlinfo->mouse_face_end_col = -1;
+      hlinfo->mouse_face_window = Qnil;
+    }
+
+  if (FRAME_RIF (f)->update_window_end_hook)
+    FRAME_RIF (f)->update_window_end_hook (w,
+                                           cursor_on_p,
+                                           mouse_face_overwritten_p);
+  unblock_input ();
+}
+
+#endif /* HAVE_WINDOW_SYSTEM  */
+
 /* Specify how many text lines, from the top of the window,
    should be affected by insert-lines and delete-lines operations.
    This, and those operations, are used only within an update
diff --git a/src/w32term.c b/src/w32term.c
index 451dd54dd8..0abec3d92a 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -529,7 +529,7 @@ w32_display_pixel_width (struct w32_display_info *dpyinfo)
 
 /* Start an update of frame F.  This function is installed as a hook
    for update_begin, i.e. it is called when update_begin is called.
-   This function is called prior to calls to w32_update_window_begin
+   This function is called prior to calls to gui_update_window_begin
    for each window being updated.  */
 
 static void
@@ -555,58 +555,12 @@ w32_update_begin (struct frame *f)
 static void
 w32_update_window_begin (struct window *w)
 {
-  struct frame *f = XFRAME (WINDOW_FRAME (w));
-  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
-
   /* Hide the system caret during an update.  */
   if (w32_use_visible_system_caret && w32_system_caret_hwnd)
     {
       SendMessageTimeout (w32_system_caret_hwnd, WM_EMACS_HIDE_CARET, 0, 0,
 			  0, 6000, NULL);
     }
-
-  w->output_cursor = w->cursor;
-
-  block_input ();
-
-  if (f == hlinfo->mouse_face_mouse_frame)
-    {
-      /* Don't do highlighting for mouse motion during the update.  */
-      hlinfo->mouse_face_defer = true;
-
-      /* If F needs to be redrawn, simply forget about any prior mouse
-	 highlighting.  */
-      if (FRAME_GARBAGED_P (f))
-	hlinfo->mouse_face_window = Qnil;
-
-#if 0 /* Rows in a current matrix containing glyphs in mouse-face have
-	 their mouse_face_p flag set, which means that they are always
-	 unequal to rows in a desired matrix which never have that
-	 flag set.  So, rows containing mouse-face glyphs are never
-	 scrolled, and we don't have to switch the mouse highlight off
-	 here to prevent it from being scrolled.  */
-
-      /* Can we tell that this update does not affect the window
-	 where the mouse highlight is?  If so, no need to turn off.
-	 Likewise, don't do anything if the frame is garbaged;
-	 in that case, the frame's current matrix that we would use
-	 is all wrong, and we will redisplay that line anyway.  */
-      if (!NILP (hlinfo->mouse_face_window)
-	  && w == XWINDOW (hlinfo->mouse_face_window))
-	{
-	  int i;
-
-          for (i = 0; i < w->desired_matrix->nrows; ++i)
-	    if (MATRIX_ROW_ENABLED_P (w->desired_matrix, i))
-	      break;
-
-	  if (i < w->desired_matrix->nrows)
-	    clear_mouse_face (hlinfo);
-	}
-#endif /* 0 */
-    }
-
-  unblock_input ();
 }
 
 /* Draw a vertical window border from (x,y0) to (x,y1)  */
@@ -694,39 +648,8 @@ w32_draw_window_divider (struct window *w, int x0, int x1, int y0, int y1)
 
 static void
 w32_update_window_end (struct window *w, bool cursor_on_p,
-		     bool mouse_face_overwritten_p)
+                       bool mouse_face_overwritten_p)
 {
-  if (!w->pseudo_window_p)
-    {
-      block_input ();
-
-      if (cursor_on_p)
-	display_and_set_cursor (w, true,
-				w->output_cursor.hpos, w->output_cursor.vpos,
-				w->output_cursor.x, w->output_cursor.y);
-
-      if (draw_window_fringes (w, true))
-	{
-	  if (WINDOW_RIGHT_DIVIDER_WIDTH (w))
-	    gui_draw_right_divider (w);
-	  else
-	    gui_draw_vertical_border (w);
-	}
-
-      unblock_input ();
-    }
-
-  /* If a row with mouse-face was overwritten, arrange for
-     XTframe_up_to_date to redisplay the mouse highlight.  */
-  if (mouse_face_overwritten_p)
-    {
-      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (XFRAME (w->frame));
-
-      hlinfo->mouse_face_beg_row = hlinfo->mouse_face_beg_col = -1;
-      hlinfo->mouse_face_end_row = hlinfo->mouse_face_end_col = -1;
-      hlinfo->mouse_face_window = Qnil;
-    }
-
   /* Unhide the caret.  This won't actually show the cursor, unless it
      was visible before the corresponding call to HideCaret in
      w32_update_window_begin.  */
@@ -2859,7 +2782,7 @@ w32_scroll_run (struct window *w, struct run *run)
 
   block_input ();
 
-  /* Cursor off.  Will be switched on again in w32_update_window_end.  */
+  /* Cursor off.  Will be switched on again in gui_update_window_end.  */
   gui_clear_cursor (w);
 
   {
diff --git a/src/xdisp.c b/src/xdisp.c
index d52d1333a0..7450da59b6 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -18031,10 +18031,10 @@ try_window_reusing_current_matrix (struct window *w)
 	  if (run.height > 0 && run.current_y != run.desired_y)
 	    {
 	      update_begin (f);
-	      FRAME_RIF (f)->update_window_begin_hook (w);
+	      gui_update_window_begin (w);
 	      FRAME_RIF (f)->clear_window_mouse_face (w);
 	      FRAME_RIF (f)->scroll_run_hook (w, &run);
-	      FRAME_RIF (f)->update_window_end_hook (w, false, false);
+	      gui_update_window_end (w, false, false);
 	      update_end (f);
 	    }
 
@@ -18195,10 +18195,10 @@ try_window_reusing_current_matrix (struct window *w)
       if (run.height)
 	{
 	  update_begin (f);
-	  FRAME_RIF (f)->update_window_begin_hook (w);
+	  gui_update_window_begin (w);
 	  FRAME_RIF (f)->clear_window_mouse_face (w);
 	  FRAME_RIF (f)->scroll_run_hook (w, &run);
-	  FRAME_RIF (f)->update_window_end_hook (w, false, false);
+	  gui_update_window_end (w, false, false);
 	  update_end (f);
 	}
 
@@ -19147,10 +19147,10 @@ try_window_id (struct window *w)
 
       if (FRAME_WINDOW_P (f))
 	{
-	  FRAME_RIF (f)->update_window_begin_hook (w);
+	  gui_update_window_begin (w);
 	  FRAME_RIF (f)->clear_window_mouse_face (w);
 	  FRAME_RIF (f)->scroll_run_hook (w, &run);
-	  FRAME_RIF (f)->update_window_end_hook (w, false, false);
+	  gui_update_window_end (w, false, false);
 	}
       else
 	{
diff --git a/src/xterm.c b/src/xterm.c
index dd19b8bde1..26f74cde91 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -989,7 +989,7 @@ x_set_frame_alpha (struct frame *f)
 
 /* Start an update of frame F.  This function is installed as a hook
    for update_begin, i.e. it is called when update_begin is called.
-   This function is called prior to calls to x_update_window_begin for
+   This function is called prior to calls to gui_update_window_begin for
    each window being updated.  Currently, there is nothing to do here
    because all interesting stuff is done on a window basis.  */
 
@@ -1033,33 +1033,6 @@ x_update_begin (struct frame *f)
 #endif /* USE_CAIRO */
 }
 
-/* Start update of window W.  */
-
-static void
-x_update_window_begin (struct window *w)
-{
-  struct frame *f = XFRAME (WINDOW_FRAME (w));
-  Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (f);
-
-  w->output_cursor = w->cursor;
-
-  block_input ();
-
-  if (f == hlinfo->mouse_face_mouse_frame)
-    {
-      /* Don't do highlighting for mouse motion during the update.  */
-      hlinfo->mouse_face_defer = true;
-
-      /* If F needs to be redrawn, simply forget about any prior mouse
-	 highlighting.  */
-      if (FRAME_GARBAGED_P (f))
-	hlinfo->mouse_face_window = Qnil;
-    }
-
-  unblock_input ();
-}
-
-
 /* Draw a vertical window border from (x,y0) to (x,y1)  */
 
 static void
@@ -1139,55 +1112,6 @@ x_draw_window_divider (struct window *w, int x0, int x1, int y0, int y1)
     }
 }
 
-/* End update of window W.
-
-   Draw vertical borders between horizontally adjacent windows, and
-   display W's cursor if CURSOR_ON_P is non-zero.
-
-   MOUSE_FACE_OVERWRITTEN_P non-zero means that some row containing
-   glyphs in mouse-face were overwritten.  In that case we have to
-   make sure that the mouse-highlight is properly redrawn.
-
-   W may be a menu bar pseudo-window in case we don't have X toolkit
-   support.  Such windows don't have a cursor, so don't display it
-   here.  */
-
-static void
-x_update_window_end (struct window *w, bool cursor_on_p,
-		     bool mouse_face_overwritten_p)
-{
-  if (!w->pseudo_window_p)
-    {
-      block_input ();
-
-      if (cursor_on_p)
-	display_and_set_cursor (w, true,
-				w->output_cursor.hpos, w->output_cursor.vpos,
-				w->output_cursor.x, w->output_cursor.y);
-
-      if (draw_window_fringes (w, true))
-	{
-	  if (WINDOW_RIGHT_DIVIDER_WIDTH (w))
-	    gui_draw_right_divider (w);
-	  else
-	    gui_draw_vertical_border (w);
-	}
-
-      unblock_input ();
-    }
-
-  /* If a row with mouse-face was overwritten, arrange for
-     XTframe_up_to_date to redisplay the mouse highlight.  */
-  if (mouse_face_overwritten_p)
-    {
-      Mouse_HLInfo *hlinfo = MOUSE_HL_INFO (XFRAME (w->frame));
-
-      hlinfo->mouse_face_beg_row = hlinfo->mouse_face_beg_col = -1;
-      hlinfo->mouse_face_end_row = hlinfo->mouse_face_end_col = -1;
-      hlinfo->mouse_face_window = Qnil;
-    }
-}
-
 /* Show the frame back buffer.  If frame is double-buffered,
    atomically publish to the user's screen graphics updates made since
    the last call to show_back_buffer.  */
@@ -4306,7 +4230,7 @@ x_scroll_run (struct window *w, struct run *run)
 
   block_input ();
 
-  /* Cursor off.  Will be switched on again in x_update_window_end.  */
+  /* Cursor off.  Will be switched on again in gui_update_window_end.  */
   gui_clear_cursor (w);
 
 #ifdef USE_CAIRO
@@ -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,
+    NULL, /* update_window_begin */
+    NULL, /* update_window_end   */
     x_flip_and_flush,
     gui_clear_window_mouse_face,
     gui_get_glyph_overhangs,
-- 
2.21.0