bug#6806: Set comment-multi-line in js-mode

bug#6806: Set comment-multi-line in js-mode

[Nathan Weizenbaum]

[-- Attachment #1.1: Type: text/plain, Size: 318 bytes --]

Package: emacs
Version: 24.0.50.1

js-mode doesn't set the comment-multi-line variable. This results in
comment-indent-new-line behaving improperly when used in a multi-line
comment, which also affects auto-fill-mode, according to the documentation
for comment-multi-line.

Attached is a patch that sets the variable.

[-- Attachment #1.2: Type: text/html, Size: 383 bytes --]

[-- Attachment #2: 0001-Set-comment-multi-line-in-js-mode.patch --]
[-- Type: text/x-patch, Size: 723 bytes --]

From 277d96c62cdfb8b1d862631724f67518152588f3 Mon Sep 17 00:00:00 2001
From: Nathan Weizenbaum <nex342@gmail.com>
Date: Tue, 3 Aug 2010 17:46:33 -0700
Subject: [PATCH] Set comment-multi-line in js-mode.

---
 lisp/progmodes/js.el |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 6bd8fbc..023d253 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -3302,6 +3302,7 @@ Key bindings:
   (setq comment-end "")
   (set (make-local-variable 'fill-paragraph-function)
        'js-c-fill-paragraph)
+  (set (make-local-variable 'comment-multi-line) t)
 
   ;; Parse cache
   (add-hook 'before-change-functions #'js--flush-caches t t)
-- 
1.7.1

bug#6806: Set comment-multi-line in js-mode

[Chong Yidong]

Nathan Weizenbaum <nweiz@google.com> writes:

> js-mode doesn't set the comment-multi-line variable. This results in
> comment-indent-new-line behaving improperly when used in a multi-line comment,
> which also affects auto-fill-mode, according to the documentation for
> comment-multi-line.
>
> Attached is a patch that sets the variable.

Could you provide a precise recipe for the problem?

bug#6806: Set comment-multi-line in js-mode

[Nathan Weizenbaum]

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

Open a new Javascript file. Type "/*". Run M-x comment-indent-new-line. This
inserts "\n  /*", which is incorrect.

Now that I try to reproduce my fix, though, it doesn't seem to work. I'm not
sure what the proper solution is.

On Sun, Aug 8, 2010 at 1:40 PM, Chong Yidong <cyd@stupidchicken.com> wrote:

> Nathan Weizenbaum <nweiz@google.com> writes:
>
> > js-mode doesn't set the comment-multi-line variable. This results in
> > comment-indent-new-line behaving improperly when used in a multi-line
> comment,
> > which also affects auto-fill-mode, according to the documentation for
> > comment-multi-line.
> >
> > Attached is a patch that sets the variable.
>
> Could you provide a precise recipe for the problem?
>

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

bug#6806: Set comment-multi-line in js-mode

[Stefan Monnier]

> js-mode doesn't set the comment-multi-line variable. This results in
> comment-indent-new-line behaving improperly when used in a multi-line
> comment, which also affects auto-fill-mode, according to the documentation
> for comment-multi-line.

If I understand and remember correctly, this is not the right fix for
your problem: setting comment-multi-line to t will fix your problem with
"/*..*/" but will introduce another for "//....\n" and vice-versa.


        Stefan

bug#6806: Set comment-multi-line in js-mode

[Nathan Weizenbaum]

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

My fix doesn't actually fix the "/* */" issue, unfortunately. However, it
doesn't break "//" either; note that comment-multi-line is t for e.g.
c-mode, and comment-indent-new-line works for "//" there.

On Mon, Aug 9, 2010 at 4:33 AM, Stefan Monnier <monnier@iro.umontreal.ca>wrote:

> > js-mode doesn't set the comment-multi-line variable. This results in
> > comment-indent-new-line behaving improperly when used in a multi-line
> > comment, which also affects auto-fill-mode, according to the
> documentation
> > for comment-multi-line.
>
> If I understand and remember correctly, this is not the right fix for
> your problem: setting comment-multi-line to t will fix your problem with
> "/*..*/" but will introduce another for "//....\n" and vice-versa.
>
>
>        Stefan
>

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

bug#6806: Set comment-multi-line in js-mode

[Stefan Monnier]

> My fix doesn't actually fix the "/* */" issue, unfortunately. However, it
> doesn't break "//" either; note that comment-multi-line is t for e.g.
> c-mode, and comment-indent-new-line works for "//" there.

Indeed, I misremembered.  Feel free to install this patch.


        Stefan
        

> On Mon, Aug 9, 2010 at 4:33 AM, Stefan Monnier <monnier@iro.umontreal.ca>wrote:

>> > js-mode doesn't set the comment-multi-line variable. This results in
>> > comment-indent-new-line behaving improperly when used in a multi-line
>> > comment, which also affects auto-fill-mode, according to the
>> documentation
>> > for comment-multi-line.
>> 
>> If I understand and remember correctly, this is not the right fix for
>> your problem: setting comment-multi-line to t will fix your problem with
>> "/*..*/" but will introduce another for "//....\n" and vice-versa.
>> 
>> 
>> Stefan
>> 

bug#6806: Set comment-multi-line in js-mode

[Chong Yidong]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> My fix doesn't actually fix the "/* */" issue, unfortunately. However, it
>> doesn't break "//" either; note that comment-multi-line is t for e.g.
>> c-mode, and comment-indent-new-line works for "//" there.
>
> Indeed, I misremembered.  Feel free to install this patch.

The patch doesn't do the right thing.  The reported problem is that if
you enter "/*" in a js-mode buffer and do M-x comment-indent-new-line,
Emacs inserts another "/*".

This problem is not limited to js-mode.  It afflicts C++ also.  Try
this:

  C-x C-f foo.cc RET
  /*
  M-x comment-indent-new-line RET

Emacs inserts another /*.  The reason is this stretch of code in
newcomment.el:1311:

     (normalp
      (string-match (regexp-quote (comment-string-strip
                                   comment-start t t))
                    comstart))
     (comment-end
      (if normalp comment-end
        ;; The comment starter is not the normal comment-start
        ;; so we can't just use comment-end.
        (save-excursion
          (goto-char compos)
          (if (not (comment-forward)) comment-end
            (comment-string-strip
             (buffer-substring
              (save-excursion (comment-enter-backward) (point))
              (point))
             nil t)))))

When the default comment-start is "//" but the current comment begins in
"/*", this code tries to find the appropriate comment-end by doing
comment-forward.  But if the comment-end "*/" is not already present in
the buffer, it fails.

Any suggestion?

bug#6806: another reason for the patch

[Tom Tromey]

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

Hi.  I'd like to resurrect this bug.

While working in JS I found a bug, which I tracked down to the same
change indicated here.  Basically, auto-fill in JS mode will sometimes
insert a "/*" on a filled line, where it should not.

You can see a precise recipe in the new test case, in the attached
patch.

One funny thing is that if you do this interactively (in my real-world
case I had auto-fill-mode enabled), you have to be sure that there isn't
a newline after point.  If there is a newline, the filling works
properly.

Anyway, let me know what you think.  I'd like to check this in, but,
given the comments in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=6806#23
I suppose I would not close the bug.

Tom


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: the patch --]
[-- Type: text/x-patch, Size: 1538 bytes --]

commit 5a615aeae34a257a1879134a883c61bc42f41691
Author: Tom Tromey <tom@tromey.com>
Date:   Tue Jan 17 21:50:14 2017 -0700

    Set comment-multi-line in js-mode
    
    Bug#6806:
    * lisp/progmodes/js.el (js-mode): Set comment-multi-line to t.
    * test/lisp/progmodes/js-tests.el (js-mode-auto-fill): New test.

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 2e5c6ae..cfd58d2 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -3852,6 +3852,7 @@ js-mode
         comment-start-skip "\\(//+\\|/\\*+\\)\\s *")
   (setq-local comment-line-break-function #'c-indent-new-comment-line)
   (setq-local c-block-comment-start-regexp "/\\*")
+  (setq-local comment-multi-line t)
 
   (setq-local electric-indent-chars
 	      (append "{}():;," electric-indent-chars)) ;FIXME: js2-mode adds "[]*".
diff --git a/test/lisp/progmodes/js-tests.el b/test/lisp/progmodes/js-tests.el
index 84749ef..7cb737c 100644
--- a/test/lisp/progmodes/js-tests.el
+++ b/test/lisp/progmodes/js-tests.el
@@ -85,6 +85,20 @@
       (should (= (current-column) x))
       (forward-line))))
 
+(ert-deftest js-mode-auto-fill ()
+  (with-temp-buffer
+    (js-mode)
+    (setq fill-column 70)
+    (insert "/* ")
+    (dotimes (_ 16)
+      (insert "test "))
+    (do-auto-fill)
+    ;; The bug is that, after auto-fill, the second line starts with
+    ;; "/*", whereas it should start with " * ".
+    (goto-char (point-min))
+    (forward-line)
+    (should (looking-at " \\* test"))))
+
 (provide 'js-tests)
 
 ;;; js-tests.el ends here

bug#6806: another reason for the patch

[npostavs]

Tom Tromey <tom@tromey.com> writes:

> Hi.  I'd like to resurrect this bug.
>
> While working in JS I found a bug, which I tracked down to the same
> change indicated here.  Basically, auto-fill in JS mode will sometimes
> insert a "/*" on a filled line, where it should not.
>
> You can see a precise recipe in the new test case, in the attached
> patch.
>
> One funny thing is that if you do this interactively (in my real-world
> case I had auto-fill-mode enabled), you have to be sure that there isn't
> a newline after point.  If there is a newline, the filling works
> properly.
>
> Anyway, let me know what you think.  I'd like to check this in, but,
> given the comments in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=6806#23
> I suppose I would not close the bug.

Sorry I missed this before you pushed the patch, but I'm not sure this
is a bug.  If I revert the setting of `comment-multi-line' then with
your test scenario, the result of auto fill is

    /* test test test test test test test test test test test test test */
    /* test test test 

So the opened comment was correctly terminated, and a new one started,
just as documented.  Perhaps you don't like that `comment-multi-line' is
nil by default, but it doesn't look like a bug to me.

bug#6806: another reason for the patch

[npostavs]

found 6806 24.5
fixed 6806 25.1
quit

npostavs@users.sourceforge.net writes:
>
>  If I revert the setting of `comment-multi-line' then with
> your test scenario, the result of auto fill is
>
>     /* test test test test test test test test test test test test test */
>     /* test test test 

Up until 24.5 the result is

    /* test test test test test test test test test test test test test
       /* test test test 

So this bug has been fixed in 25.1.

bug#6806: another reason for the patch

[Tom Tromey]

Noam> So the opened comment was correctly terminated, and a new one started,
Noam> just as documented.  Perhaps you don't like that `comment-multi-line' is
Noam> nil by default, but it doesn't look like a bug to me.

I'm not sure what to do about this now.
Should I revert the patch?

If it were solely up to me I would leave the patch in, since I think
non-nil comment-multi-line is more normal in JS code.  And, if the patch
is reverted then I think comment-multi-line needs a :safe setting so it
can by set in .dir-locals.el.

Tom

bug#6806: another reason for the patch

[npostavs]

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

Tom Tromey <tom@tromey.com> writes:

> Noam> So the opened comment was correctly terminated, and a new one started,
> Noam> just as documented.  Perhaps you don't like that `comment-multi-line' is
> Noam> nil by default, but it doesn't look like a bug to me.
>
> I'm not sure what to do about this now.
> Should I revert the patch?
>
> If it were solely up to me I would leave the patch in, since I think
> non-nil comment-multi-line is more normal in JS code.  And, if the patch
> is reverted then I think comment-multi-line needs a :safe setting so it
> can by set in .dir-locals.el.

I don't really have much of an opinion regarding the best value for
comment-multi-line.  But we should test that filling works correctly
regardless of its value, and we should add a :safe setting anyway.  Here
is a patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 2160 bytes --]

From cbf522f2446035bcd34a676c2ef3e641b4f20f90 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 11 Feb 2017 23:15:13 -0500
Subject: [PATCH] Test comment-multi-line = nil auto fill case too

* test/lisp/progmodes/js-tests.el (js-mode-auto-fill): Test with
`comment-multi-line' both nil and non-nil.
* lisp/newcomment.el (comment-multi-line): Mark safe if it's a
boolean.
---
 lisp/newcomment.el              |  1 +
 test/lisp/progmodes/js-tests.el | 22 ++++++++++++----------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/lisp/newcomment.el b/lisp/newcomment.el
index 1af89293b6..4b261c34c6 100644
--- a/lisp/newcomment.el
+++ b/lisp/newcomment.el
@@ -309,6 +309,7 @@ comment-multi-line
 It also affects \\[indent-new-comment-line].  However, if you want this
 behavior for explicit filling, you might as well use \\[newline-and-indent]."
   :type 'boolean
+  :safe #'booleanp
   :group 'comment)
 
 (defcustom comment-empty-lines nil
diff --git a/test/lisp/progmodes/js-tests.el b/test/lisp/progmodes/js-tests.el
index d61f084e0d..99f5898525 100644
--- a/test/lisp/progmodes/js-tests.el
+++ b/test/lisp/progmodes/js-tests.el
@@ -89,16 +89,18 @@
 (ert-deftest js-mode-auto-fill ()
   (with-temp-buffer
     (js-mode)
-    (setq fill-column 70)
-    (insert "/* ")
-    (dotimes (_ 16)
-      (insert "test "))
-    (do-auto-fill)
-    ;; The bug is that, after auto-fill, the second line starts with
-    ;; "/*", whereas it should start with " * ".
-    (goto-char (point-min))
-    (forward-line)
-    (should (looking-at " \\* test"))))
+    (let ((fill-column 10)
+          (comment-multi-line t))
+      (insert "/* test test")
+      (do-auto-fill)
+      ;; Filling should continue the multi line comment.
+      (should (equal (buffer-string) "/* test\n * test"))
+      (erase-buffer)
+      (insert "/* test test")
+      (setq comment-multi-line nil)
+      (do-auto-fill)
+      ;; Filling should start a new comment on the next line.
+      (should (equal (buffer-string) "/* test */\n/* test")))))
 
 (ert-deftest js-mode-regexp-syntax-bug-25529 ()
   (dolist (regexp-contents '("[^[]"
-- 
2.11.1


[-- Attachment #3: Type: text/plain, Size: 500 bytes --]


Perhaps we should also revert the js-mode comment-multi-line setting,
not sure.

--- i/lisp/progmodes/js.el
+++ w/lisp/progmodes/js.el
@@ -3856,7 +3856,6 @@ js-mode
         comment-start-skip "\\(//+\\|/\\*+\\)\\s *")
   (setq-local comment-line-break-function #'c-indent-new-comment-line)
   (setq-local c-block-comment-start-regexp "/\\*")
-  (setq-local comment-multi-line t)
 
   (setq-local electric-indent-chars
 	      (append "{}():;," electric-indent-chars)) ;FIXME: js2-mode adds "[]*".

bug#6806: another reason for the patch

[Tom Tromey]

>>>>> "Noam" == Noam Postavsky <npostavs@users.sourceforge.net> writes:

Noam> Perhaps we should also revert the js-mode comment-multi-line setting,
Noam> not sure.

My belief is that more JS code follows the comment-multi-line style than
follows the other style.

Tom

bug#6806: another reason for the patch

[npostavs]

close 6806
quit

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Noam" == Noam Postavsky <npostavs@users.sourceforge.net> writes:
>
> Noam> Perhaps we should also revert the js-mode comment-multi-line setting,
> Noam> not sure.
>
> My belief is that more JS code follows the comment-multi-line style than
> follows the other style.

Okay, I pushed the patch without changing comment-multi-line back [1:
0a64666288]; I added a mention about the new comment-multi-line setting
in NEWS, we'll see if anyone objects.

1: 2017-02-14 22:29:56 -0500 0a64666288e3f32967db4ad683a4bc2f225fb952
  Test comment-multi-line = nil auto fill case too

bug#6806: checked in the patch

[Tom Tromey]

I went ahead and checked in the patch, mostly because comment #20 said
it was fine, and also because it fixed a bug.

I think this bug should be left open, though, due to the other issues
identified.

Tom