bug#37755: Logic in init_fringe_bitmap should be moved to backends (maybe rif->define_fringe_bitmap)

[Carlos Pita <carlosjosepita@gmail.com>]

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

This is the modified initialization logic implemented by the attached patch:

# When a frame is created, actually call rif->define_fringe_bitmap
# for each standard bitmap and also for lisp defined
# bitmaps that were created in a daemon with no frame / rif

gui_init_fringe (I assume this is called once per frame)
    for each standard bitmap
        rif->define_fringe_bitmap
    for each additional bitmap (recently introduced by [1])
        rif->define_fringe_bitmap

# When defined from lisp call rif->define_fringe_bitmap
# except that we're in daemon with no frame / rif

define-fringe-bitmap (lisp)
      when rif: rif->define_fringe_bitmap

Much simpler, don't you think? Also removed ~30 LOCs.

Except that I'm overlooking something obvious it seems to do exactly
the same, only that the bit shuffling part is postponed until the
frame is actually created (that is, moved into
rif->define_fringe_bitmap).

Some remarks:

1. There was this comment on top of init_fringe_bitmap: "On MAC with
big-endian CPU, we need to byte-swap each short". But all the code
there were either specific to NTGUI, specific to XWINDOWS or related
to bitmap destruction. So nothing remained that could be actually
moved into the NS backend.

2. I left the HAVE_NTGUI guard even after moving the relevant code
into w32term.c because I'm not sure whether all w32 code share the
NTGUI path or not.

3. The previous implementation modified bits as a way of connecting
init with define, this was the state passed from one stage to the
other. Now, since both parts are done together, there is no need to
modify the passed bits array, the array is only required in order to
initialize backend-specific structures. Nevertheless, I decided to
keep that as it was in order to prevent regressions, but I believe an
implementation that preserves the original value of bits would be
preferable now that it is indeed possible. In particular, for X the
only "backend-specific structure" is an utterly modified bits array
(in some cases shorts are converted into chars, that's why I said in
#37689 that it's difficult for the upper layer to "split rows" after
this kind of manipulations had already taken place).

Any reviews would be much appreciated.

Best regards
--
Carlos

