Re: Line wrap reconsidered

[Eli Zaretskii <eliz@gnu.org>]

> From: Yuan Fu <casouri@gmail.com>
> Date: Tue, 26 May 2020 18:29:04 -0400
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,
>  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.