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

[Eli Zaretskii <eliz@gnu.org>]

> 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.