bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Gregory Heytings]

When the init file contains define-fringe-bitmap that override default 
fringe bitmaps, they are ignored by Emacs when it is started as a daemon. 
This has stopped working at commit 88efc736f5, when Cairo became the 
default backend.

How to reproduce:

1. create a ~/.emacs file with:

(define-fringe-bitmap 'empty-line [ #xff ] nil nil '(top t))
(setq-default indicate-empty-lines t)

2. emacs ~/.emacs => the empty lines fringe bitmaps are displayed 
correctly, with a solid black column.

3. emacs --daemon

4. emacsclient -c ~/.emacs => the empty line fringe bitmaps are the 
default ones, which are 4x1 pixel horizontal black lines.

When the init file contains define-fringe-bitmap that do _not_ override 
default fringe bitmaps however, they are not ignored.  For example with

(define-fringe-bitmap 'solid [ #xff ] nil nil '(top t))

the "solid" fringe bitmap can be used in frames created by emacsclient -c.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> Date: Fri, 16 Apr 2021 22:22:35 +0000
> From: Gregory Heytings <gregory@heytings.org>
> 
> When the init file contains define-fringe-bitmap that override default 
> fringe bitmaps, they are ignored by Emacs when it is started as a daemon. 
> This has stopped working at commit 88efc736f5, when Cairo became the 
> default backend.

That commit simply made Cairo being available by default, if found on
the system.  It didn't change any code.  Are you saying that non-Cairo
build shows this problem as well, after that commit?  Or is the
problem limited to Cairo builds on your system?

> 1. create a ~/.emacs file with:
> 
> (define-fringe-bitmap 'empty-line [ #xff ] nil nil '(top t))
> (setq-default indicate-empty-lines t)
> 
> 2. emacs ~/.emacs => the empty lines fringe bitmaps are displayed 
> correctly, with a solid black column.
> 
> 3. emacs --daemon
> 
> 4. emacsclient -c ~/.emacs => the empty line fringe bitmaps are the 
> default ones, which are 4x1 pixel horizontal black lines.

In general, display features that need GUI framework should be turned
on from after-make-frame-functions or server-after-make-frame-hook to
work reliably in daemon-based sessions.  However, if this used to
work, it would be good to understand which change broke it.  But I
doubt that the commit you identified is the culprit: I see the same
behavior on MS-Windows, where Cairo and the commit you pointed to have
no effect whatsoever.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Gregory Heytings]

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


>
> However, if this used to work,
>

This used to work indeed; it worked on GNU/Linux with default (non-Cairo) 
builds for Emacs 24 to 27.

>
> it would be good to understand which change broke it.
>

Got it! :-)  The commit I identified is indeed the culprit.  With 
USE_CAIRO, gui_init_fringe() is called in x_initialize(); without 
USE_CAIRO it isn't.  And gui_init_fringe() had a bug, which should now be 
fixed on Windows, too.  Patch attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Consider-all-user-defined-bitmaps-in-gui_init_fringe.patch, Size: 988 bytes --]

From 4d5a18bbb18496eb02e949b993cc3f9bca7c4d91 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 17 Apr 2021 09:37:21 +0000
Subject: [PATCH] Consider all user-defined bitmaps in gui_init_fringe()

* src/fringe.c (gui_init_fringe): Also consider user-defined bitmaps that
override default ones (Bug#47832).
---
 src/fringe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/fringe.c b/src/fringe.c
index 65c9a84ac9..f2b60b5c8e 100644
--- a/src/fringe.c
+++ b/src/fringe.c
@@ -1783,7 +1783,7 @@ gui_init_fringe (struct redisplay_interface *rif)
      before the frame of this kind was initialized.  This can happen
      if Emacs is started as a daemon and the init files define fringe
      bitmaps.  */
-  for ( ; bt < max_used_fringe_bitmap; bt++)
+  for (bt = NO_FRINGE_BITMAP + 1; bt < max_used_fringe_bitmap; bt++)
     {
       struct fringe_bitmap *fb = fringe_bitmaps[bt];
       if (fb)
-- 
2.30.2

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> Date: Sat, 17 Apr 2021 09:49:46 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 47832@debbugs.gnu.org
> 
> This used to work indeed; it worked on GNU/Linux with default (non-Cairo) 
> builds for Emacs 24 to 27.

And with Emacs 28 it doesn't work in non-Cairo builds?

> Got it! :-)  The commit I identified is indeed the culprit.  With 
> USE_CAIRO, gui_init_fringe() is called in x_initialize(); without 
> USE_CAIRO it isn't.  And gui_init_fringe() had a bug, which should now be 
> fixed on Windows, too.  Patch attached.

Thanks, but is this really all that needs to be done?  How will
gui_init_fringe be called in the non-Cairo builds?  And what about NS?

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> Date: Sat, 17 Apr 2021 13:57:14 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 47832@debbugs.gnu.org
> 
> > Got it! :-)  The commit I identified is indeed the culprit.  With 
> > USE_CAIRO, gui_init_fringe() is called in x_initialize(); without 
> > USE_CAIRO it isn't.  And gui_init_fringe() had a bug, which should now be 
> > fixed on Windows, too.  Patch attached.
> 
> Thanks, but is this really all that needs to be done?  How will
> gui_init_fringe be called in the non-Cairo builds?  And what about NS?

