bug#39517: [PATCH] Add new option 'word-wrap-boundary'

bug#39517: [PATCH] Add new option 'word-wrap-boundary'

[Jaehwang Jerry Jung]

* src/buffer.c (syms_of_buffer): Define buffer-local variable
word-wrap-boundary.
* src/buffer.h (struct buffer): Add word_wrap_boundary_.
* src/xdisp.c (IT_DISPLAYING_WORD_WRAP_BOUNDARY): replaces
IT_DISPLAYING_WHITESPACE.
---
 etc/NEWS     |  5 +++++
 src/buffer.c | 16 ++++++++++++++--
 src/buffer.h |  3 +++
 src/xdisp.c  | 43 ++++++++++++++++++++++---------------------
 4 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 7b358ff271..40f432e5bb 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -70,6 +70,11 @@ specify cursor-type to be '(box . SIZE)', the cursor becomes a hollow
 box if the point is on an image larger than 'SIZE' pixels in any
 dimension.
 
+** New option 'word-wrap-boundary'.
+This defines the characters that can be used as a wrapping boundary
+when 'word-wrap' is on. It defaults to " \t", the characters that had
+been hard-coded in the previous versions of Emacs.
+
 \f
 * Editing Changes in Emacs 28.1
 
diff --git a/src/buffer.c b/src/buffer.c
index cc7d4e4817..fa48461b2f 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -369,6 +369,11 @@ bset_word_wrap (struct buffer *b, Lisp_Object val)
   b->word_wrap_ = val;
 }
 static void
+bset_word_wrap_boundary (struct buffer *b, Lisp_Object val)
+{
+  b->word_wrap_boundary_ = val;
+}
+static void
 bset_zv_marker (struct buffer *b, Lisp_Object val)
 {
   b->zv_marker_ = val;
@@ -5168,6 +5173,7 @@ init_buffer_once (void)
   /* Make this one a permanent local.  */
   buffer_permanent_local_flags[idx++] = 1;
   XSETFASTINT (BVAR (&buffer_local_flags, word_wrap), idx); ++idx;
+  XSETFASTINT (BVAR (&buffer_local_flags, word_wrap_boundary), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, ctl_arrow), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, fill_column), idx); ++idx;
   XSETFASTINT (BVAR (&buffer_local_flags, left_margin), idx); ++idx;
@@ -5265,6 +5271,7 @@ init_buffer_once (void)
   XSETFASTINT (BVAR (&buffer_defaults, tab_width), 8);
   bset_truncate_lines (&buffer_defaults, Qnil);
   bset_word_wrap (&buffer_defaults, Qnil);
+  bset_word_wrap_boundary (&buffer_defaults, build_string (" \t"));
   bset_ctl_arrow (&buffer_defaults, Qt);
   bset_bidi_display_reordering (&buffer_defaults, Qt);
   bset_bidi_paragraph_direction (&buffer_defaults, Qnil);
@@ -5752,8 +5759,8 @@ syms_of_buffer (void)
 
   DEFVAR_PER_BUFFER ("word-wrap", &BVAR (current_buffer, word_wrap), Qnil,
 		     doc: /* Non-nil means to use word-wrapping for continuation lines.
-When word-wrapping is on, continuation lines are wrapped at the space
-or tab character nearest to the right window edge.
+When word-wrapping is on, continuation lines are wrapped at the character
+defined in `word-wrap-boundary` nearest to the right window edge.
 If nil, continuation lines are wrapped at the right screen edge.
 
 This variable has no effect if long lines are truncated (see
@@ -5768,6 +5775,11 @@ syms_of_buffer (void)
 visual lines rather than logical lines.  See the documentation of
 `visual-line-mode'.  */);
 
