[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Question about display engine

From: Ergus
Subject: Re: Question about display engine
Date: Sun, 29 Sep 2019 12:30:34 +0200
User-agent: NeoMutt/20180716

On Sat, Sep 28, 2019 at 01:35:30PM +0300, Eli Zaretskii wrote:
Hi Eli:

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?

Use double variable to describe the same is an error prone
practice. This change simplifies the checks in all the code related (as
there will be only one variable to set/check), reduces one name and
member in the struct and avoids errors related to changing one value and
not the other.

. In this hunk from append_space_for_newline, why did you change

    -         int local_default_face_id =
    +      int char_width = 1;
    +      if (default_face_p
    +          || FRAME_WINDOW_P (it->f)
    +         )
    +       {
    +         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);

The call is made after a lookup_basic_face and it's for DEFAULT_FACE_ID
is it needed the check? Normally for other faces if it is NULL then we
use the DEFAULT_FACE_ID. In this case it should be unneeded right?

. 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, 
    -                 && !INT_ADD_WRAPV (it->lnum_pixel_width, 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);

The comment is now in the function fill_column_indicator_column where
the calculation is performed now.

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

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

For me this looked better (shorter and clearer), but I will restore it
if you prefer that.

. This also looks redundant:

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

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

The line was too long. I just tried to reduce it a bit.

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

Fixed both.

. This hunk looks redundant:

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

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

The line was too long. 85 chars long.

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

It seems unneeded two lines for this; but fixed.
Thanks again for working on this, and sorry for my unusually slow

Thank to you.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]