From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: Line wrap reconsidered Date: Wed, 27 May 2020 20:29:54 +0300 Message-ID: <83sgfls2ul.fsf@gnu.org> References: <92FF4412-04FB-4521-B6CE-52B08526E4E5@gmail.com> <878shfsq35.fsf@gnus.org> <83imgivjak.fsf@gnu.org> <83lfletr03.fsf@gnu.org> <4895C6EE-5E1F-44BF-93C1-CC5F7C096F73@gmail.com> <9766BA3D-B8F9-456B-9F59-60D21B86E390@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="2707"; mail-complaints-to="usenet@ciao.gmane.io" Cc: larsi@gnus.org, emacs-devel@gnu.org To: Yuan Fu Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Wed May 27 19:30:48 2020 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 1jdzt6-0000VV-3W for ged-emacs-devel@m.gmane-mx.org; Wed, 27 May 2020 19:30:44 +0200 Original-Received: from localhost ([::1]:54910 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jdzt5-0003jt-42 for ged-emacs-devel@m.gmane-mx.org; Wed, 27 May 2020 13:30:43 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:38602) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jdzsX-0003Hh-4l for emacs-devel@gnu.org; Wed, 27 May 2020 13:30:09 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:41134) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jdzsW-0006Az-HW; Wed, 27 May 2020 13:30:08 -0400 Original-Received: from [176.228.60.248] (port=2888 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jdzsT-0005Ai-BR; Wed, 27 May 2020 13:30:07 -0400 In-Reply-To: <9766BA3D-B8F9-456B-9F59-60D21B86E390@gmail.com> (message from Yuan Fu on Tue, 26 May 2020 18:29:04 -0400) 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-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:251515 Archived-At: > From: Yuan Fu > Date: Tue, 26 May 2020 18:29:04 -0400 > Cc: Lars Ingebrigtsen , > emacs-devel@gnu.org > > Here is the version that doesn’t use text properties. Thanks, few comments below. > I assume by string you mean display properties? I checked with display property and it wraps fine in this version. Display properties whose values are strings, and also before-string and after-string overlay properties. > +static bool char_can_wrap_before (struct it *it) > +{ > + /* We used to only check for whitespace for wrapping, hence this > + macro. You cannot wrap before a whitespace. */ > + return ((it->what == IT_CHARACTER > + && !CHAR_HAS_CATEGORY(it->c, NOT_AT_BOL)) > + /* There used to be */ > + && !IT_DISPLAYING_WHITESPACE (it)); > +} The order here is wrong: the IT_DISPLAYING_WHITESPACE should be tested first, as that is the more frequent situation, so it should be processed faster. > +/* Return true if the current character allows wrapping after it. */ > +static bool char_can_wrap_after (struct it *it) > +{ > + return ((it->what == IT_CHARACTER > + && CHAR_HAS_CATEGORY (it->c, LINE_BREAKABLE) > + && !CHAR_HAS_CATEGORY(it->c, NOT_AT_EOL)) > + /* We used to only check for whitespace for wrapping, hence > + this macro. Obviously you can wrap after a space. */ > + || IT_DISPLAYING_WHITESPACE (it)); > +} Do we really need two separate functions? And note that each one calls IT_DISPLAYING_WHITESPACE, so in some situations you will be testing that twice for the same character -- because you have 2 separate functions. > - if (IT_DISPLAYING_WHITESPACE (it)) > - may_wrap = true; > - else if (may_wrap) > + /* Can we wrap here? */ > + if (may_wrap && char_can_wrap_before(it)) > { > /* We have reached a glyph that follows one or more > - whitespace characters. If the position is > - already found, we are done. */ > + whitespace characters (or a character that allows > + wrapping after it). If the position is already > + found, we are done. */ The code calls char_can_wrap_before, but the comment says we can wrap after it. Which one is right? > + /* This has to run after the previous block. */ This kind of comment begs the question: "why?" Please rewrite the comment to answer that question up front. > - /* If we are at a whitespace character > - that barely fits on this screen line, > - but the next character is also > - whitespace, we cannot wrap here. */ > + /* If the previous character says we can > + wrap after it, but the current > + character says we can't wrap before > + it, then we can't wrap here. */ It sounds like your Emacs is set up to use only spaces for indentation in C source files, whereas our convention is to use tabs and spaces. > + DEFSYM (Qword_wrap, "word-wrap"); > + DEFSYM (Qonly_before, "only-before"); > + DEFSYM (Qonly_after, "only-after"); > + DEFSYM (Qno_wrap, "no-wrap"); These symbols are not used in the code. And finally, one more general comment/question: isn't your code assume implicitly that buffer text is always processed in the logical order, i.e. in the increasing order of buffer positions? I mean, the fact that you have the "before" and the "after" function seems to imply that you do assume that, and the logic of processing the categories is relying on that, expecting that when you see a wrap_after character, you can wrap on the next character. Is this so? because if it is, then this will break when processing RTL text, since we may process it in the reverse order of buffer positions. Please look into these situations and see that the code does TRT in them.