+  DEFVAR_PER_BUFFER ("word-wrap-boundary",
+		     &BVAR (current_buffer, word_wrap_boundary), Qstringp,
+		     doc: /* Characters that may cause line wrapping when `word-wrap` is on.
+" \\t" initially. */);
+
   DEFVAR_PER_BUFFER ("default-directory", &BVAR (current_buffer, directory),
 		     Qstringp,
 		     doc: /* Name of default directory of current buffer.
diff --git a/src/buffer.h b/src/buffer.h
index fd05fdd37d..69707c4020 100644
--- a/src/buffer.h
+++ b/src/buffer.h
@@ -394,6 +394,9 @@ #define BVAR(buf, field) ((buf)->field ## _)
   /* Non-nil means to use word wrapping when displaying continuation lines.  */
   Lisp_Object word_wrap_;
 
+  /* Characters that may cause word wrapping.  */
+  Lisp_Object word_wrap_boundary_;
+
   /* Non-nil means display ctl chars with uparrow.  */
   Lisp_Object ctl_arrow_;
 
diff --git a/src/xdisp.c b/src/xdisp.c
index e41ceaf0bb..7e3710230c 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -412,11 +412,12 @@ #define IT_OVERFLOW_NEWLINE_INTO_FRINGE(it) false
 #endif /* HAVE_WINDOW_SYSTEM */
 
 /* Test if the display element loaded in IT, or the underlying buffer
-   or string character, is a space or a TAB character.  This is used
-   to determine where word wrapping can occur.  */
+   or string character, is a word wrap boundary character.  */
 
-#define IT_DISPLAYING_WHITESPACE(it)					\
-  ((it->what == IT_CHARACTER && (it->c == ' ' || it->c == '\t'))	\
+#define IT_DISPLAYING_WORD_WRAP_BOUNDARY(it)				\
+  ((it->what == IT_CHARACTER						\
+    && strchr ((char *) SDATA (BVAR (current_buffer, word_wrap_boundary)), \
+		it->c))							\
    || ((STRINGP (it->string)						\
 	&& (SREF (it->string, IT_STRING_BYTEPOS (*it)) == ' '		\
 	    || SREF (it->string, IT_STRING_BYTEPOS (*it)) == '\t'))	\
@@ -9078,12 +9079,12 @@ #define IT_RESET_X_ASCENT_DESCENT(IT)			\
 	{
 	  if (it->line_wrap == WORD_WRAP && it->area == TEXT_AREA)
 	    {
-	      if (IT_DISPLAYING_WHITESPACE (it))
+	      if (IT_DISPLAYING_WORD_WRAP_BOUNDARY (it))
 		may_wrap = true;
 	      else if (may_wrap)
 		{
 		  /* We have reached a glyph that follows one or more
-		     whitespace characters.  If the position is
+		     boundary characters.  If the position is
 		     already found, we are done.  */
 		  if (atpos_it.sp >= 0)
 		    {
@@ -9228,10 +9229,10 @@ #define IT_RESET_X_ASCENT_DESCENT(IT)			\
 			    {
 			      bool can_wrap = true;
 
-			      /* If we are at a whitespace character
+			      /* If we are at a boundary character
 				 that barely fits on this screen line,
 				 but the next character is also
-				 whitespace, we cannot wrap here.  */
+				 boundary, we cannot wrap here.  */
 			      if (it->line_wrap == WORD_WRAP
 				  && wrap_it.sp >= 0
 				  && may_wrap
@@ -9243,13 +9244,13 @@ #define IT_RESET_X_ASCENT_DESCENT(IT)			\
 				  SAVE_IT (tem_it, *it, tem_data);
 				  set_iterator_to_next (it, true);
 				  if (get_next_display_element (it)
-				      && IT_DISPLAYING_WHITESPACE (it))
+				      && IT_DISPLAYING_WORD_WRAP_BOUNDARY (it))
 				    can_wrap = false;
 				  RESTORE_IT (it, &tem_it, tem_data);
 				}
 			      if (it->line_wrap != WORD_WRAP
 				  || wrap_it.sp < 0
-				  /* If we've just found whitespace
+				  /* If we've just found boundary
 				     where we can wrap, effectively
 				     ignore the previous wrap point --
 				     it is no longer relevant, but we
@@ -9322,19 +9323,19 @@ #define IT_RESET_X_ASCENT_DESCENT(IT)			\
 		  else
 		    IT_RESET_X_ASCENT_DESCENT (it);
 
-		  /* If the screen line ends with whitespace, and we
+		  /* If the screen line ends with boundary, and we
 		     are under word-wrap, don't use wrap_it: it is no
 		     longer relevant, but we won't have an opportunity
 		     to update it, since we are done with this screen
 		     line.  */
 		  if (may_wrap && IT_OVERFLOW_NEWLINE_INTO_FRINGE (it)
 		      /* If the character after the one which set the
-			 may_wrap flag is also whitespace, we can't
+			 may_wrap flag is also boundary, we can't
 			 wrap here, since the screen line cannot be
-			 wrapped in the middle of whitespace.
+			 wrapped in the middle of boundary.
 			 Therefore, wrap_it _is_ relevant in that
 			 case.  */
-		      && !(moved_forward && IT_DISPLAYING_WHITESPACE (it)))
+		      && !(moved_forward && IT_DISPLAYING_WORD_WRAP_BOUNDARY (it)))
 		    {
 		      /* If we've found TO_X, go back there, as we now
 			 know the last word fits on this screen line.  */
@@ -23163,7 +23164,7 @@ #define RECORD_MAX_MIN_POS(IT)					\
 
 	  if (it->line_wrap == WORD_WRAP && it->area == TEXT_AREA)
 	    {
-	      if (IT_DISPLAYING_WHITESPACE (it))
+	      if (IT_DISPLAYING_WORD_WRAP_BOUNDARY (it))
 		may_wrap = true;
 	      else if (may_wrap)
 		{
@@ -23308,10 +23309,10 @@ #define RECORD_MAX_MIN_POS(IT)					\
 			      /* Even if there is a previous wrap
 				 point, continue the line here as
 				 usual, if (i) the previous character
-				 was a space or tab AND (ii) the
-				 current character is not.  */
+				 was a boundary AND (ii) the current
+				 character is not.  */
 			      && (!may_wrap
-				  || IT_DISPLAYING_WHITESPACE (it)))
+				  || IT_DISPLAYING_WORD_WRAP_BOUNDARY (it)))
 			    goto back_to_wrap;
 
 			  /* Record the maximum and minimum buffer
@@ -23342,10 +23343,10 @@ #define RECORD_MAX_MIN_POS(IT)					\
 				       /* Even if there is a previous wrap
 					  point, continue the line here as
 					  usual, if (i) the previous character
-					  was a space or tab AND (ii) the
-					  current character is not.  */
+					  was a boundary AND (ii) the current
+					  character is not.  */
 				       && (!may_wrap
-					   || IT_DISPLAYING_WHITESPACE (it)))
+					   || IT_DISPLAYING_WORD_WRAP_BOUNDARY (it)))
 				goto back_to_wrap;
 
 			    }
-- 
2.17.1

bug#39517: [PATCH] Add new option 'word-wrap-boundary'

[Eli Zaretskii]

> From: Jaehwang Jerry Jung <tomtomjhj@gmail.com>
> Date: Sun,  9 Feb 2020 16:43:34 +0900
> Cc: Jaehwang Jerry Jung <tomtomjhj@gmail.com>
> 
> * src/buffer.c (syms_of_buffer): Define buffer-local variable
> word-wrap-boundary.
> * src/buffer.h (struct buffer): Add word_wrap_boundary_.
> * src/xdisp.c (IT_DISPLAYING_WORD_WRAP_BOUNDARY): replaces
> IT_DISPLAYING_WHITESPACE.

Thank you for your interest in Emacs.

When proposing a new feature, please always describe the rationale and
relevant practical use cases.  In this case, it is not clear to me
when it would make sense to allow arbitrary characters as wrap
boundaries.  Maybe you had only white-space characters in mind, but
then why not use [:space:] or some Unicode category specification
instead of allowing any characters?

> +  /* Characters that may cause word wrapping.  */
> +  Lisp_Object word_wrap_boundary_;

Any reason this is a Lisp object and not a C string?

> -#define IT_DISPLAYING_WHITESPACE(it)					\
> -  ((it->what == IT_CHARACTER && (it->c == ' ' || it->c == '\t'))	\
> +#define IT_DISPLAYING_WORD_WRAP_BOUNDARY(it)				\
> +  ((it->what == IT_CHARACTER						\
> +    && strchr ((char *) SDATA (BVAR (current_buffer, word_wrap_boundary)), \
> +		it->c))							\

This cannot be right: characters are stored in Lisp strings in a
multibyte encoding that is superset of UTF-8, so the above will only
support pure-ASCII boundary characters, which is probably not what you
had in mind.

This feature, if we decide to accept it, will also need to be
described in the Emacs user manual, as that is why you exposed this to
Lisp.  We would also want a defcustom form for it, probably in
cus-start.el.

Last, but not least, for a contribution this large, we will need you
to assign the copyright to the FSF.  If you agree, I will send you the
form to fill and the instructions to send it.

bug#39517: [PATCH] Add new option 'word-wrap-boundary'

[Jaehwang Jerry Jung]

[-- Attachment #1: Type: text/plain, Size: 2489 bytes --]

Hi. Thank you for the comments.

On 2/10/20 12:34 AM, Eli Zaretskii wrote:
>> From: Jaehwang Jerry Jung<tomtomjhj@gmail.com>
>> Date: Sun,  9 Feb 2020 16:43:34 +0900
>> Cc: Jaehwang Jerry Jung<tomtomjhj@gmail.com>
>>
>> * src/buffer.c (syms_of_buffer): Define buffer-local variable
>> word-wrap-boundary.
>> * src/buffer.h (struct buffer): Add word_wrap_boundary_.
>> * src/xdisp.c (IT_DISPLAYING_WORD_WRAP_BOUNDARY): replaces
>> IT_DISPLAYING_WHITESPACE.
> Thank you for your interest in Emacs.
>
> When proposing a new feature, please always describe the rationale and
> relevant practical use cases.  In this case, it is not clear to me
> when it would make sense to allow arbitrary characters as wrap
> boundaries.  Maybe you had only white-space characters in mind, but
> then why not use [:space:] or some Unicode category specification
> instead of allowing any characters?

I noticed that word wrapping looks a bit weird when the text contains
long URLs.  So I wanted to add non-word ASCII characters so that URLs
can be wrapped more naturally as in other editors, while not changing
the default behavior.

>> +  /* Characters that may cause word wrapping.  */
>> +  Lisp_Object word_wrap_boundary_;
> Any reason this is a Lisp object and not a C string?

No specific reason for that. I chose Lisp object because other members
like name_ are Lisp objects too.

>> -#define IT_DISPLAYING_WHITESPACE(it)					\
>> -  ((it->what == IT_CHARACTER && (it->c == ' ' || it->c == '\t'))	\
>> +#define IT_DISPLAYING_WORD_WRAP_BOUNDARY(it)				\
>> +  ((it->what == IT_CHARACTER						\
>> +    && strchr ((char *) SDATA (BVAR (current_buffer, word_wrap_boundary)), \
>> +		it->c))							\
> This cannot be right: characters are stored in Lisp strings in a
> multibyte encoding that is superset of UTF-8, so the above will only
> support pure-ASCII boundary characters, which is probably not what you
> had in mind.

You're right. Actually I think it would be simpler to hard-code a better
list of boundary characters in that macro.

> This feature, if we decide to accept it, will also need to be
> described in the Emacs user manual, as that is why you exposed this to
> Lisp.  We would also want a defcustom form for it, probably in
> cus-start.el.

Thanks for letting me know that.

> Last, but not least, for a contribution this large, we will need you
> to assign the copyright to the FSF.  If you agree, I will send you the
> form to fill and the instructions to send it.

Yes, I agree.


[-- Attachment #2: Type: text/html, Size: 3846 bytes --]

bug#39517: [PATCH] Add new option 'word-wrap-boundary'

[Eli Zaretskii]

> From: Jaehwang Jerry Jung <tomtomjhj@gmail.com>
> Cc: 39517@debbugs.gnu.org
> Date: Tue, 11 Feb 2020 00:36:01 +0900
> 
> I noticed that word wrapping looks a bit weird when the text contains
> long URLs.  So I wanted to add non-word ASCII characters so that URLs
> can be wrapped more naturally as in other editors, while not changing
> the default behavior.

OK, but is that the only relevant use case?  Maybe we have others.  We
need to think in more general terms, in case the other use cases might
suggest different solutions.

> -  ((it->what == IT_CHARACTER && (it->c == ' ' || it->c == '\t'))	\
> +#define IT_DISPLAYING_WORD_WRAP_BOUNDARY(it)				\
> +  ((it->what == IT_CHARACTER						\
> +    && strchr ((char *) SDATA (BVAR (current_buffer, word_wrap_boundary)), \
> +		it->c))							\
> 
> This cannot be right: characters are stored in Lisp strings in a
> multibyte encoding that is superset of UTF-8, so the above will only
> support pure-ASCII boundary characters, which is probably not what you
> had in mind.
> 
> You're right. Actually I think it would be simpler to hard-code a better
> list of boundary characters in that macro.

I don't think we can hardcode them, because the set of characters must
be buffer-local: we don't want to wrap on '/' in a general text-mode
buffer, let alone in a C-mode buffer, right?

>  Last, but not least, for a contribution this large, we will need you
> to assign the copyright to the FSF.  If you agree, I will send you the
> form to fill and the instructions to send it.
> 
> Yes, I agree.

Form sent off-list.

Thanks.

bug#39517: [PATCH] Add new option 'word-wrap-boundary'

[Jaehwang Jerry Jung]

On 11/02/2020 01:10, Eli Zaretskii wrote:
>> From: Jaehwang Jerry Jung <tomtomjhj@gmail.com>
>> Cc: 39517@debbugs.gnu.org
>> Date: Tue, 11 Feb 2020 00:36:01 +0900
>>
>> I noticed that word wrapping looks a bit weird when the text contains
>> long URLs.  So I wanted to add non-word ASCII characters so that URLs
>> can be wrapped more naturally as in other editors, while not changing
>> the default behavior.
> OK, but is that the only relevant use case?  Maybe we have others.  We
> need to think in more general terms, in case the other use cases might
> suggest different solutions.

Not that I know of.

>> -  ((it->what == IT_CHARACTER && (it->c == ' ' || it->c == '\t'))	\
>> +#define IT_DISPLAYING_WORD_WRAP_BOUNDARY(it)				\
>> +  ((it->what == IT_CHARACTER						\
>> +    && strchr ((char *) SDATA (BVAR (current_buffer, word_wrap_boundary)), \
>> +		it->c))							\
>>
>> This cannot be right: characters are stored in Lisp strings in a
>> multibyte encoding that is superset of UTF-8, so the above will only
>> support pure-ASCII boundary characters, which is probably not what you
>> had in mind.
>>
>> You're right. Actually I think it would be simpler to hard-code a better
>> list of boundary characters in that macro.
> I don't think we can hardcode them, because the set of characters must
> be buffer-local: we don't want to wrap on '/' in a general text-mode
> buffer, let alone in a C-mode buffer, right?

It's not uncommon to wrap on '/'. For example,
* Vim: http://vimdoc.sourceforge.net/htmldoc/options.html#'breakat'
* Visual Studio Code: https://github.com/microsoft/vscode/blob/39370d9c988c584fa2e834728d8ffa56259ff1ad/src/vs/editor/common/config/editorOptions.ts#L279-L283

Anyway, I agree with configurability. Perhaps I'll need some more time
to implement it with multibyte characters in mind. Or we could use the
syntax table.

>>   Last, but not least, for a contribution this large, we will need you
>> to assign the copyright to the FSF.  If you agree, I will send you the
>> form to fill and the instructions to send it.
>>
>> Yes, I agree.
> Form sent off-list.
>
> Thanks.

Thanks. I sent the request.

bug#39517: [PATCH] Add new option 'word-wrap-boundary'

[Lars Ingebrigtsen]

Jaehwang Jerry Jung <tomtomjhj@gmail.com> writes:

>>>   Last, but not least, for a contribution this large, we will need you
>>> to assign the copyright to the FSF.  If you agree, I will send you the
>>> form to fill and the instructions to send it.
>>>
>>> Yes, I agree.
>> Form sent off-list.
>>
>> Thanks.
>
> Thanks. I sent the request.

Copyright assignment papers are on file now, I see.  And I think the
feature makes sense -- wrapping on "/" characters, for instance, is
something I think buffers for editing HTML (and the like) would benefit
from.

Eli had some comments about the implementation, though.  Could you spin
a new version of the patch that takes those comments into consideration?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39517: [PATCH] Add new option 'word-wrap-boundary'

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  39517@debbugs.gnu.org
> Date: Sun, 09 Aug 2020 13:23:38 +0200
> 
> Copyright assignment papers are on file now, I see.  And I think the
> feature makes sense -- wrapping on "/" characters, for instance, is
> something I think buffers for editing HTML (and the like) would benefit
> from.
> 
> Eli had some comments about the implementation, though.  Could you spin
> a new version of the patch that takes those comments into consideration?

Isn't this feature superseded by the more general one submitted in the
discussion on emacs-devel Re: Line wrap reconsidered?  There, I'm
waiting for the final submission after the last review, and will
probably install after that.

If the changes there don't do everything this proposal does, I suggest
to modify this proposal to be compatible with that one.

Thanks.

bug#39517: [PATCH] Add new option 'word-wrap-boundary'

[Jaehwang Jerry Jung]

I think my proposal is subsumed by "Line wrap reconsidered". If I
understood correctly, one can simply add characters like "/" to "|"
category to get the similar effect.

On Sun, Aug 9, 2020 at 11:11 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Lars Ingebrigtsen <larsi@gnus.org>
> > Cc: Eli Zaretskii <eliz@gnu.org>,  39517@debbugs.gnu.org
> > Date: Sun, 09 Aug 2020 13:23:38 +0200
> >
> > Copyright assignment papers are on file now, I see.  And I think the
> > feature makes sense -- wrapping on "/" characters, for instance, is
> > something I think buffers for editing HTML (and the like) would benefit
> > from.
> >
> > Eli had some comments about the implementation, though.  Could you spin
> > a new version of the patch that takes those comments into consideration?
>
> Isn't this feature superseded by the more general one submitted in the
> discussion on emacs-devel Re: Line wrap reconsidered?  There, I'm
> waiting for the final submission after the last review, and will
> probably install after that.
>
> If the changes there don't do everything this proposal does, I suggest
> to modify this proposal to be compatible with that one.
>
> Thanks.

bug#39517: [PATCH] Add new option 'word-wrap-boundary'

[Lars Ingebrigtsen]

Jaehwang Jerry Jung <tomtomjhj@gmail.com> writes:

> I think my proposal is subsumed by "Line wrap reconsidered". If I
> understood correctly, one can simply add characters like "/" to "|"
> category to get the similar effect.

OK, then I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39517: [PATCH] Add new option 'word-wrap-boundary'

[Eli Zaretskii]

> From: Jaehwang Jerry Jung <tomtomjhj@gmail.com>
> Date: Mon, 10 Aug 2020 00:23:39 +0900
> Cc: Lars Ingebrigtsen <larsi@gnus.org>, 39517@debbugs.gnu.org
> 
> I think my proposal is subsumed by "Line wrap reconsidered". If I
> understood correctly, one can simply add characters like "/" to "|"
> category to get the similar effect.

That's my understanding as well, thanks.