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

[martin rudalics]

>> Below find two backtraces with emacs -Q where PRODUCE_GLYPHS here sets
>> inhibit_free_realized_faces outside the scope of redisplay_internal
>> (that is, while redisplaying_p is nil) such that these will not get
>> caught by the unbind form in redisplay_internal.
>
> Thanks.  All of these enter redisplay via echo_area_display.  That
> calls redisplay_internal, but only after it displays the mini-window.
>
> However, I had my doubts regarding the accuracy of my mental model.
> Namely, the part where I said that inhibit_free_realized_faces should
> never be non-zero outside of redisplay.  So I looked at the code and
> its history, and it turns out I was wrong: the line that sets the flag
> in PRODUCE_GLYPHS was there since Emacs 21, and I see the flag set to
> non-zero all the way back to Emacs 22.1 (and probably earlier).

C-x v l told me it was

commit 18155a1d5a93cec23255becab15f26e77cace9e7
Author: Richard M. Stallman <rms@gnu.org>
Date:   Thu Aug 29 14:41:28 2002 +0000

    (PRODUCE_GLYPHS): Set inhibit_free_realized_faces
    when iterator is adding glyphs to a glyph matrix.

but I didn't dig any further.

> 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.  Redisplay
takes care of the face change for normal frames only when it evaluates
their titles and that strange incident is what I used to fix these bugs
on master.

The situation with the present bug is that I rewrote
'window-text-pixel-size' as