Answering myself: they don't need to call gui_init_fringe.  Since
gui_init_fringe returns immediately if the frame interface doesn't
provide a method for fringe initialization, I think we should call
gui_init_fringe unconditionally on X.

And in any case, the patch for gui_init_fringe is sub-optimal: it
unnecessarily loops over the standard bitmaps that were superseded.
It is better to leave the first loop go over the standard bitmaps,
whether superseded or not, and the second loop go over non-standard
bitmaps only.

Thanks.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Gregory Heytings]

>> This used to work indeed; it worked on GNU/Linux with default 
>> (non-Cairo) builds for Emacs 24 to 27.
>
> And with Emacs 28 it doesn't work in non-Cairo builds?
>

Yes, with Emacs 28 it works with non-Cairo builds.

>> Got it! :-)  The commit I identified is indeed the culprit.  With 
>> USE_CAIRO, gui_init_fringe() is called in x_initialize(); without 
>> USE_CAIRO it isn't.  And gui_init_fringe() had a bug, which should now 
>> be fixed on Windows, too.  Patch attached.
>
> Thanks, but is this really all that needs to be done?  How will 
> gui_init_fringe be called in the non-Cairo builds?  And what about NS?
>

On non-Cairo builds gui_init_finge is not called, the only call to 
gui_init_fringe in xterm.c is inside an #if USE_CAIRO.  And on NS 
gui_init_fringe isn't called either.  I just checked, NS builds are not 
affected by this bug.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Gregory Heytings]

>>> Got it! :-)  The commit I identified is indeed the culprit.  With 
>>> USE_CAIRO, gui_init_fringe() is called in x_initialize(); without 
>>> USE_CAIRO it isn't.  And gui_init_fringe() had a bug, which should now 
>>> be fixed on Windows, too.  Patch attached.
>>
>> Thanks, but is this really all that needs to be done?  How will 
>> gui_init_fringe be called in the non-Cairo builds?  And what about NS?
>
> Answering myself: they don't need to call gui_init_fringe.
>

Indeed.

>
> Since gui_init_fringe returns immediately if the frame interface doesn't 
> provide a method for fringe initialization, I think we should call 
> gui_init_fringe unconditionally on X.
>
> And in any case, the patch for gui_init_fringe is sub-optimal: it 
> unnecessarily loops over the standard bitmaps that were superseded. It 
> is better to leave the first loop go over the standard bitmaps, whether 
> superseded or not, and the second loop go over non-standard bitmaps 
> only.
>

It is not sub-optimal, rif->define_fringe_bitmap will only be called for 
those standard bitmaps that were superseded.  For the non-superseded ones, 
fringe_bitmaps[bt] is NULL.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Gregory Heytings]

>
> Since gui_init_fringe returns immediately if the frame interface doesn't 
> provide a method for fringe initialization, I think we should call 
> gui_init_fringe unconditionally on X.
>

Wouldn't that leave the bug unfixed on Windows?

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> Date: Sat, 17 Apr 2021 11:32:25 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 47832@debbugs.gnu.org
> 
> > Since gui_init_fringe returns immediately if the frame interface doesn't 
> > provide a method for fringe initialization, I think we should call 
> > gui_init_fringe unconditionally on X.

Here I meant to call gui_init_fringe unconditionally.  It will make
the code less confusing.

