bug#47167: Multi-line comment-region with empty comment-continue

bug#47167: Multi-line comment-region with empty comment-continue

[Juri Linkov]

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

Tags: patch

Tried to customize 'comment-region' to use multi-line comments,
so when commenting out such region in html-mode:

line 1
line 2
line 3

instead of adding comments to each line:

<!-- line 1 -->
<!-- line 2 -->
<!-- line 3 -->

to add comments only at the beginning/end:

<!--
line 1
line 2
line 3
  -->

According to the documentation, a legitimate way to do this is
to use such customization:

(setq-local comment-style 'extra-line
            comment-continue "")

But currently it has no effect.  So needed to fix newcomment.el.
One possible fix is to return the original string from 'comment-padright'
even when the string contains only whitespace.  Maybe this would be
the right fix, but still not sure about other calls of 'comment-padright'.
So a safer fix would be to do this in the caller:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: comment-region-padright-continue.patch --]
[-- Type: text/x-diff, Size: 751 bytes --]

diff --git a/lisp/newcomment.el b/lisp/newcomment.el
index ea47eec4fd..a5bfb06795 100644
--- a/lisp/newcomment.el
+++ b/lisp/newcomment.el
@@ -1300,7 +1300,11 @@ comment-region-default-1
 	 (let ((s (comment-padleft comment-end numarg)))
 	   (and s (if (string-match comment-end-skip s) s
 		    (comment-padright comment-end))))
-	 (if multi (comment-padright comment-continue numarg))
+	 (if multi
+             (or (comment-padright comment-continue numarg)
+                 ;; `comment-padright' returns nil when
+                 ;; `comment-continue' contains only whitespace
+                 (and (stringp comment-continue) comment-continue)))
 	 (if multi
 	     (comment-padleft (comment-string-reverse comment-continue) numarg))
 	 block

bug#47167: [External] : bug#47167: Multi-line comment-region with empty comment-continue

[Drew Adams]

> Tried to customize 'comment-region' to use multi-line comments,
> so when commenting out such region in html-mode:
> 
> line 1
> line 2
> line 3
> 
> instead of adding comments to each line:
> 
> <!-- line 1 -->
> <!-- line 2 -->
> <!-- line 3 -->
> 
> to add comments only at the beginning/end:
> 
> <!--
> line 1
> line 2
> line 3
>   -->

(Haven't looked at your patch.)

Sounds like a reasonable, and useful thing to do.

However, `comment-region' also has optional behaviors
that involve its use of a prefix arg.  How with those
interact with what you propose?  Do they even make
sense (e.g. nesting comment levels and unnesting them)?

My guess is that what's needed is a separate command.

I'm guessing completely different prefix-arg behavior
would be appropriate for multi-line comments.  Maybe
you can find a way to merge the two without confusing
users (and the doc).  Or maybe it's not worth trying
to do that, and a separate command makes more sense.
Dunno.

bug#47167: [External] : bug#47167: Multi-line comment-region with empty comment-continue

[Juri Linkov]

> (Haven't looked at your patch.)
>
> Sounds like a reasonable, and useful thing to do.
>
> However, `comment-region' also has optional behaviors
> that involve its use of a prefix arg.  How with those
> interact with what you propose?  Do they even make
> sense (e.g. nesting comment levels and unnesting them)?

With (setq-local comment-style 'extra-line comment-continue ""),
`comment-region' currently breaks HTML syntax
because HTML doesn't support nested comments:

<!--
<!-- line 1 -->
<!-- line 2 -->
<!-- line 3 -->
  -->

With the proposed patch, HTML syntax stays valid:

<!--
line 1
line 2
line 3
  -->

bug#47167: [External] : bug#47167: Multi-line comment-region with empty comment-continue

[Drew Adams]

> > (Haven't looked at your patch.)
> > Sounds like a reasonable, and useful thing to do.
> >
> > However, `comment-region' also has optional behaviors
> > that involve its use of a prefix arg.  How with those
> > interact with what you propose?  Do they even make
> > sense (e.g. nesting comment levels and unnesting them)?
> 
> With (setq-local comment-style 'extra-line comment-continue ""),
> `comment-region' currently breaks HTML syntax
> because HTML doesn't support nested comments:
> 
> <!--
> <!-- line 1 -->
> <!-- line 2 -->
> <!-- line 3 -->
>   -->
> 
> With the proposed patch, HTML syntax stays valid:
> 
> <!--
> line 1
> line 2
> line 3
>   -->

I see, I guess.  But why change `comment-region',
instead of creating a new command for multi-line
(aka block) commenting?

If there's a bug now (e.g. wrt `comment-continue')
when someone uses `comment-region', then maybe
either that can be fixed (without involving
multiline) or that command can be made to ignore,
DTRT, or raise an error when `comment-continue'
is used.  (Just a wild guess, without looking at
any code.)

bug#47167: [External] : bug#47167: Multi-line comment-region with empty comment-continue

[Juri Linkov]

>> With (setq-local comment-style 'extra-line comment-continue ""),
>> `comment-region' currently breaks HTML syntax
>> because HTML doesn't support nested comments:
>>
>> <!--
>> <!-- line 1 -->
>> <!-- line 2 -->
>> <!-- line 3 -->
>>   -->
>>
>> With the proposed patch, HTML syntax stays valid:
>>
>> <!--
>> line 1
>> line 2
>> line 3
>>   -->
>
> I see, I guess.  But why change `comment-region',
> instead of creating a new command for multi-line
> (aka block) commenting?

'multi-line' is just one of possible styles in 'comment-styles'.
I'm not interested in creating separate commands for each style.
If you want, you can create a separate bug report.

> If there's a bug now (e.g. wrt `comment-continue')
> when someone uses `comment-region', then maybe
> either that can be fixed (without involving
> multiline) or that command can be made to ignore,

Indeed, there is a bug.  The documentation of 'comment-continue' says:

  Continuation string to insert for multiline comments.

  If it is nil a value will be automatically derived from ‘comment-start’
  by replacing its first character with a space.

The problem is that currently the empty string behaves as if it's nil.
The patch fixes it.

bug#47167: Multi-line comment-region with empty comment-continue

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> But currently it has no effect.  So needed to fix newcomment.el.
> One possible fix is to return the original string from 'comment-padright'
> even when the string contains only whitespace.  Maybe this would be
> the right fix, but still not sure about other calls of 'comment-padright'.
> So a safer fix would be to do this in the caller:

Looks good to me.

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

bug#47167: Multi-line comment-region with empty comment-continue

[Juri Linkov]

tags 47167 fixed
close 47167 28.0.50
quit

>> But currently it has no effect.  So needed to fix newcomment.el.
>> One possible fix is to return the original string from 'comment-padright'
>> even when the string contains only whitespace.  Maybe this would be
>> the right fix, but still not sure about other calls of 'comment-padright'.
>> So a safer fix would be to do this in the caller:
>
> Looks good to me.

Now pushed to master.