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.devel Subject: Re: move_it_vertically_backward question Date: Sat, 18 Dec 2021 12:28:12 +0200 Message-ID: <83pmpub41v.fsf@gnu.org> References: <87lf0pw78r.fsf.ref@yahoo.com> <87lf0pw78r.fsf@yahoo.com> <837dc8mue3.fsf@gnu.org> <874k7cuhv4.fsf@yahoo.com> <83lf0nl56t.fsf@gnu.org> <875yrrtiwj.fsf@yahoo.com> <837dc7l2pa.fsf@gnu.org> <87ilvqty24.fsf@yahoo.com> <8335muj8zk.fsf@gnu.org> <87h7bang3d.fsf@yahoo.com> <83mtl1j527.fsf@gnu.org> <87zgp1mldh.fsf@yahoo.com> <83tuf9gdg5.fsf@gnu.org> <87pmpwkikp.fsf@yahoo.com> <83mtl0hnm5.fsf@gnu.org> <87czlwkfpk.fsf@yahoo.com> <8735mskal2.fsf@yahoo.com> <83ee6che8h.fsf@gnu.org> <87y24kisgf.fsf@yahoo.com> <838rwkhcqb.fsf@gnu.org> <87r1achulq.fsf@yahoo.com> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="18297"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Po Lu Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sat Dec 18 11:29:36 2021 Return-path: Envelope-to: ged-emacs-devel@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 1myWy7-0004bQ-AT for ged-emacs-devel@m.gmane-mx.org; Sat, 18 Dec 2021 11:29:35 +0100 Original-Received: from localhost ([::1]:45804 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1myWy5-0007u1-Ml for ged-emacs-devel@m.gmane-mx.org; Sat, 18 Dec 2021 05:29:33 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:55168) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1myWx8-0007Cm-Op for emacs-devel@gnu.org; Sat, 18 Dec 2021 05:28:35 -0500 Original-Received: from [2001:470:142:3::e] (port=46564 helo=fencepost.gnu.org) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1myWx8-00046y-G4; Sat, 18 Dec 2021 05:28:34 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=0c+uRFp9MXlKQOQUxJbcCHb8M2L/Z2BfidyM1mkwE4o=; b=D+BEpiCCYZjg 1hMnGuQLsRsx98esJP94fVkilZ/jFwDrqwQAP9EP8EUvw8TvzMz76d4q/Ge3dIzfuvThPSZeS3b3W l/DStqYKiOd8pOtqaKzfZONJi//GJ3qIdmv/mYdWWxO13X7eQdwFpeUM2lZKEm2eN16jFBiZj5ErW rjHVsIa5Qe40L7Kxw971xhy5iZ3X+sMNHGai64umU7nGC1+KBCZoRnyjX7GLk0lbSzD3kU4HxpS3h /SZLiCkvzqdNHwk/RReuDekbtEj4DwUvdjLcQUBhs0cPKWbOAVYpbYPyex0LjqZEzq6ksHhF3wjKb TwOPzgv/oouhsrvRP5ECrQ==; Original-Received: from [87.69.77.57] (port=1962 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 1myWwz-0005MJ-6d; Sat, 18 Dec 2021 05:28:30 -0500 In-Reply-To: <87r1achulq.fsf@yahoo.com> (message from Po Lu on Fri, 17 Dec 2021 09:45:53 +0800) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 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-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:282275 Archived-At: > From: Po Lu > Cc: emacs-devel@gnu.org > Date: Fri, 17 Dec 2021 09:45:53 +0800 > > > The difference is that window-text-pixel-size already exists, we just > > extend it for another use case. > > > Well, then this is equivalent to telling window-text-pixel-size to > > stop at the visual left edge of a screen line, right? That is, > > interpret TO not as a literal buffer position, but as the visual > > beginning of the line which shows position TO. > > Yes, I think the two meanings are equivalent. I documented it as the > former, but it wouldn't hurt to document it like this instead. I think it would be a lot easier to understand if we use the description I proposed. Lisp programmers do not necessarily know what are ascent and descent, and what they have to do with this function's operation. However, I don't think I see where that additional argument causes the function to stop at the visual left edge of a screen line containing TO. What did I miss? > Anyway, here are two patches that implement the extensions to > `window-text-pixel-size', along with the code in > pixel-scroll-precision-mode that makes use of them. Please for the next iteration show them as a single patch. One of them seems to depend on the other, and it's hard to review the changes piecemeal. > I tried to update the documentation, but to be frank I'm not good at > writing documentation and couldn't word it very well Don't give up trying! No one is born with these abilities, they are acquired by writing documentation and learning from that. For now, in my comments below I focused on the code, and left the documentation for later, when we arrive at the final version of the code. > @@ -10969,8 +10969,19 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to, Li > if (IT_CHARPOS (it) == end) > { > x += it.pixel_width; > - it.max_ascent = max (it.max_ascent, it.ascent); > - it.max_descent = max (it.max_descent, it.descent); > + > + /* DTRT if no_ascents_descents is t. */ > + if (no_ascents_descents) > + { > + saw_display_prop_at_end_p = true; This bool value seems to be a misnomer: we don't really have evidence that we found a display property at end of text. Also, you should use NILP in the if-clause, as no_ascents_descents is a Lisp object. > @@ -10991,8 +11002,14 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to, Li > > /* Subtract height of header-line and tab-line which was counted > automatically by start_display. */ > - y = it.current_y + it.max_ascent + it.max_descent > - - WINDOW_TAB_LINE_HEIGHT (w) - WINDOW_HEADER_LINE_HEIGHT (w); > + y = (it.current_y + (NILP (no_ascents_descents) > + ? it.max_ascent + it.max_descent > + : 0) > + - WINDOW_TAB_LINE_HEIGHT (w) - WINDOW_HEADER_LINE_HEIGHT (w)); > + > + if (saw_display_prop_at_end_p) > + y += doff; This is quite confusing: you actually add the same value, but in two different ways and with 2 different (but subtly equivalent) conditions. It would be good to make this less confusing. > @@ -10866,6 +10867,13 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to, Li > break; > } > } > + else if (CONSP (from)) > + { > + start = clip_to_bounds (BEGV, fix_position (XCAR (from)), ZV); > + bpos = CHAR_TO_BYTE (start); > + CHECK_FIXNAT (XCDR (from)); > + movement = XFIXNAT (XCDR (from)); > + } > else > { > start = clip_to_bounds (BEGV, fix_position (from), ZV); There's code duplication here, which it would be good to avoid: the calculation of bpos and the call to clip_to_bounds in calculation of start. The name 'movement' is not the best name. I would suggest 'offset' or maybe even 'y_offset' instead. Also, the assumption that this offset is always positive, and that it means the position _before_ FROM, is also something that makes the function less general than it could be. It would be better to have positive values mean _after_ FROM and negative values to mean before it. Then you could use move_it_vertically instead of move_it_vertically_backward. > + if (movement > 0) > + { > + int last_y; > + it.current_y = 0; > + > + move_it_by_lines (&it, 0); ^^^^^^^^^^^^^^^^ Why is this needed? > + while (-it.current_y < movement) > + { > + last_y = it.current_y; > + move_it_vertically_backward (&it, movement + it.current_y); > + > + if (it.current_y == last_y) > + break; > + } > + > + it.current_y = 0; > + start = clip_to_bounds (BEGV, IT_CHARPOS (it), ZV); > + } > + > int start_y = it.current_y; > /* It makes no sense to measure dimensions of region of text that > crosses the point where bidi reordering changes scan direction. The code that follows this, which you leave intact, will reseat the iterator to the beginning of the visual line where you ended up after the loop that goes backward. Is that really TRT? what if there's a multi-line display or overlay string at that place? Did you test the code in that case? AFAIU, you have already assured that you are at the beginning of a visual line, so the reseat, and the following return to the START position, might not be needed. When you return to START, you may end up very far from where the loop going backward ended, if START is "covered" by a display property or overlay string. > @@ -11053,26 +11085,32 @@ window_text_pixel_size (Lisp_Object window, Lisp_Object from, Lisp_Object to, Li > > bidi_unshelve_cache (itdata, false); > > - return Fcons (make_fixnum (x - start_x), make_fixnum (y)); > + return (!movement > + ? Fcons (make_fixnum (x - start_x), make_fixnum (y)) > + : list3i (x - start_x, y, start)); Why not use list2i instead of Fcons there? Do these changes solve the performance problem in pixel-scroll-precision-mode? Thanks for working.