> > And in any case, the patch for gui_init_fringe is sub-optimal: it 
> > unnecessarily loops over the standard bitmaps that were superseded. It 
> > is better to leave the first loop go over the standard bitmaps, whether 
> > superseded or not, and the second loop go over non-standard bitmaps 
> > only.
> 
> It is not sub-optimal, rif->define_fringe_bitmap will only be called for 
> those standard bitmaps that were superseded.  For the non-superseded ones, 
> fringe_bitmaps[bt] is NULL.

I meant the first loop, not the second one.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> Date: Sat, 17 Apr 2021 11:34:06 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: 47832@debbugs.gnu.org
> 
> > Since gui_init_fringe returns immediately if the frame interface doesn't 
> > provide a method for fringe initialization, I think we should call 
> > gui_init_fringe unconditionally on X.
> >
> 
> Wouldn't that leave the bug unfixed on Windows?

No, because Windows calls it in another place.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Gregory Heytings]

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


>>> And in any case, the patch for gui_init_fringe is sub-optimal: it 
>>> unnecessarily loops over the standard bitmaps that were superseded. It 
>>> is better to leave the first loop go over the standard bitmaps, 
>>> whether superseded or not, and the second loop go over non-standard 
>>> bitmaps only.
>>
>> It is not sub-optimal, rif->define_fringe_bitmap will only be called 
>> for those standard bitmaps that were superseded.  For the 
>> non-superseded ones, fringe_bitmaps[bt] is NULL.
>
> I meant the first loop, not the second one.
>

Okay, the patch was meant only to fix the bug, but indeed the small 
optimization you mention makes sense.  Updated patch attached.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Consider-all-user-defined-bitmaps-in-gui_init_fringe.patch, Size: 1395 bytes --]

From d02af2c7007b2ba2ffa6c0fb47e33e65116808c4 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Sat, 17 Apr 2021 12:48:48 +0000
Subject: [PATCH] Consider all user-defined bitmaps in gui_init_fringe()

