bug#38181: Actual height of mode-line not taken into account

[Eli Zaretskii]

> Cc: jonas@bernoul.li, 38181@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Tue, 5 May 2020 18:57:06 +0200
> 
>  > So it sounds like we always were running like that.  Therefore, I must
>  > turn the table and ask you to please describe in more detail,
>  > preferably with Lisp snippets to try, how the fact that this flag is
>  > non-zero gets in the way of something we need to do with faces, both
>  > in this bug and the other one you mentioned.  I'd like to understand
>  > better how this flag interferes in these use cases.
> 
> With Bug#40639 and Bug#41071 the situation is simple: The internal
> border changed face but when we enter init_iterator
> inhibit_free_realized_faces is true and we don't handle it.

Can you show the backtrace from this call to init_iterator?  If it is
called directly or indirectly from redisplay_internal, the
inhibit_free_realized_faces flag should be reset to zero.  So I'm
guessing it is called in some other way.  That other way is not
necessarily the situation which should solve the delayed face
refreshing, since the corresponding parts of the frame will not be
redrawn until we enter redisplay_internal anyway, right?  Or am I
missing something.

>         if (face_change)
>           {
>             face_change = false;
>             XFRAME (w->frame)->face_change = 0;
>             free_all_realized_faces (Qnil);
>           }
>         else
>           {
>             XFRAME (w->frame)->face_change = 0;
>             free_all_realized_faces (w->frame);
>           }

You don't really need to free the faces everywhere here, only for the
window W's frame, right?  I think we shouldn't throw away face caches
of other frames in this situation, it's too radical.

> This takes any face change into consideration and works.  But it
> obviously disregards inhibit_free_realized_faces and so I'm not sure
> whether it's TRT (rather, I'm quite confident that it is not TRT).

I think the only situation where it can hurt is when this code is
called while redisplay_internal runs.  IOW, we need to test the value
of redisplaying_p.

> At the very least I probably should set windows_or_buffers_changed.
> There is this comment in redisplay_internal
> 
>    /* If face_change, init_iterator will free all realized faces, which
>       includes the faces referenced from current matrices.  So, we
>       can't reuse current matrices in this case.  */
>    if (face_change)
>      windows_or_buffers_changed = 47;
> 
> which hints at that (without considering that f->face_change is not
> handled there).

The call to free_all_realized_faces invalidates the current matrices
of all the windows on the frame, so the stale face IDs will not be
used after that on that frame, and therefore nothing else needs to be
done in that case.  If _all_ the face caches are discarded on all
frames, we need windows_or_buffers_changed to force redisplay_internal
consider more than just the selected frame.  So if you modify your
code to free faces only on the frame of the window for which you
compute the mode-line height, the problem with
windows_or_buffers_changed will disappear.

> Nevertheless, the fact that inhibit_free_realized_faces is true when
> entering the iterator after a face change is IMO a bug.  We apparently
> always run the iterator with the old faces first.  Only after we have
> (incidentally?) detected some change that forces us to "retry", we have
> redisplay set inhibit_free_realized_faces to false itself and the face
> change will get applied in the next iteration.  If so, the outcome of
> the previous iterations gets lost if a face changed there.  Does such a
> strategy give us any gains?

I don't think I follow.  redisplay_internal resets the
inhibit_free_realized_faces flag to zero near its beginning.  It is
true that we also reset it when we retry, but the first try doesn't
bypass this resetting.  Am I missing something?

> Again, the question I tried to ask earlier was: Does a current matrix in
> between two redisplays rely on the old realized faces?

Of course it does.  The glyph matrices don't hold any face
information, they only hold the ID of each face, and the ID is just
the index of the face's cache slot.  If the face cache is thrown away,
we will not be able to use the current matrix.

The current matrix is used for the following purposes:

  . outside of the redisplay cycle, for redisplaying portions of
    windows when we get expose events -- in this case we simply write
    to the glass the relevant portions of the matrix, without
    reproducing it

  . during a redisplay cycle, for decisions about redisplay
    optimizations

  . at the end of a redisplay cycle, for comparison with the desired
    matrix, which is needed for figuring out what to write to the
    glass

For the first purpose, we have the following defenses:

 . expose_frame:

  if (FRAME_FACE_CACHE (f) == NULL
      || FRAME_FACE_CACHE (f)->used < BASIC_FACE_ID_SENTINEL)
    {
      redisplay_trace ("expose_frame no faces\n");
      return;
    }

 . expose_window (called by expose_frame):

  if (w->current_matrix == NULL)
    return false;

So this case is covered, i.e. you can call free_all_realized_faces,
and the next expose event will be handled correctly.

The other two are the reason why we set the
inhibit_free_realized_faces in PRODUCE_GLYPHS: the flag must be set
during redisplay, until update_frame finished its job.  Otherwise we
will sooner or later crash.

So I think each function that might need to notice face changes as
soon as they happened, should be able to reset
inhibit_free_realized_faces, provided that (a) it makes sure
redisplaying_p is zero, and (b) it only does that if necessary and for
a single frame.  The latter part is because clearing the face cache
will cause all the move_it_* functions to work harder, because they
will have to recompute all the faces.

> If so, the answer is that inhibit_free_realized_faces should be
> always true but for the small window within retrying in
> redisplay_internal.

I don't think I agree, but maybe I'm missing something.

> In either case, it strikes me as a strange idea that redisplay_internal
> saves and restores the value of a variable which is apparently always
> true when it is entered (I suppose redisplay_internal is not re-entrant
> given
> 
>    /* I don't think this happens but let's be paranoid.  */
>    if (redisplaying_p)
>      return;

redisplay_internal is non-reentrant, but I see no harm in restoring
the value on exit.