From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#50660: 28.0.50; Text artifacting when the cursor moves over text under mouse face that originally displayed a box Date: Sat, 02 Oct 2021 11:43:53 +0300 Message-ID: <838rzbddfa.fsf@gnu.org> References: <87czp6ysw7.fsf.ref@yahoo.com> <831r5l5d6d.fsf@gnu.org> <87h7egy8jo.fsf@yahoo.com> <838rzs4i09.fsf@gnu.org> <87fsu06oxi.fsf@yahoo.com> <83ee9j3ju3.fsf@gnu.org> <834kaf3ets.fsf@gnu.org> <8335pz3dli.fsf@gnu.org> <87r1dj4q4m.fsf@yahoo.com> <83zgs71su7.fsf@gnu.org> <87k0jb4k5l.fsf@yahoo.com> <83sfxz1pw0.fsf@gnu.org> <87o88m14g0.fsf@yahoo.com> <83sfxyxeer.fsf@gnu.org> <87h7ee1252.fsf@yahoo.com> <83pmt2xcox.fsf@gnu.org> <87lf3myhlb.fsf@yahoo.com> <87a6jzvnpq.fsf@yahoo.com> <83o88fom1k.fsf@gnu.org> <871r5bvex8.fsf@yahoo.com> <83v92mkzft.fsf@gnu.org> <87k0j0rwo3.fsf@yahoo.com> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="38379"; mail-complaints-to="usenet@ciao.gmane.io" Cc: larsi@gnus.org, 50660@debbugs.gnu.org To: Po Lu Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Oct 02 10:45:20 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mWae0-0009la-0n for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 02 Oct 2021 10:45:20 +0200 Original-Received: from localhost ([::1]:58108 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mWady-0003n6-RX for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 02 Oct 2021 04:45:18 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:53490) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mWadi-0003md-9K for bug-gnu-emacs@gnu.org; Sat, 02 Oct 2021 04:45:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:46250) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mWadh-0004c0-VU for bug-gnu-emacs@gnu.org; Sat, 02 Oct 2021 04:45:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mWadh-0002hM-LL for bug-gnu-emacs@gnu.org; Sat, 02 Oct 2021 04:45:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 02 Oct 2021 08:45:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 50660 X-GNU-PR-Package: emacs Original-Received: via spool by 50660-submit@debbugs.gnu.org id=B50660.163316426210313 (code B ref 50660); Sat, 02 Oct 2021 08:45:01 +0000 Original-Received: (at 50660) by debbugs.gnu.org; 2 Oct 2021 08:44:22 +0000 Original-Received: from localhost ([127.0.0.1]:57796 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mWad4-0002gH-26 for submit@debbugs.gnu.org; Sat, 02 Oct 2021 04:44:22 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:48274) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mWad0-0002g2-Pz for 50660@debbugs.gnu.org; Sat, 02 Oct 2021 04:44:20 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:47792) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mWacu-0003vK-HN; Sat, 02 Oct 2021 04:44:13 -0400 Original-Received: from 84.94.185.95.cable.012.net.il ([84.94.185.95]:4280 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mWacs-0001C9-Fc; Sat, 02 Oct 2021 04:44:11 -0400 In-Reply-To: <87k0j0rwo3.fsf@yahoo.com> (message from Po Lu on Wed, 29 Sep 2021 09:35:24 +0800) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:216109 Archived-At: > From: Po Lu > Cc: larsi@gnus.org, 50660@debbugs.gnu.org > Date: Wed, 29 Sep 2021 09:35:24 +0800 > > Thanks, but I think I've already solved the problem. Can you try the > attached patch and see if there are any problems with it? TIA Thanks, this looks almost right to me, see the minor stylistic comments and questions below. (I didn't yet have time to try the patch, but if you did try it in all the relevant combinations, including R2L text, that's good enough for me.) > + if (cursor_in_mouse_face_p (w) > + && cursor_on_p) This could be on a single line. > +#ifdef HAVE_WINDOW_SYSTEM > + if (mouse_face_here_p) > + { > + get_cursor_offset_for_mouse_face (w, cursor_row, &mouse_delta); > + w->phys_cursor.x += mouse_delta; > + } > +#endif Please add a comment here explaining the problem and the general idea of the solution. > +#ifdef HAVE_WINDOW_SYSTEM > + if (MATRIX_ROW_VPOS (row, w->current_matrix) > + == w->phys_cursor.vpos && !w->pseudo_window_p > + && draw == DRAW_MOUSE_FACE) Style: we line up the logical && and || operators, like this: if ((MATRIX_ROW_VPOS (row, w->current_matrix) == w->phys_cursor.vpos) && !w->pseudo_window_p && draw == DRAW_MOUSE_FACE) > +#ifdef HAVE_WINDOW_SYSTEM > +/* Get the offset to apply before drawing phys_cursor, and return it > + in OFFSET, if ROW has something currently under mouse face. */ This comment doesn't tell the main part of the function's job, which is related to the box face. Please include that, and please explain in the comment why the box face requires the offset for the cursor. > + if (row->mode_line_p) > + return; This warrants a comment regarding the reason why the function returns in that case. > + for (; start != end; row->reversed_p ? > + --start : ++start) Style: this should not break the last part of the 'for'. Like this: for (; start != end; row->reversed_p ? --start : ++start) > + if (row->reversed_p && g->type == IMAGE_GLYPH) > + { > + struct image *img = IMAGE_FROM_ID (WINDOW_XFRAME (w), > + g->u.img_id); > + do_left_box_p = g->left_box_line_p && > + g->slice.img.x + g->slice.img.width == img->width; > + do_right_box_p = g->right_box_line_p && > + g->slice.img.x == 0; > + } > + else if (g->type == IMAGE_GLYPH) > + { > + struct image *img = IMAGE_FROM_ID (WINDOW_XFRAME (w), > + g->u.img_id); > + do_left_box_p = g->left_box_line_p && > + g->slice.img.x + g->slice.img.width == img->width; > + do_right_box_p = g->right_box_line_p && > + g->slice.img.x == 0; > + } It is better to have a two-level if here: if (g->type == IMAGE_GLYPH) { if (row->reversed_p) { ... } else { ... } But it looks like the code in both conditions is the same? More generally, what kind of problem specific to images does this part try to handle? > + if (do_left_box_p) > + *offset -= max (0, regular_face->box_vertical_line_width); > + /* Likewise with the right box line, as there may be a > + box there as well. */ > + if (do_right_box_p) > + *offset -= max (0, regular_face->box_vertical_line_width); There's no reason to use -= and += here. The callers never initialize the argument to anything but zero, nor should they. This function _calculates_ the offset, it doesn't _correct_ it. So a simple assignment should do better, because using the above begs the question: what could the initial value be? The callers should add or subtract the corrections as they see fit. Thanks.