Re: Question about display engine

[Eli Zaretskii]

> Date: Thu, 26 Sep 2019 18:32:04 +0200
> From: Ergus <address@hidden>
> Cc: address@hidden, address@hidden
> 
> Please check the branch scratch/extend_face_id and tell me what's
> missing to merge in master.

Thanks, I took a look.  I think the branch is almost ready to be
merged, once we address the following minor comments:

 . I don't understand why you changed the underlined_p and
   underline_type stuff in struct face.  What were the reasons for
   that part of the changeset?  Its sole effect seems to be to
   generate some redundant changes, or am I missing something?

 . In this hunk from append_space_for_newline, why did you change
   FACE_FROM_ID_OR_NULL to FACE_FROM_ID?

     -         int local_default_face_id =
     +      int char_width = 1;
     +
     +      if (default_face_p
     +#ifdef HAVE_WINDOW_SYSTEM
     +          || FRAME_WINDOW_P (it->f)
     +#endif
     +         )
     +       {
     +         const int local_default_face_id =
                 lookup_basic_face (it->w, it->f, DEFAULT_FACE_ID);
               struct face* default_face =
     -           FACE_FROM_ID_OR_NULL (it->f, local_default_face_id);
     +           FACE_FROM_ID (it->f, local_default_face_id);

 . Please use comments /* in this style */, not // like this.

 . In this hunk from extend_face_to_end_of_line you lost the
   comment.  Is that comment no longer correct?

     -             int column_x;

     -             if (!INT_MULTIPLY_WRAPV (indicator_column, char_width, 
&column_x)
     -                 && !INT_ADD_WRAPV (it->lnum_pixel_width, column_x, 
&column_x)
     -                 && column_x >= it->current_x
     -                 && column_x <= it->last_visible_x)
     -               {
     +         const int indicator_column =
     +           fill_column_indicator_column (it, char_width);
     +
               const char saved_char = it->char_to_display;
               const struct text_pos saved_pos = it->position;
               const bool saved_avoid_cursor = it->avoid_cursor_p;
     -                 const int saved_face_id = it->face_id;
               const bool saved_box_start = it->start_of_box_run_p;
               Lisp_Object save_object = it->object;
     +         const int saved_face_id = it->face_id;

     -                 /* The stretch width needs to considet the latter
     -                    added glyph.  */
     -                 const int stretch_width
     -                   = column_x - it->current_x - char_width;
     -
     -                 memset (&it->position, 0, sizeof it->position);
     +         it->face_id = extend_face_id;
               it->avoid_cursor_p = true;
               it->object = Qnil;

     +         if (indicator_column >= 0
     +             && indicator_column > it->current_x
     +             && indicator_column < it->last_visible_x)
     +            {
     +
     +             const int stretch_width =
     +               indicator_column - it->current_x - char_width;
     +
     +             memset (&it->position, 0, sizeof it->position);
     +

 . And here the comment was not moved with the code which it
   describes:

               /* Restore the face after the indicator was generated.  */
     -                 it->face_id = saved_face_id;

               /* If there is space after the indicator generate an
                  extra empty glyph to restore the face.  Issue was
     @@ -20634,8 +20633,8 @@ extend_face_to_end_of_line (struct it *it)
               it->avoid_cursor_p = saved_avoid_cursor;
               it->start_of_box_run_p = saved_box_start;
               it->object = save_object;
     -               }
     -            }
     +         it->face_id = saved_face_id;

 . This hunk looks redundant to me:

     @@ -20681,10 +20680,9 @@ extend_face_to_end_of_line (struct it *it)
                   /* The last row's stretch glyph should get the default
                      face, to avoid painting the rest of the window with
                      the region face, if the region ends at ZV.  */
     -             if (it->glyph_row->ends_at_zv_p)
     -               it->face_id = default_face->id;
     -             else
     -               it->face_id = face->id;
     +             it->face_id = (it->glyph_row->ends_at_zv_p ?
     +                            default_face->id : face->id);

   (there's at least one more like it in the changeset).

 . This also looks redundant:

     @@ -25436,8 +25423,8 @@ display_string (const char *string, Lisp_Object 
lisp_str

        /* Initialize the iterator IT for iteration over STRING beginning
           with index START.  */
     -  reseat_to_string (it, NILP (lisp_string) ? string : NULL, lisp_string, 
start,
     -                   precision, field_width, multibyte);
     +  reseat_to_string (it, NILP (lisp_string) ? string : NULL, lisp_string,
     +                    start, precision, field_width, multibyte);

 . In this commentary, please upcase attr_filter, as that is our
   convention for describing function arguments in comments:

     @@ -2269,6 +2282,11 @@ filter_face_ref (Lisp_Object face_ref,
         of ERR_MSGS).  Use NAMED_MERGE_POINTS to detect loops in face
         inheritance or list structure; it may be 0 for most callers.

     +   attr_filter is the index of a parameter that conditions the merging
     +   for named faces (case 1) to only the face_ref where
     +   lface[merge_face_ref] is non-nil. To merge unconditionally set this
     +   value to 0.

 . Likewise here:

     @@ -5988,6 +6039,8 @@ compute_char_face (struct frame *f, int ch, 
Lisp_Object pr
         which a different face is needed, as far as text properties and
         overlays are concerned.  W is a window displaying current_buffer.

     +   attr_filter is passed merge_face_ref.

   The sentence you added sounds incomplete here: did you mean to say
   "passed to merge_face_ref"?

 . This hunk looks redundant:

     @@ -4505,7 +4552,8 @@ lookup_face (struct frame *f, Lisp_Object *attr)
         suitable face is found, realize a new one.  */

      int
     -face_for_font (struct frame *f, Lisp_Object font_object, struct face 
*base_face
     +face_for_font (struct frame *f, Lisp_Object font_object,
     +               struct face *base_face)

 . This also looks redundant:

     @@ -6068,19 +6122,18 @@ face_at_buffer_position (struct window *w, 
ptrdiff_t pos
        }

        /* Optimize common cases where we can use the default face.  */
     -  if (noverlays == 0
     -      && NILP (prop))
     +  if (noverlays == 0 && NILP (prop))

 . And this one:

        /* Begin with attributes from the default face.  */
     -  memcpy (attrs, default_face->lface, sizeof attrs);
     +  memcpy (attrs, default_face->lface, sizeof(attrs));

 . Finally, please write some short description for NEWS, as this
   feature definitely needs to be mentioned there.

Thanks again for working on this, and sorry for my unusually slow
responses.