[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Line wrap reconsidered
From: |
Yuan Fu |
Subject: |
Re: Line wrap reconsidered |
Date: |
Sat, 18 Jul 2020 13:14:15 -0400 |
> On Jul 18, 2020, at 4:15 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>
>> 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.
Cool, I’ll start on NEWS and manual once we are settled on the name of the new
variable. I agree cjk-word-wrap isn’t a good name. I just used it as a
placeholder.
>
> 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.
>
Fixed.
>> +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.)
I changed the it, do you code below this is ok?
if (ch == 0)
return false;
else
return CHAR_HAS_CATEGORY(ch, cat);
>
>> + 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.
Fixed.
>
>> 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.
In both can_wrap_before and can_wrap_after, I have a short circuit for the case
when cjk_word_wrap is nil:
if (!Vcjk_word_wrap)
return IT_DISPLAYING_WHITESPACE (it);
That should guarantee the old behavior when cjk_word_wrap is nil, if that’s
what you are asking about.
>
>> {
>> 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.
Sorry, fixed.
>
>> + 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?
I added more detail and hopefully they are clearer now.
>
> Thanks.
Thanks!
Yuan
word-wrap.patch
Description: Binary data
- 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, 2020/07/18
- Re: Line wrap reconsidered,
Yuan Fu <=
- 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