bug#11068: 24.0.94; Face-remapped background does not extend to end of window

[Eli Zaretskii]

> From: Chong Yidong <cyd@gnu.org>
> Cc: darthandrus@gmail.com,  monnier@iro.umontreal.ca,  11068@debbugs.gnu.org
> Date: Sun, 25 Mar 2012 11:01:17 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> This looks a bit strange.  Why is DEFAULT_FACE_ID handled specially
> >> here?
> >
> > In my testing, I didn't see the need to do it even for the default
> > face, because it->face_id is already set to the ID of the remapped
> > face.  So it's just me being paranoiac.
> 
> I'd just leave that bit out.

OK.

> Also, in this hunk
> 
> @@ -18173,7 +18173,12 @@
>         it->len = 1;
>  
>         if (default_face_p)
> -         it->face_id = DEFAULT_FACE_ID;
> +         {
> +           if (NILP (Vface_remapping_alist))
> +             it->face_id = DEFAULT_FACE_ID;
> +           else
> +             it->face_id = lookup_basic_face (it->f, DEFAULT_FACE_ID);
> +         }
>         else if (it->face_before_selective_p)
>           it->face_id = it->saved_face_id;
>         face = FACE_FROM_ID (it->f, it->face_id);
> 
> You should call lookup_basic_face without the surrounding if statement,
> because lookup_basic_face returns its second arg immediately if
> face-remapping-alist is nil, making the extra check redundant.

Right.

> And here
> 
> +       if (row->reversed_p
> +           || lookup_basic_face (it->f, DEFAULT_FACE_ID) != DEFAULT_FACE_ID)
> 
> shouldn't you just compare default_face->id to DEFAULT_FACE_ID instead
> of making another lookup_basic_face call?

Here I disagree: I think we should confine the internal details of
face remapping to lookup_basic_face, instead of spilling our knowledge
about that all over.  As you just said above, if face-remapping-alist
is nil, that function returns immediately, so there's nothing lost
here for the "usual" use-case.

> With these changes, the patch seems pretty safe; even if something
> screws up, it will only affect how remapped faces extend to the end of
> the buffer, which was broken before anyway.  So feel free to commit.

OK, but to be absolutely fair, I must point out that, while the patch
is very simple, it has one non-trivial consequence: when the default
face _is_ remapped, the empty space to the right of any L2R line and
the empty lines beyond EOB are filled with stretch glyphs of a
suitably computed width.  Before the patch, there were no glyphs in
those places.  So, from the POV of the content of the glyph matrices,
the change introduced by these patches is quite prominent.  In
particular, this could potentially affect any code that examines the
glyphs of a glyph matrix; previously, such stretch glyphs were only
present at the left side of R2L glyph rows.