bug#36337: 26.1; XBM images are sometimes not displayed correctly

[Pip Cet <pipcet@gmail.com>]

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

On Sun, Jun 23, 2019 at 4:41 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sun, 23 Jun 2019 16:26:16 +0000
> > Cc: triska@metalevel.at, 36337@debbugs.gnu.org
> >
> > My suggestion would be to expand `substring' to work on bool vectors,
> > then building a vector of bool vectors and using the existing code for
> > that case. Less code in image.c, plus a new utility function that
> > might be generally useful.
>
> Or maybe we should have a variant of make-bool-vector that accepts 2
> dimension s instead of just one?

I don't really see how that would be generally useful, to be honest.
In fact, I just played around with removing bool vector support
entirely.

> > (However, do we want to encourage people to use bool vectors?)
> Why not?

We seem to lack even very basic functions for interacting with bool
vectors, and hardly anyone appears to be using them. Even the :stipple
face property doesn't. Emacs starts up fine with bool vector support
removed. We can use vectors of nil/t (in most cases) or unibyte
strings or bignums (which have arbitrary size limits now, but
bigbignums would be just a few lines of code, I think).

And people _think_ bool vectors have a natural presentation as bytes,
but they don't, because some people start with the most significant
bit.

So I just don't see where bool vectors fit in.

> Evidently, it's convenient in this particular use case.

Is the convenience worth a thousand lines of code (much of it C) and
documentation?

[-- Attachment #2: 0001-Don-t-assume-the-width-of-xbm-images-is-divisible-by.patch --]
[-- Type: text/x-patch, Size: 3655 bytes --]

From 2fbff32843dceb6a903ca98a03b2f981c076d3b3 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sun, 23 Jun 2019 08:02:18 +0000
Subject: [PATCH] Don't assume the width of xbm images is divisible by 8.

---
 src/alloc.c |  2 +-
 src/data.c  | 20 ++++++++++++++++++++
 src/fns.c   |  3 +++
 src/image.c | 14 +++++++++++---
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 64aaa8acdf..a80fce078f 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -2210,7 +2210,7 @@ make_uninit_bool_vector (EMACS_INT nbits)
 
 DEFUN ("make-bool-vector", Fmake_bool_vector, Smake_bool_vector, 2, 2, 0,
        doc: /* Return a new bool-vector of length LENGTH, using INIT for each element.
-LENGTH must be a number.  INIT matters only in whether it is t or nil.  */)
+LENGTH must be a number.  INIT matters only in whether it is true or nil.  */)
   (Lisp_Object length, Lisp_Object init)
 {
   Lisp_Object val;
diff --git a/src/data.c b/src/data.c
index c1699aeae7..76b1c6ca4c 100644
--- a/src/data.c
+++ b/src/data.c
@@ -3783,6 +3783,25 @@ DEFUN ("bool-vector-count-consecutive", Fbool_vector_count_consecutive,
   return make_fixnum (count);
 }
 
+DEFUN ("bool-vector-extract", Fbool_vector_extract, Sbool_vector_extract, 1, 3, 0,
+       doc: /* Return a new bool vector which consists of the bits
+between index FROM (inclusive) and index TO (exclusive) of VECTOR.  */)
+  (Lisp_Object vector, Lisp_Object from, Lisp_Object to)
+{
+  Lisp_Object res;
+  ptrdiff_t size, ifrom, ito;
+
+  CHECK_BOOL_VECTOR (vector);
+  size = bool_vector_size (vector);
+  validate_subarray (vector, from, to, size, &ifrom, &ito);
+
+  res = make_uninit_bool_vector (ito - ifrom);
+
+  for (ptrdiff_t i = ifrom; i < ito; i++)
+    bool_vector_set (res, i - ifrom, bool_vector_bitref (vector, i));
+
+  return res;
+}
 \f
 void
 syms_of_data (void)
@@ -4064,6 +4083,7 @@ #define PUT_ERROR(sym, tail, msg)			\
   defsubr (&Sbool_vector_subsetp);
   defsubr (&Sbool_vector_count_consecutive);
   defsubr (&Sbool_vector_count_population);
+  defsubr (&Sbool_vector_extract);
 
   set_symbol_function (Qwholenump, XSYMBOL (Qnatnump)->u.s.function);
 
diff --git a/src/fns.c b/src/fns.c
index fd0c7fc71a..6bf469d1e9 100644
--- a/src/fns.c
+++ b/src/fns.c
@@ -1326,6 +1326,9 @@ DEFUN ("substring", Fsubstring, Ssubstring, 1, 3, 0,
   Lisp_Object res;
   ptrdiff_t size, ifrom, ito;
 
+  if (BOOL_VECTOR_P (string))
+    return Fbool_vector_extract (string, from, to);
+
   size = CHECK_VECTOR_OR_STRING (string);
   validate_subarray (string, from, to, size, &ifrom, &ito);
 
diff --git a/src/image.c b/src/image.c
index 7b648c46ae..79c8ac0df8 100644
--- a/src/image.c
+++ b/src/image.c
@@ -3242,7 +3242,7 @@ xbm_image_p (Lisp_Object object)
 	}
       else if (BOOL_VECTOR_P (data))
 	{
-	  if (bool_vector_size (data) / height < width)
+	  if (height > 0 && bool_vector_size (data) / height < width)
 	    return 0;
 	}
       else
@@ -3794,6 +3794,16 @@ xbm_load (struct frame *f, struct image *img)
 	{
 	  USE_SAFE_ALLOCA;
 
+	  if (BOOL_VECTOR_P (data))
+	    {
+	      Lisp_Object newdata = Fmake_vector (make_fixnum (img->height), Qnil);
+	      for (int y = 0; y < img->height; y++)
+		ASET (newdata, y, Fsubstring (data,
+					      make_fixnum (y * img->width),
+					      make_fixnum ((y + 1) * img->width)));
+	      data = newdata;
+	    }
+
 	  if (VECTORP (data))
 	    {
 	      int i;
@@ -3813,8 +3823,6 @@ xbm_load (struct frame *f, struct image *img)
 	    }
 	  else if (STRINGP (data))
 	    bits = SSDATA (data);
-	  else
-	    bits = (char *) bool_vector_data (data);
 
 #ifdef HAVE_NTGUI
           {
-- 
2.20.1