[-- Attachment #2: 0001-Fringe-refactor-move-platform-specific-code-into-red.patch --]
[-- Type: text/x-patch, Size: 8538 bytes --]

From c4792b1d7ad7957bb07107d6e461173ab42d87a7 Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
Date: Tue, 15 Oct 2019 16:37:24 -0300
Subject: [PATCH] Fringe refactor: move platform-specific code into redisplay
 interface

* src/fringe.c: remove init_fringe_bitmap
* src/w32term.c: add NTGUI parts to w32_define_fringe_bitmap
* src/xterm.c:
  - New interface function for X11: x_define_fringe_bitmap
  - add XWINDOWS parts to x_define_fringe_bitmap
  - add CAIRO parts to x_cr_define_fringe_bitmap

Detailed discussion: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37755
---
 src/fringe.c  | 128 +++++---------------------------------------------
 src/w32term.c |  12 +++++
 src/xterm.c   |  58 ++++++++++++++++++++++-
 3 files changed, 80 insertions(+), 118 deletions(-)

diff --git a/src/fringe.c b/src/fringe.c
index 22f3bdc..a17b684 100644
--- a/src/fringe.c
+++ b/src/fringe.c
@@ -1388,112 +1388,6 @@ DEFUN ("destroy-fringe-bitmap", Fdestroy_fringe_bitmap, Sdestroy_fringe_bitmap,
   return Qnil;
 }
 
-
-/* Initialize bitmap bit.
-
-   On X, we bit-swap the built-in bitmaps and reduce bitmap
-   from short to char array if width is <= 8 bits.
-
-   On MAC with big-endian CPU, we need to byte-swap each short.
-
-   On W32 and MAC (little endian), there's no need to do this.
-*/
-
-#if defined (HAVE_X_WINDOWS)
-static const unsigned char swap_nibble[16] = {
-  0x0, 0x8, 0x4, 0xc,           /* 0000 1000 0100 1100 */
-  0x2, 0xa, 0x6, 0xe,           /* 0010 1010 0110 1110 */
-  0x1, 0x9, 0x5, 0xd,           /* 0001 1001 0101 1101 */
-  0x3, 0xb, 0x7, 0xf};          /* 0011 1011 0111 1111 */
-#endif                          /* HAVE_X_WINDOWS */
-
-static void
-init_fringe_bitmap (int which, struct fringe_bitmap *fb, int once_p)
-{
-  if (once_p || fb->dynamic)
-    {
-#if defined (HAVE_X_WINDOWS)
-      unsigned short *bits = fb->bits;
-      int j;
-
-#ifdef USE_CAIRO
-      for (j = 0; j < fb->height; j++)
-	{
-	  unsigned short b = *bits;
-#ifdef WORDS_BIGENDIAN
-	  *bits++ = (b << (16 - fb->width));
-#else
-	  b = (unsigned short)((swap_nibble[b & 0xf] << 12)
-			       | (swap_nibble[(b>>4) & 0xf] << 8)
-			       | (swap_nibble[(b>>8) & 0xf] << 4)
-			       | (swap_nibble[(b>>12) & 0xf]));
-	  *bits++ = (b >> (16 - fb->width));
-#endif
-	}
-#else  /* not USE_CAIRO */
-      if (fb->width <= 8)
-	{
-	  unsigned char *cbits = (unsigned char *)fb->bits;
-	  for (j = 0; j < fb->height; j++)
-	    {
-	      unsigned short b = *bits++;
-	      unsigned char c;
-	      c = (unsigned char)((swap_nibble[b & 0xf] << 4)
-				  | (swap_nibble[(b>>4) & 0xf]));
-	      *cbits++ = (c >> (8 - fb->width));
-	    }
-	}
-      else
-	{
-	  for (j = 0; j < fb->height; j++)
-	    {
-	      unsigned short b = *bits;
-	      b = (unsigned short)((swap_nibble[b & 0xf] << 12)
-				   | (swap_nibble[(b>>4) & 0xf] << 8)
-				   | (swap_nibble[(b>>8) & 0xf] << 4)
-				   | (swap_nibble[(b>>12) & 0xf]));
-	      b >>= (16 - fb->width);
-#ifdef WORDS_BIGENDIAN
-	      b = bswap_16 (b);
-#endif
-	      *bits++ = b;
-	    }
-	}
-#endif /* not USE_CAIRO */
-#endif /* HAVE_X_WINDOWS */
-
-#ifdef HAVE_NTGUI
-      unsigned short *bits = fb->bits;
-      int j;
-      for (j = 0; j < fb->height; j++)
-	{
-	  unsigned short b = *bits;
-	  b <<= (16 - fb->width);
-	  /* Windows is little-endian, so the next line is always
-	     needed.  */
-	  b = ((b >> 8) | (b << 8));
-	  *bits++ = b;
-	}
-#endif
-    }
-
-  if (!once_p)
-    {
-      /* XXX Is SELECTED_FRAME OK here? */
-      struct redisplay_interface *rif = FRAME_RIF (SELECTED_FRAME ());
-
-      destroy_fringe_bitmap (which);
-
-      if (rif && rif->define_fringe_bitmap)
-	rif->define_fringe_bitmap (which, fb->bits, fb->height, fb->width);
-
-      fringe_bitmaps[which] = fb;
-      if (which >= max_used_fringe_bitmap)
-	max_used_fringe_bitmap = which + 1;
-    }
-}
-
-
 DEFUN ("define-fringe-bitmap", Fdefine_fringe_bitmap, Sdefine_fringe_bitmap,
        2, 5, 0,
        doc: /* Define fringe bitmap BITMAP from BITS of size HEIGHT x WIDTH.
@@ -1625,7 +1519,17 @@ list (ALIGN PERIODIC) where PERIODIC non-nil specifies that the bitmap
 
   *xfb = fb;
 
-  init_fringe_bitmap (n, xfb, 0);
+  /* XXX Is SELECTED_FRAME OK here? */
+  struct redisplay_interface *rif = FRAME_RIF (SELECTED_FRAME ());
+
+  destroy_fringe_bitmap (n);
+
+  if (rif && rif->define_fringe_bitmap)
+    rif->define_fringe_bitmap (n, xfb->bits, xfb->height, xfb->width);
+
+  fringe_bitmaps[n] = xfb;
+  if (n >= max_used_fringe_bitmap)
+    max_used_fringe_bitmap = n + 1;
 
   return bitmap;
 }
@@ -1743,19 +1647,9 @@ mark_fringe_data (void)
 
 /* Initialize this module when Emacs starts.  */
 
-static void init_fringe_once_for_pdumper (void);
-
 void
 init_fringe_once (void)
 {
-  pdumper_do_now_and_after_load (init_fringe_once_for_pdumper);
-}
-
-static void
-init_fringe_once_for_pdumper (void)
-{
-  for (int bt = NO_FRINGE_BITMAP + 1; bt < MAX_STANDARD_FRINGE_BITMAPS; bt++)
-    init_fringe_bitmap (bt, &standard_bitmaps[bt], 1);
 }
 
 void
diff --git a/src/w32term.c b/src/w32term.c
index 9da0845..c43ad81 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -835,6 +835,18 @@ w32_draw_fringe_bitmap (struct window *w, struct glyph_row *row,
 static void
 w32_define_fringe_bitmap (int which, unsigned short *bits, int h, int wd)
 {
+#ifdef HAVE_NTGUI
+  int j;
+  for (j = 0; j < h; j++)
+    {
+      unsigned short b = *bits;
+      b <<= (16 - wd);
+      /* Windows is little-endian, so the next line is always
+         needed.  */
+      b = ((b >> 8) | (b << 8));
+      *bits++ = b;
+    }
+#endif
   if (which >= max_fringe_bmp)
     {
       int i = max_fringe_bmp;
diff --git a/src/xterm.c b/src/xterm.c
index 5d8b148..a8c2944 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -548,6 +548,11 @@ x_end_cr_xlib_drawable (struct frame *f, GC gc)
 
 static int max_fringe_bmp = 0;
 static cairo_pattern_t **fringe_bmp = 0;
+static const unsigned char swap_nibble[16] = {
+  0x0, 0x8, 0x4, 0xc,           /* 0000 1000 0100 1100 */
+  0x2, 0xa, 0x6, 0xe,           /* 0010 1010 0110 1110 */
+  0x1, 0x9, 0x5, 0xd,           /* 0001 1001 0101 1101 */
+  0x3, 0xb, 0x7, 0xf};          /* 0011 1011 0111 1111 */
 
 static void
 x_cr_define_fringe_bitmap (int which, unsigned short *bits, int h, int wd)
@@ -557,6 +562,20 @@ x_cr_define_fringe_bitmap (int which, unsigned short *bits, int h, int wd)
   cairo_pattern_t *pattern;
   unsigned char *data;
 
+  for (i = 0; i < h; i++)
+    {
+      unsigned short b = *bits;
+#ifdef WORDS_BIGENDIAN
+      *bits++ = (b << (16 - fb->width));
+#else
+      b = (unsigned short)((swap_nibble[b & 0xf] << 12)
+                           | (swap_nibble[(b>>4) & 0xf] << 8)
+                           | (swap_nibble[(b>>8) & 0xf] << 4)
+                           | (swap_nibble[(b>>12) & 0xf]));
+      *bits++ = (b >> (16 - wd));
+#endif
+    }
+
   if (which >= max_fringe_bmp)
     {
       i = max_fringe_bmp;
@@ -1393,6 +1412,43 @@ x_after_update_window_line (struct window *w, struct glyph_row *desired_row)
 #endif
 }
 
+static void
+x_define_fringe_bitmap (int which, unsigned short *bits, int h, int wd)
+{
+  /* On X, we bit-swap the built-in bitmaps and reduce bitmap */
+  /* from short to char array if width is <= 8 bits. */
+  int j;
+
+  if (wd <= 8)
+    {
+      unsigned char *cbits = (unsigned char *)bits;
+      for (j = 0; j < h; j++)
+        {
+          unsigned short b = *bits++;
+          unsigned char c;
+          c = (unsigned char)((swap_nibble[b & 0xf] << 4)
+                              | (swap_nibble[(b>>4) & 0xf]));
+          *cbits++ = (c >> (8 - wd));
+        }
+    }
+  else
+    {
+      for (j = 0; j < h; j++)
+        {
+          unsigned short b = *bits;
+          b = (unsigned short)((swap_nibble[b & 0xf] << 12)
+                               | (swap_nibble[(b>>4) & 0xf] << 8)
+                               | (swap_nibble[(b>>8) & 0xf] << 4)
+                               | (swap_nibble[(b>>12) & 0xf]));
+          b >>= (16 - wd);
+#ifdef WORDS_BIGENDIAN
+          b = bswap_16 (b);
+#endif
+          *bits++ = b;
+        }
+    }
+}
+
 static void
 x_draw_fringe_bitmap (struct window *w, struct glyph_row *row, struct draw_fringe_bitmap_params *p)
 {
@@ -13361,7 +13417,7 @@ x_activate_timeout_atimer (void)
     x_cr_define_fringe_bitmap,
     x_cr_destroy_fringe_bitmap,
 #else
-    0, /* define_fringe_bitmap */
+    x_define_fringe_bitmap,
     0, /* destroy_fringe_bitmap */
 #endif
     x_compute_glyph_string_overhangs,
-- 
2.20.1