Re: Line wrap reconsidered

[Eli Zaretskii <eliz@gnu.org>]

> From: Yuan Fu <casouri@gmail.com>
> Date: Mon, 13 Jul 2020 15:46:16 -0400
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,
>  emacs-devel@gnu.org
> 
> Please have a look at the patch and see if it’s ok. If you think it’s good I can then update NEWS and the manual and submit a bug report. wrap.txt is the file I used to test word wrapping. To enable the full feature, set cjk-word-wrap to t and load kinsoku.el.

Yes, we need to update NEWS and the manual.

Also, we may need to rename cjk-word-wrap to something more accurate,
as result of your answers to my questions below.

A few minor comments below.

> * src/xdisp.c (it_char_has_category, char_can_wrap_before,
> char_can_wrap_after): New function.
                        ^^^^^^^^^^^^
"New functions", in plural.

> (move_it_in_display_line_to, display_line): Replace
> IT_DISPLAYING_WHITESPACE with char_can_wrap_before and
> char_can_wrap_after.

Please quote all references in commit log messages to functions and
variables 'like this'.

> +/* These are the category sets we use.  */
> +#define NOT_AT_EOL 60 /* < */
> +#define NOT_AT_BOL 62 /* > */
> +#define LINE_BREAKABLE 124 /* | */

Why not just use the characters themselves, as in '<' and '|' ?

Also, if these characters are from kinsoku.el, please says so in
comments, because if kinsoku.el changes, we may need to update those.

> +static bool it_char_has_category(struct it *it, int cat)
> +{
> +  if (it->what == IT_CHARACTER)
> +    return CHAR_HAS_CATEGORY (it->c, cat);
> +  else if (STRINGP (it->string))
> +    return CHAR_HAS_CATEGORY (SREF (it->string,
> +                                    IT_STRING_BYTEPOS (*it)), cat);
> +  else if (it->s)
> +    return CHAR_HAS_CATEGORY (it->s[IT_BYTEPOS (*it)], cat);
> +  else if (IT_BYTEPOS (*it) < ZV_BYTE)
> +    return CHAR_HAS_CATEGORY (*BYTE_POS_ADDR (IT_BYTEPOS (*it)), cat);
> +  else
> +    return false;
> +}

A minor stylistic nit: I'd prefer the if - elseif clauses to yield the
relevant character, and then apply CHAR_HAS_CATEGORY only once to that
character at the end.  (It is generally better to have only one return
point from a function, especially when the function is short.  If
nothing else, it makes debugging easier.)

> +  return (!IT_DISPLAYING_WHITESPACE (it)
> +          // Can be at BOL.

Please don't use //-style C++ comments, we use the C /* style */
comments instead.

> +  return (IT_DISPLAYING_WHITESPACE (it)
> +          // Can break after && can be at EOL.
> +            || (it_char_has_category (it, LINE_BREAKABLE)
> +                && !it_char_has_category (it, not_at_eol)));

Same here.

>  	  if (it->line_wrap == WORD_WRAP && it->area == TEXT_AREA)
>  	    {
> -	      if (IT_DISPLAYING_WHITESPACE (it))
> -		may_wrap = true;
> -	      else if (may_wrap)
> +              /* Can we wrap here? */
> +	      if (may_wrap && char_can_wrap_before (it))

I'm worried about a potential change in logic here, when cjk-word-wrap
is off.  Previously, we just tested IT_DISPLAYING_WHITESPACE, but now
we also test may_wrap.  Is it guaranteed that may_wrap is always true
in that case?

> @@ -23292,9 +23365,8 @@ #define RECORD_MAX_MIN_POS(IT)					\
>  
>  	  if (it->line_wrap == WORD_WRAP && it->area == TEXT_AREA)
>  	    {
> -	      if (IT_DISPLAYING_WHITESPACE (it))
> -		may_wrap = true;
> -	      else if (may_wrap)
> +              /* Can we wrap here? */
> +	      if (may_wrap && char_can_wrap_before (it))

Likewise here.

>  		{
>  		  SAVE_IT (wrap_it, *it, wrap_data);
>  		  wrap_x = x;
> @@ -23308,9 +23380,13 @@ #define RECORD_MAX_MIN_POS(IT)					\
>  		  wrap_row_min_bpos = min_bpos;
>  		  wrap_row_max_pos = max_pos;
>  		  wrap_row_max_bpos = max_bpos;
> -		  may_wrap = false;
>  		}
> -	    }
> +              /* This has to run after the previous block.  */
> +	      if (char_can_wrap_after (it))
> +		may_wrap = true;
> +              else
> +                may_wrap = false;

Please use TABs and spaces to indent code in C source files.  The last
2 lines use only spaces.

> +  DEFVAR_BOOL("cjk-word-wrap", Vcjk_word_wrap,
> +    doc: /*  Non-nil means wrap after CJK chracters.

This is unclear.  Does it mean after _any_ CJK character, or just
after some?  And if the latter, which ones?

Thanks.