From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Ergus Newsgroups: gmane.emacs.devel Subject: Re: Question about display engine Date: Sun, 29 Sep 2019 12:30:34 +0200 Message-ID: <20190929103034.jw5qolfjtbjkbk5p@Ergus> References: <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> <83zhiohfnx.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="56064"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: NeoMutt/20180716 Cc: rudalics@gmx.at, emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun Sep 29 12:31:10 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 1iEWTt-000EVO-JV for ged-emacs-devel@m.gmane.org; Sun, 29 Sep 2019 12:31:10 +0200 Original-Received: from localhost ([::1]:37776 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iEWTs-0004a2-DU for ged-emacs-devel@m.gmane.org; Sun, 29 Sep 2019 06:31:08 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:40422) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iEWTh-0004ZX-5h for emacs-devel@gnu.org; Sun, 29 Sep 2019 06:30:59 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iEWTe-0001o6-0W for emacs-devel@gnu.org; Sun, 29 Sep 2019 06:30:55 -0400 Original-Received: from sonic314-19.consmr.mail.ir2.yahoo.com ([77.238.177.145]:46261) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1iEWTd-0001lo-Ng for emacs-devel@gnu.org; Sun, 29 Sep 2019 06:30:53 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=aol.com; s=a2048; t=1569753051; bh=oX832v2Axhz3uZNSKiG8DHqGSWrOxJoe6DCGarM5qt4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From:Subject; b=LVdzZApXuTNi4vZlTbPWlbL2QnyRSgVLMnnd3pG0x1S7rnuRsJ1K6+H5LDvE+v1+RJ0FhFcWYp9BtrLJSe0UOJV/ztOEWzUwLYNcopOHmv6D9a7e7lRdsfg1USYwBpGOUQoFDp+xKnT5O0RS6FkiCGof0V8CNj55Z94DChmSk1Z10D5u4faOioDeQorEJU3VwEjgmCMOno+NIu4/qI0LizSUM/0MNd2sO+IoUSAXC2fxQAYKdeR/nPWkJrFFWh4ss2FbJqce0O3eA9k5Wa+CHYmLqmww46VV3cUb6a9kjN/mcP4DYlSOsVQa4qKmrvJ+NcZi2oXdN2VE43Gfez/dFw== X-YMail-OSG: piOPELEVM1kqGUwilN1D3gLHN3J0q9cZ3hj4Y0bP2HGAZA8u9x7rEauZR.krqtq DPlcy5ejBW3WizqItTqOzftzBhygF2p.OFAwqbsNth51tff9hf9vLjehvWp3SLSda5si7SgFpb5g H_fNaOH9W_M6f5i1k0ek4fbp013sa1MfU7beSfnLQggadvtl7fPlssNayM4cR2dSjrA4A15ZzLux S2cYeMuu0Cl1hFnI.rfC5g0UTHqIQct13biPXzBTS9P1wCWgAE3RqZLQ_wkRyDcHR.5feeITT4e4 _Q.YmbP8gC46KSko5qtip7.LqBS5dAtgJ2mRxzamEGyMbXNMKF5iHvZc42tIeJrh4ztxX42cfMJ1 6SVEbu18SM3tDGVDZH.1fuhrowT7Zi1dyUQrDt1jcP9HqKU3ll0RO6aabUMn9HhXWMLOv3T9Q9c. PQwMYn_NgrUbscA1lM0zNr_aYTx3Ip2Pp2bJGjMIFFW3tdCuJIi38xfxRapxD3MYxYAS76SfXrOL 4K0crkTIyJkwhgYpaqBp9NDDUCcp6YzKTmK0DWBK5hypPT6lhg1zNRi2_LsyIsLnBLVUr3J8cgx2 vEZBGbK.orCXVmT13VreWWxOsc.6hdfkqxq9HcOqXB5TsBZ4x4V6zpmlLiJp336lTPGqFvVP8JMV XNDyOBdL814Bxeb3GB.DKWj_z80p69WCbCuhvVeaAYq7SX4sYjCe.Xz9J25bDbOMv8LPTq0YZEIN zNAEuVKTnbJRpQ63fPXVTO30HOF4PZhvnk6F1xSlDlFxCCzBEJrgbfjwHl7R8aYMHgUTjrFub1q_ kL2kuSn23X6rULSurxiBtrQgtIKgfTrquskebBrgr0 Original-Received: from sonic.gate.mail.ne1.yahoo.com by sonic314.consmr.mail.ir2.yahoo.com with HTTP; Sun, 29 Sep 2019 10:30:51 +0000 Original-Received: by smtp428.mail.ir2.yahoo.com (Oath Hermes SMTP Server) with ESMTPA ID f791c96af543cf608205514d901f850f; Sun, 29 Sep 2019 10:30:45 +0000 (UTC) Content-Disposition: inline In-Reply-To: <83zhiohfnx.fsf@gnu.org> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 77.238.177.145 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:240386 Archived-At: 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 > 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); > 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. > Fixed > > . 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); > + > 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 > 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; > Fixed. > . 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 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); > 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. */ > > 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) > 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 >responses. Thank to you.