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: Mon, 20 Sep 2021 08:19:16 +0300 Message-ID: <83ee9j3ju3.fsf@gnu.org> References: <87czp6ysw7.fsf.ref@yahoo.com> <87czp6ysw7.fsf@yahoo.com> <87y27uro4c.fsf@gnus.org> <877dfdz9ni.fsf@yahoo.com> <831r5l5d6d.fsf@gnu.org> <87h7egy8jo.fsf@yahoo.com> <838rzs4i09.fsf@gnu.org> <87fsu06oxi.fsf@yahoo.com> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="17924"; 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 Mon Sep 20 07:20:14 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 1mSBiw-0004Sd-06 for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 20 Sep 2021 07:20:14 +0200 Original-Received: from localhost ([::1]:43858 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mSBiu-0002Bw-5l for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 20 Sep 2021 01:20:12 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:49150) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mSBik-00029M-N1 for bug-gnu-emacs@gnu.org; Mon, 20 Sep 2021 01:20:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:57257) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mSBik-00029o-BR for bug-gnu-emacs@gnu.org; Mon, 20 Sep 2021 01:20:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mSBik-0007U0-7J for bug-gnu-emacs@gnu.org; Mon, 20 Sep 2021 01:20:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 20 Sep 2021 05:20:02 +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.163211517628710 (code B ref 50660); Mon, 20 Sep 2021 05:20:02 +0000 Original-Received: (at 50660) by debbugs.gnu.org; 20 Sep 2021 05:19:36 +0000 Original-Received: from localhost ([127.0.0.1]:40567 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mSBiK-0007T0-3u for submit@debbugs.gnu.org; Mon, 20 Sep 2021 01:19:36 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:60282) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mSBiG-0007Sj-Fd for 50660@debbugs.gnu.org; Mon, 20 Sep 2021 01:19:34 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:49030) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mSBiB-0001fx-2r; Mon, 20 Sep 2021 01:19:27 -0400 Original-Received: from 84.94.185.95.cable.012.net.il ([84.94.185.95]:2990 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 1mSBiA-000665-ME; Mon, 20 Sep 2021 01:19:27 -0400 In-Reply-To: <87fsu06oxi.fsf@yahoo.com> (message from Po Lu on Mon, 20 Sep 2021 09:00:57 +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:214813 Archived-At: > From: Po Lu > Cc: larsi@gnus.org, 50660@debbugs.gnu.org > Date: Mon, 20 Sep 2021 09:00:57 +0800 > > Eli Zaretskii writes: > > > I admit that I'm confused. I don't think I understand what did you > > find was the problem, and how it came into existence. Can you explain > > it in detail, step by step, with references to the current code on > > master? > > In set_cursor_from_row, the cursor's X position is calculated by summing > up the pixel_width of each glyph, from the start of the row (if not > reversed), or from the end (if reversed). > > Inside the produce_XXX_glyph series of functions, the widths of each box > line is added to the iterator's pixel_width (take this snippet from > produce_image_glyph) > > if (face->box != FACE_NO_BOX) > { > if (face->box_horizontal_line_width > 0) > { > if (slice.y == 0) > it->ascent += face->box_horizontal_line_width; > if (slice.y + slice.height == img->height) > it->descent += face->box_horizontal_line_width; > } > > if (face->box_vertical_line_width > 0) > { > if (it->start_of_box_run_p && slice.x == 0) > it->pixel_width += face->box_vertical_line_width; > if (it->end_of_box_run_p && slice.x + slice.width == img->width) > it->pixel_width += face->box_vertical_line_width; > } > } > > But when part of the row gets highlighted, it becomes drawn in the mouse > face, which might have greatly different box widths (or none at all) > compared to the face in which it was originally drawn. This causes > issues when moving the point (and thus the cursor) over the highlighted > area, because the glyph_width of the glyphs reflect the state before the > area was highlighted, and not after, and the cursor will be offset by > the original width of the box lines, which may be incorrect under mouse > face. > > For example, the face `custom-button' has a vertical line 2 pixels wide, > but `highlight' has no vertical line. If an area under the face > `custom-button' is becomes highlighted and under the face `highlight', > and the cursor is moved into the area, the cursor will be 2 pixels too > far towards the end of the row (assuming it is not reversed), and > potentially more, if there are more glyphs between the cursor and the > start/end of the highlighted area with left and/or right box lines! > > > You said the problem happens when one moves the mouse pointer over > > text whose face has a box. You said explicitly that clicking the > > mouse or dragging it over the text was not required. Did I understand > > you correctly? If I understood you correctly, then I don't think I > > see how drawing the cursor could be involved, because moving the mouse > > pointer without clicking doesn't move point, and thus the cursor > > doesn't need to be moved. > > Yes, but if you move the point into the highlighted area with other > editing commands, such as the arrow keys, the problem will manifest. > > >> Unfortunately, previously, this information would not be updated when > >> the the glyphs become displayed under mouse face > > > What information was not updated, exactly? > > The pixel_width of each glyph in the row that was highlighted and has > left_box_line_p, or right_box_line_p. Thanks, this becomes clearer now. However, it is IMO wrong to "fix" the glyphs' pixel_width to account for the box face. The glyphs didn't change, only their X-coordinate did. By changing the width, we are in effect lying to any code that accesses the glyphs. We should find another solution, perhaps similar to what the iterator does during the layout phase. > > When the text becomes highlighted, the face in effect is mouse-face, > > which usually doesn't have the box attribute, and so the text moves > > horizontally, which is expected. > > Yes, that isn't the problem I'm talking about. I still don't think I understand completely the problem you are talking about. Is the problematic recipe as below? . move the mouse pointer above text with box-face, so it is highlighted . move the text cursor into the highlighted text Are there any other problems, or is the above the only problematic situation you saw and intended to fix? > > In any case, I'm surprised that fixing such a minor issue needed so > > much code, including changes to data structures. Are you sure you > > used all the information we store in glyph_row and in each glyph? > > (Once I understand the problem better, I will be able to answer this > > question myself, but I'm not there yet.) > > I hope I have, but please tell me if I haven't. Why do you need the two new flags? If it's so you could avoid accounting for the box face too many times, isn't that a case of premature optimization? A loop through a glyph-row's glyphs is straightforward and runs very fast. The face of each glyph is stored in glyph->face_id, so you can easily see if its face includes the box attribute and get the box line thickness from that, and there are flags that tell you whether the box line is drawn on the left and on the right of the glyph. What else is missing? > >> @@ -17131,6 +17131,8 @@ set_cursor_from_row (struct window *w, struct glyph_row *row, > >> x = -1; > >> } > >> } > >> + if (row->have_glyph_with_box_p) > >> + x = -1; > > > > Here, I don't understand why the cursor position is affected, and I > > don't understand why you subtract the fixed value of 1 pixel. The box > > thickness doesn't have to be 1, and it could be positive or negative. > > If I understand correctly, setting x to -1 should force it to be > re-computed later, when the code enters compute_x. Is that not correct? Why did you need to recompute X in that case? why not fix the original computation instead? > >> + struct face *mouse_face = > >> + FACE_FROM_ID_OR_NULL (f, MOUSE_HL_INFO (f)->mouse_face_face_id); > >> + > >> + if (mouse_face == NULL) > >> + mouse_face = FACE_FROM_ID (f, MOUSE_FACE_ID); > > > When is this last NULL check actually needed? > > I'm not exactly sure, but a lot of code seems to do that. I see only a couple of places, and they are all on the level of xterm.c/w32term.c, which is in an entirely different layer of the display code. On the level of xdisp.c we only use mouse_face_face_id, AFAICT. > >> + /* If there's a row with a box somewhere, by all likelyhood > >> + the dimensions of the row has been changed. If that is > >> + the case, and we also find a row where the phys cursor > >> + is, recalculate the dimensions of the phys cursor. */ > > > I also don't understand this part. When is it needed and why? And > > why not handle it in display_and_set_cursor (if it isn't handled > > already)? > > I think display_and_set_cursor doesn't do any calculating. Here, what's > happening is that the X-offset of the phys cursor is being re-computed, > to compensate for any changes that might have happened to the > pixel_width of the glyphs. Doesn't it logically belong to the job of display_and_set_cursor? Thanks.