DEFUN ("window-text-pixel-size", Fwindow_text_pixel_size, 
Swindow_text_pixel_size, 0, 6, 0,
       doc: /* Return the size of the text of WINDOW's buffer in pixels.
WINDOW must be a live window and defaults to the selected one.  The
return value is a cons of the maximum pixel-width of any text line and
the maximum pixel-height of all text lines.

The optional argument FROM, if non-nil, specifies the first text
position and defaults to the minimum accessible position of the buffer.
If FROM is t, use the minimum accessible position that starts a
non-empty line.  TO, if non-nil, specifies the last text position and
defaults to the maximum accessible position of the buffer.  If TO is t,
use the maximum accessible position that ends a non-empty line.

The optional argument X-LIMIT, if non-nil, specifies the maximum text
width that can be returned.  X-LIMIT nil or omitted, means to use the
pixel-width of WINDOW's body; use this if you want to know how high
WINDOW should be become in order to fit all of its buffer's text with
the width of WINDOW unaltered.  Use the maximum width WINDOW may assume
if you intend to change WINDOW's width.  In any case, text whose
x-coordinate is beyond X-LIMIT is ignored.  Since calculating the width
of long lines can take some time, it's always a good idea to make this
argument as small as possible; in particular, if the buffer contains
long lines that shall be truncated anyway.

The optional argument Y-LIMIT, if non-nil, specifies the maximum text
height (excluding the height of the mode- or header-line, if any) that
can be returned.  Text lines whose y-coordinate is beyond Y-LIMIT are
ignored.  Since calculating the text height of a large buffer can take
some time, it makes sense to specify this argument if the size of the
buffer is large or unknown.

Optional argument MODE-AND-HEADER-LINE nil or omitted means do not
include the height of the mode- or header-line of WINDOW in the return
value.  If it is either the symbol `mode-line' or `header-line', include
only the height of that line, if present, in the return value.  If t,
include the height of both, if present, in the return value.  */)
  (Lisp_Object window, Lisp_Object from, Lisp_Object to, Lisp_Object x_limit,
   Lisp_Object y_limit, Lisp_Object mode_and_header_line)
{
  struct window *w = decode_live_window (window);
  Lisp_Object buffer = w->contents;
  struct buffer *b;
  struct it it;
  struct buffer *old_b = NULL;
  ptrdiff_t start, end, pos;
  struct text_pos startp;
  void *itdata = NULL;
  int c, max_x = 0, max_y = 0, x = 0, y = 0;

  CHECK_BUFFER (buffer);
  b = XBUFFER (buffer);

  if (b != current_buffer)
    {
      old_b = current_buffer;
      set_buffer_internal (b);
    }

  if (NILP (from))
    start = BEGV;
  else if (EQ (from, Qt))
    {
      start = pos = BEGV;
      while ((pos++ < ZV) && (c = FETCH_CHAR (pos))
             && (c == ' ' || c == '\t' || c == '\n' || c == '\r'))
        start = pos;
      while ((pos-- > BEGV) && (c = FETCH_CHAR (pos)) && (c == ' ' || c == 
'\t'))
        start = pos;
    }
  else
    start = clip_to_bounds (BEGV, fix_position (from), ZV);

  if (NILP (to))
    end = ZV;
  else if (EQ (to, Qt))
    {
      end = pos = ZV;
      while ((pos-- > BEGV) && (c = FETCH_CHAR (pos))
             && (c == ' ' || c == '\t' || c == '\n' || c == '\r'))
        end = pos;
      while ((pos++ < ZV) && (c = FETCH_CHAR (pos)) && (c == ' ' || c == '\t'))
        end = pos;
    }
  else
    end = clip_to_bounds (start, fix_position (to), ZV);

  if (!NILP (x_limit) && RANGED_FIXNUMP (0, x_limit, INT_MAX))
    max_x = XFIXNUM (x_limit);

  if (NILP (y_limit))
    max_y = INT_MAX;
  else if (RANGED_FIXNUMP (0, y_limit, INT_MAX))
    max_y = XFIXNUM (y_limit);

  /* Recompute faces if needed.  */
  if (face_change || XFRAME (w->frame)->face_change)
    {
      Lisp_Object window_header_line_format
        = window_parameter (w, Qheader_line_format);
      Lisp_Object window_tab_line_format
        = window_parameter (w, Qtab_line_format);
      Lisp_Object window_mode_line_format
        = window_parameter (w, Qmode_line_format);

      if (((EQ (mode_and_header_line, Qheader_line)
            || EQ (mode_and_header_line, Qt))
           && !EQ (window_header_line_format, Qnone)
           && (!NILP (window_header_line_format)
               || !NILP (BVAR (current_buffer, header_line_format))))
          || ((EQ (mode_and_header_line, Qtab_line)
               || EQ (mode_and_header_line, Qt))
              && !EQ (window_tab_line_format, Qnone)
              && (!NILP (window_tab_line_format)
                  || !NILP (BVAR (current_buffer, tab_line_format))))
          || ((EQ (mode_and_header_line, Qmode_line)
               || EQ (mode_and_header_line, Qt))
              && !EQ (window_mode_line_format, Qnone)
              && (!NILP (window_mode_line_format)
                  || !NILP (BVAR (current_buffer, mode_line_format)))))
        {
          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);
            }
        }
    }

  itdata = bidi_shelve_cache ();
  SET_TEXT_POS (startp, start, CHAR_TO_BYTE (start));
  start_display (&it, w, startp);
  /* It makes no sense to measure dimensions of region of text that
     crosses the point where bidi reordering changes scan direction.
     By using unidirectional movement here we at least support the use
     case of measuring regions of text that have a uniformly R2L
     directionality, and regions that begin and end in text of the
     same directionality.  */
  it.bidi_p = false;

  int move_op = MOVE_TO_POS | MOVE_TO_Y;
  int to_x = -1;
  if (!NILP (x_limit))
    {
      it.last_visible_x = max_x;
      /* Actually, we never want move_it_to stop at to_x.  But to make
         sure that move_it_in_display_line_to always moves far enough,
         we set to_x to INT_MAX and specify MOVE_TO_X.  */
      move_op |= MOVE_TO_X;
      to_x = INT_MAX;
    }

  void *it2data = NULL;
  struct it it2;
  SAVE_IT (it2, it, it2data);

  x = move_it_to (&it, end, to_x, max_y, -1, move_op);

  /* We could have a display property at END, in which case asking
     move_it_to to stop at END will overshoot and stop at position
     after END.  So we try again, stopping before END, and account for
     the width of the last buffer position manually.  */
  if (IT_CHARPOS (it) > end)
    {
      end--;
      RESTORE_IT (&it, &it2, it2data);
      x = move_it_to (&it, end, to_x, max_y, -1, move_op);
      /* Add the width of the thing at TO, but only if we didn't
         overshoot it; if we did, it is already accounted for.  Also,
         account for the height of the thing at TO.  */
      if (IT_CHARPOS (it) == end)
        {
          x += it.pixel_width;
          it.max_ascent = max (it.max_ascent, it.ascent);
          it.max_descent = max (it.max_descent, it.descent);
        }
    }
  if (!NILP (x_limit))
    {
      /* Don't return more than X-LIMIT.  */
      if (x > max_x)
        x = max_x;
    }

  /* Subtract height of header-line which was counted automatically by
     start_display.  */
  y = it.current_y + it.max_ascent + it.max_descent
    - WINDOW_TAB_LINE_HEIGHT (w) - WINDOW_HEADER_LINE_HEIGHT (w);
  /* Don't return more than Y-LIMIT.  */
  if (y > max_y)
    y = max_y;

  if (EQ (mode_and_header_line, Qtab_line)
      || EQ (mode_and_header_line, Qt))
    {
      Lisp_Object window_tab_line_format
        = window_parameter (w, Qtab_line_format);

      if (!EQ (window_tab_line_format, Qnone)
          && (!NILP (window_tab_line_format)
              || !NILP (BVAR (current_buffer, tab_line_format))))
        /* Re-add height of tab-line as requested.  */
        y = y + display_mode_line (w, TAB_LINE_FACE_ID,
                                   NILP (window_tab_line_format)
                                   ? BVAR (current_buffer, tab_line_format)
                                   : window_tab_line_format);
    }

  if (EQ (mode_and_header_line, Qheader_line)
      || EQ (mode_and_header_line, Qt))
    {
      Lisp_Object window_header_line_format
        = window_parameter (w, Qheader_line_format);

      if (!EQ (window_header_line_format, Qnone)
          && (!NILP (window_header_line_format)
              || !NILP (BVAR (current_buffer, header_line_format))))
        /* Re-add height of header-line as requested.  */
        y = y + display_mode_line (w, HEADER_LINE_FACE_ID,
                                   NILP (window_header_line_format)
                                   ? BVAR (current_buffer, header_line_format)
                                   : window_header_line_format);
    }

  if (EQ (mode_and_header_line, Qmode_line)
      || EQ (mode_and_header_line, Qt))
    {
      Lisp_Object window_mode_line_format
        = window_parameter (w, Qmode_line_format);

      if (!EQ (window_mode_line_format, Qnone)
          && (!NILP (window_mode_line_format)
              || !NILP (BVAR (current_buffer, mode_line_format))))
        /* Add height of mode-line as requested.  */
        y = y + display_mode_line (w, CURRENT_MODE_LINE_FACE_ID (w),
                                   NILP (window_mode_line_format)
                                   ? BVAR (current_buffer, mode_line_format)
                                   : window_mode_line_format);
    }

  bidi_unshelve_cache (itdata, false);

  if (old_b)
    set_buffer_internal (old_b);

  return Fcons (make_fixnum (x), make_fixnum (y));
}

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

> Thanks (and sorry for spreading misinformation: I was somehow
> confident that it was myself who added the setting of
> inhibit_free_realized_faces in PRODUCE_GLYPHS, but the truth is that
> it was Gerd, long ago).

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?

Again, the question I tried to ask earlier was: Does a current matrix in
between two redisplays rely on the old realized 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.  And maybe
somewhere at the beginning of redisplay when a face change occurred for
the window's frame and we clear all matrices therefore and avoid any
redisplay optimizations.

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;

) but maybe there are exceptions I don't know.

martin