* src/fringe.c (gui_init_fringe): Consider user-defined bitmaps that
override default ones (Bug#47832).
---
 src/fringe.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/fringe.c b/src/fringe.c
index 65c9a84ac9..47615f51f9 100644
--- a/src/fringe.c
+++ b/src/fringe.c
@@ -1776,14 +1776,15 @@ gui_init_fringe (struct redisplay_interface *rif)
   for (bt = NO_FRINGE_BITMAP + 1; bt < MAX_STANDARD_FRINGE_BITMAPS; bt++)
     {
       struct fringe_bitmap *fb = &standard_bitmaps[bt];
-      rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
+      if (!fringe_bitmaps[bt])
+        rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
     }
 
   /* Set up user-defined fringe bitmaps that might have been defined
      before the frame of this kind was initialized.  This can happen
      if Emacs is started as a daemon and the init files define fringe
      bitmaps.  */
-  for ( ; bt < max_used_fringe_bitmap; bt++)
+  for (bt = NO_FRINGE_BITMAP + 1; bt < max_used_fringe_bitmap; bt++)
     {
       struct fringe_bitmap *fb = fringe_bitmaps[bt];
       if (fb)
-- 
2.30.2

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Lars Ingebrigtsen]

Gregory Heytings <gregory@heytings.org> writes:

> Okay, the patch was meant only to fix the bug, but indeed the small
> optimization you mention makes sense.  Updated patch attached.

Thanks; applied to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  47832@debbugs.gnu.org
> Date: Tue, 25 May 2021 06:21:19 +0200
> 
> Gregory Heytings <gregory@heytings.org> writes:
> 
> > Okay, the patch was meant only to fix the bug, but indeed the small
> > optimization you mention makes sense.  Updated patch attached.
> 
> Thanks; applied to Emacs 28.

Bother:

> diff --git a/src/fringe.c b/src/fringe.c
> index 65c9a84..47615f5 100644
> --- a/src/fringe.c
> +++ b/src/fringe.c
> @@ -1776,14 +1776,15 @@ gui_init_fringe (struct redisplay_interface *rif)
>    for (bt = NO_FRINGE_BITMAP + 1; bt < MAX_STANDARD_FRINGE_BITMAPS; bt++)
>      {
>        struct fringe_bitmap *fb = &standard_bitmaps[bt];
> -      rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
> +      if (!fringe_bitmaps[bt])
> +        rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
>      }
>  
>    /* Set up user-defined fringe bitmaps that might have been defined
>       before the frame of this kind was initialized.  This can happen
>       if Emacs is started as a daemon and the init files define fringe
>       bitmaps.  */
> -  for ( ; bt < max_used_fringe_bitmap; bt++)
> +  for (bt = NO_FRINGE_BITMAP + 1; bt < max_used_fringe_bitmap; bt++)
>      {
>        struct fringe_bitmap *fb = fringe_bitmaps[bt];
>        if (fb)

Why does the second for-loop go again over the bitmaps that the first
one already processed?  Or what am I missing?

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Gregory Heytings]

>> diff --git a/src/fringe.c b/src/fringe.c
>> index 65c9a84..47615f5 100644
>> --- a/src/fringe.c
>> +++ b/src/fringe.c
>> @@ -1776,14 +1776,15 @@ gui_init_fringe (struct redisplay_interface *rif)
>>    for (bt = NO_FRINGE_BITMAP + 1; bt < MAX_STANDARD_FRINGE_BITMAPS; bt++)
>>      {
>>        struct fringe_bitmap *fb = &standard_bitmaps[bt];
>> -      rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
>> +      if (!fringe_bitmaps[bt])
>> +        rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
>>      }
>>
>>    /* Set up user-defined fringe bitmaps that might have been defined
>>       before the frame of this kind was initialized.  This can happen
>>       if Emacs is started as a daemon and the init files define fringe
>>       bitmaps.  */
>> -  for ( ; bt < max_used_fringe_bitmap; bt++)
>> +  for (bt = NO_FRINGE_BITMAP + 1; bt < max_used_fringe_bitmap; bt++)
>>      {
>>        struct fringe_bitmap *fb = fringe_bitmaps[bt];
>>        if (fb)
>
> Why does the second for-loop go again over the bitmaps that the first 
> one already processed?  Or what am I missing?
>

Each loop processes its "own" bitmaps.  In the first loop 
define_fringe_bitmap is called only if (!fringe_bitmaps[bt]), in the 
second loop it is called only if (fringe_bitmaps[bt]).  IOW, the first 
loop processes the standard bitmaps that are not overridden by 
user-defined bitmaps, and the second loop processes user-defined bitmaps.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> Date: Tue, 25 May 2021 12:44:34 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Lars Ingebrigtsen <larsi@gnus.org>, 47832@debbugs.gnu.org
> 
> 
> >> diff --git a/src/fringe.c b/src/fringe.c
> >> index 65c9a84..47615f5 100644
> >> --- a/src/fringe.c
> >> +++ b/src/fringe.c
> >> @@ -1776,14 +1776,15 @@ gui_init_fringe (struct redisplay_interface *rif)
> >>    for (bt = NO_FRINGE_BITMAP + 1; bt < MAX_STANDARD_FRINGE_BITMAPS; bt++)
> >>      {
> >>        struct fringe_bitmap *fb = &standard_bitmaps[bt];
> >> -      rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
> >> +      if (!fringe_bitmaps[bt])
> >> +        rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
> >>      }
> >>
> >>    /* Set up user-defined fringe bitmaps that might have been defined
> >>       before the frame of this kind was initialized.  This can happen
> >>       if Emacs is started as a daemon and the init files define fringe
> >>       bitmaps.  */
> >> -  for ( ; bt < max_used_fringe_bitmap; bt++)
> >> +  for (bt = NO_FRINGE_BITMAP + 1; bt < max_used_fringe_bitmap; bt++)
> >>      {
> >>        struct fringe_bitmap *fb = fringe_bitmaps[bt];
> >>        if (fb)
> >
> > Why does the second for-loop go again over the bitmaps that the first 
> > one already processed?  Or what am I missing?
> >
> 
> Each loop processes its "own" bitmaps.  In the first loop 
> define_fringe_bitmap is called only if (!fringe_bitmaps[bt]), in the 
> second loop it is called only if (fringe_bitmaps[bt]).  IOW, the first 
> loop processes the standard bitmaps that are not overridden by 
> user-defined bitmaps, and the second loop processes user-defined bitmaps.

If the standard bitmaps were overridden by user-defined ones, why do
we need to set those overriding user-defined bitmaps once again?

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Gregory Heytings]

>> Each loop processes its "own" bitmaps.  In the first loop 
>> define_fringe_bitmap is called only if (!fringe_bitmaps[bt]), in the 
>> second loop it is called only if (fringe_bitmaps[bt]).  IOW, the first 
>> loop processes the standard bitmaps that are not overridden by 
>> user-defined bitmaps, and the second loop processes user-defined 
>> bitmaps.
>
> If the standard bitmaps were overridden by user-defined ones, why do we 
> need to set those overriding user-defined bitmaps once again?
>

Sorry, I don't understand your question.

There are three kind of bitmaps:

1. standard bitmaps not overridden by user-defined bitmaps
2. standard bitmaps overridden by user-defined bitmaps
3. user-defined bitmaps

The first loop processes the bitmaps of the first kind, the second loop 
the bitmaps of the second and third kind.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> Date: Tue, 25 May 2021 13:03:25 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: larsi@gnus.org, 47832@debbugs.gnu.org
> 
> > If the standard bitmaps were overridden by user-defined ones, why do we 
> > need to set those overriding user-defined bitmaps once again?
> >
> 
> Sorry, I don't understand your question.
> 
> There are three kind of bitmaps:
> 
> 1. standard bitmaps not overridden by user-defined bitmaps
> 2. standard bitmaps overridden by user-defined bitmaps
> 3. user-defined bitmaps
> 
> The first loop processes the bitmaps of the first kind, the second loop 
> the bitmaps of the second and third kind.

I'm asking why do we need to process the second kind here.  That kind
was already set up when user initializations defined replacements for
those standard bitmaps.  Right?  So why do we need to set them up
again?

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Gregory Heytings]

>> There are three kind of bitmaps:
>>
>> 1. standard bitmaps not overridden by user-defined bitmaps
>> 2. standard bitmaps overridden by user-defined bitmaps
>> 3. user-defined bitmaps
>>
>> The first loop processes the bitmaps of the first kind, the second loop 
>> the bitmaps of the second and third kind.
>
> I'm asking why do we need to process the second kind here.  That kind 
> was already set up when user initializations defined replacements for 
> those standard bitmaps.  Right?  So why do we need to set them up again?
>

What you write is correct when you call "emacs", not when you call "emacs 
--daemon".  In the latter case the user-defined bitmaps have not yet been 
set up.  It's explained in the comment:

    /* Set up user-defined fringe bitmaps that might have been defined
       before the frame of this kind was initialized.  This can happen
       if Emacs is started as a daemon and the init files define fringe
       bitmaps.  */

The code was written under the erroneous assumption that there are only 
two kind of bitmaps: standard bitmaps and user-defined bitmaps.  Hence the 
bug that this patch fixes.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> Date: Tue, 25 May 2021 13:24:14 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: larsi@gnus.org, 47832@debbugs.gnu.org
> 
> > I'm asking why do we need to process the second kind here.  That kind 
> > was already set up when user initializations defined replacements for 
> > those standard bitmaps.  Right?  So why do we need to set them up again?
> 
> What you write is correct when you call "emacs", not when you call "emacs 
> --daemon".  In the latter case the user-defined bitmaps have not yet been 
> set up.  It's explained in the comment:
> 
>     /* Set up user-defined fringe bitmaps that might have been defined
>        before the frame of this kind was initialized.  This can happen
>        if Emacs is started as a daemon and the init files define fringe
>        bitmaps.  */

OK, but then perhaps we should only do that in a daemon?  Or at least
say in a comment there that the second loop does some redundant
initializations except under --daemon.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> Date: Tue, 25 May 2021 16:45:00 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: larsi@gnus.org, 47832@debbugs.gnu.org
> 
> > Date: Tue, 25 May 2021 13:24:14 +0000
> > From: Gregory Heytings <gregory@heytings.org>
> > cc: larsi@gnus.org, 47832@debbugs.gnu.org
> > 
> > > I'm asking why do we need to process the second kind here.  That kind 
> > > was already set up when user initializations defined replacements for 
> > > those standard bitmaps.  Right?  So why do we need to set them up again?
> > 
> > What you write is correct when you call "emacs", not when you call "emacs 
> > --daemon".  In the latter case the user-defined bitmaps have not yet been 
> > set up.  It's explained in the comment:
> > 
> >     /* Set up user-defined fringe bitmaps that might have been defined
> >        before the frame of this kind was initialized.  This can happen
> >        if Emacs is started as a daemon and the init files define fringe
> >        bitmaps.  */
> 
> OK, but then perhaps we should only do that in a daemon?  Or at least
> say in a comment there that the second loop does some redundant
> initializations except under --daemon.

Actually, I still think that either I misunderstand something, or
disagree with what you say.  So let me step back and explain what
bothers me.

AFAIR, the original bug was that the call to gui_init_fringe, made
when the first GUI frame is created, would overwrite any standard
bitmaps that were modified by previous calls to define-fringe-bitmap,
because the first loop in gui_init_fringe re-installs the original
standard bitmaps.

Your original patch then fixed this by making the _second_ loop go
over _all_ of the bitmaps, not just non-standard ones:

  diff --git a/src/fringe.c b/src/fringe.c
  index 65c9a84ac9..f2b60b5c8e 100644
  --- a/src/fringe.c
  +++ b/src/fringe.c
  @@ -1783,7 +1783,7 @@ gui_init_fringe (struct redisplay_interface *rif)
	before the frame of this kind was initialized.  This can happen
	if Emacs is started as a daemon and the init files define fringe
	bitmaps.  */
  -  for ( ; bt < max_used_fringe_bitmap; bt++)
  +  for (bt = NO_FRINGE_BITMAP + 1; bt < max_used_fringe_bitmap; bt++)
       {
	 struct fringe_bitmap *fb = fringe_bitmaps[bt];
	 if (fb)

This would reinstate the user-defined bitmaps for the standard ones.

I responded saying that the original patch is sub-optimal:

  And in any case, the patch for gui_init_fringe is sub-optimal: it
  unnecessarily loops over the standard bitmaps that were superseded.
  It is better to leave the first loop go over the standard bitmaps,
  whether superseded or not, and the second loop go over non-standard
  bitmaps only.

Then your modified patch attempted to fix this:

  diff --git a/src/fringe.c b/src/fringe.c
  index 65c9a84ac9..47615f51f9 100644
  --- a/src/fringe.c
  +++ b/src/fringe.c
  @@ -1776,14 +1776,15 @@ gui_init_fringe (struct redisplay_interface *rif)
     for (bt = NO_FRINGE_BITMAP + 1; bt < MAX_STANDARD_FRINGE_BITMAPS; bt++)
       {
	 struct fringe_bitmap *fb = &standard_bitmaps[bt];
  -      rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
  +      if (!fringe_bitmaps[bt])
  +        rif->define_fringe_bitmap (bt, fb->bits, fb->height, fb->width);
       }

     /* Set up user-defined fringe bitmaps that might have been defined
	before the frame of this kind was initialized.  This can happen
	if Emacs is started as a daemon and the init files define fringe
	bitmaps.  */
  -  for ( ; bt < max_used_fringe_bitmap; bt++)
  +  for (bt = NO_FRINGE_BITMAP + 1; bt < max_used_fringe_bitmap; bt++)
       {
	 struct fringe_bitmap *fb = fringe_bitmaps[bt];
	 if (fb)

With this patch, the first loop only modifies the standard bitmaps
that were not defined by previous calls, for example by a previous
call to define-fringe-bitmap.  But the second loop still reinstalls
the standard bitmaps, although they will no longer be overwritten by
the first loop.  So why should the second loop again start from the
first bitmap, when we know that all the standard bitmaps are at that
point set up correctly: either by the standard values or by modified
user-defined values?

If you disagree with my analysis, please describe the sequence of
calls that would require the second loop to go over all the bitmaps
again, and please describe in detail the effect of those calls on the
fringe_bitmaps[] and standard_bitmaps[] arrays.  The sequence of calls
that I have in mind is as described in the original report of this
bug:

  . define-fringe-bitmap is called in .emacs to modify the standard
    bitmaps
  . .emacs is read by a --daemon session of Emacs
  . emacsclient opens a GUI frame, which calls gui_init_fringe

Thanks.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Gregory Heytings]

>
> Actually, I still think that either I misunderstand something, or 
> disagree with what you say.  So let me step back and explain what 
> bothers me.
>

Thanks for your detailed comments.  I will have another look at this next 
week.

bug#47832: 28.0.50; define-fringe-bitmap and emacs --daemon

[Eli Zaretskii]

> Date: Thu, 27 May 2021 07:32:25 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: larsi@gnus.org, 47832@debbugs.gnu.org
> 
> > Actually, I still think that either I misunderstand something, or 
> > disagree with what you say.  So let me step back and explain what 
> > bothers me.
> 
> Thanks for your detailed comments.  I will have another look at this next 
> week.

Sure, no rush.