From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: Question about display engine Date: Sat, 28 Sep 2019 13:35:30 +0300 Message-ID: <83zhiohfnx.fsf@gnu.org> References: <20190908005109.s7hhcczkrcbzewdc@Ergus> <83imq24qx3.fsf@gnu.org> <20190908182346.hheaveun2pw5usb6@Ergus> <20190914204207.gfyvgbb7t4ztya7a@Ergus> <83ftkxy3r7.fsf@gnu.org> <20190915214233.xkjtoxyfxkyrd2id@Ergus> <20190917021725.xxhhhxcz3nr6sb7z@Ergus> <83blvjw8x9.fsf@gnu.org> <83v9tmqcv7.fsf@gnu.org> <20190921215551.ruu6ji6sjpxydpng@Ergus> <20190926163204.gdb4gxbjtdbysk3y@Ergus> Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="59569"; mail-complaints-to="usenet@blaine.gmane.org" Cc: rudalics@gmx.at, emacs-devel@gnu.org To: Ergus Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Sep 28 12:35:58 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1iEA4x-000FKK-Gf for ged-emacs-devel@m.gmane.org; Sat, 28 Sep 2019 12:35:55 +0200 Original-Received: from localhost ([::1]:60224 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iEA4w-0008SD-CD for ged-emacs-devel@m.gmane.org; Sat, 28 Sep 2019 06:35:54 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:53972) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iEA4j-0008PU-HT for emacs-devel@gnu.org; Sat, 28 Sep 2019 06:35:43 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:60837) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1iEA4i-0001LW-5Z; Sat, 28 Sep 2019 06:35:40 -0400 Original-Received: from [176.228.60.248] (port=4579 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1iEA4h-0007DI-9X; Sat, 28 Sep 2019 06:35:39 -0400 In-reply-to: <20190926163204.gdb4gxbjtdbysk3y@Ergus> (message from Ergus on Thu, 26 Sep 2019 18:32:04 +0200) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:240366 Archived-At: > Date: Thu, 26 Sep 2019 18:32:04 +0200 > From: Ergus > Cc: rudalics@gmx.at, emacs-devel@gnu.org > > 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.