[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Line wrap reconsidered
From: |
Eli Zaretskii |
Subject: |
Re: Line wrap reconsidered |
Date: |
Sat, 18 Jul 2020 11:15:33 +0300 |
> 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.
- Re: Line wrap reconsidered, Yuan Fu, 2020/07/12
- Re: Line wrap reconsidered, Eli Zaretskii, 2020/07/12
- Re: Line wrap reconsidered, Yuan Fu, 2020/07/12
- Re: Line wrap reconsidered, Yuan Fu, 2020/07/13
- Re: Line wrap reconsidered,
Eli Zaretskii <=
- Re: Line wrap reconsidered, Yuan Fu, 2020/07/18
- Re: Line wrap reconsidered, Yuan Fu, 2020/07/18
- Re: Line wrap reconsidered, Stefan Monnier, 2020/07/18
- Re: Line wrap reconsidered, Eli Zaretskii, 2020/07/19
- Re: Line wrap reconsidered, Yuan Fu, 2020/07/19
- Re: Line wrap reconsidered, Yuan Fu, 2020/07/19