bug#20256: 25.0.50; css-mode: filling multi-line comments

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Simen Heggestøyl]

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

Filling multi-line comments in `css-mode' is broken. Consider for
instance the following comment:

  /*
   * Multi-line comment here.
   * This comment spans
   * multiple
   * lines.
   * Better fill it!
   */

Filling it with a `fill-column' of 70, I'd expect the following
result:

  /*
   * Multi-line comment here.  This comment spans multiple lines.
   * Better fill it!
   */

But as of now, what we get is this:

  /* * Multi-line comment here.  * This comment spans * multiple *
  lines.  * Better fill it!  */

 From this comment in `css-fill-paragraph', it sounds like it is
supposed to work, or has been working some time before:

  ;; Filling inside a comment whose comment-end marker is not \n.
  ;; This code is meant to be generic, so that it works not only for
  ;; css-mode but for all modes.

Could someone fill me in?

Alternatively, we could let `css-mode' borrow comment filling code
from `cc-mode', like `js-mode' and `js2-mode' do. Please let me know
if that would be an acceptable fix.

-- Simen

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

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Stefan Monnier]

> From this comment in `css-fill-paragraph', it sounds like it is
> supposed to work, or has been working some time before:

>  ;; Filling inside a comment whose comment-end marker is not \n.
>  ;; This code is meant to be generic, so that it works not only for
>  ;; css-mode but for all modes.

> Could someone fill me in?

It is "work in progress", so it does work in some cases (e.g. if you
add a "*" line as in:

 /*
  *
  * Multi-line comment here.
  * This comment spans
  * multiple
  * lines.
  * Better fill it!
  */

).  The patch below seems to fix one half of the problem (tho it's
probably better to set adaptive-fill-function buffer-locally rather
than let-bind it, this was just a quick-hack).  Tweaking the
paragraph-separate regexp (so as to recognize the "/*" line as
a paragraph separator) should let you fix the second.


        Stefan


diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 7280080..c09245d 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -377,7 +377,7 @@ pseudo-classes, and at-rules."
   (setq-local comment-start-skip "/\\*+[ \t]*")
   (setq-local comment-end "*/")
   (setq-local comment-end-skip "[ \t]*\\*+/")
-  (setq-local fill-paragraph-function 'css-fill-paragraph)
+  (setq-local fill-paragraph-function #'css-fill-paragraph)
   (setq-local add-log-current-defun-function #'css-current-defun-name)
   (smie-setup css-smie-grammar #'css-smie-rules
               :forward-token #'css-smie--forward-token
@@ -418,7 +418,13 @@ pseudo-classes, and at-rules."
                           (string-match "[^ \t]" comment-continue))
                      (concat "\\(?:[ \t]*" (regexp-quote comment-continue)
                              "\\)?\\(?:" paragraph-start "\\)")
-                   paragraph-start)))
+                   paragraph-start))
+		(adaptive-fill-function
+		 (lambda ()
+		   (when (looking-at "[ \t]*/\\*[ \t]*")
+		     (let ((str (match-string 0)))
+		       (and (string-match "/\\*" str)
+			    (replace-match " *" t t str)))))))
             (fill-paragraph justify)
             ;; Don't try filling again.
             t)))

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Simen Heggestøyl]

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

Thanks for the hints, Stefan!

The following patch seems to work well for me, both with css-mode and
scss-mode:


 From 16b46e34bb3e0e69a039e4ed0737013aa9a06f86 Mon Sep 17 00:00:00 2001
 From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Thu, 9 Apr 2015 19:09:04 +0200
Subject: [PATCH] css-mode.el: Support multi-line comment filling

Fixes: debbugs:20256

* css-mode.el (css-fill-paragraph): Support multi-line comment
filling.
---
 lisp/textmodes/css-mode.el | 57 
++++++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 851618c..f452e17 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -381,7 +381,7 @@ pseudo-classes, and at-rules."
   (setq-local comment-start-skip "/\\*+[ \t]*")
   (setq-local comment-end "*/")
   (setq-local comment-end-skip "[ \t]*\\*+/")
-  (setq-local fill-paragraph-function 'css-fill-paragraph)
+  (setq-local fill-paragraph-function #'css-fill-paragraph)
   (setq-local add-log-current-defun-function #'css-current-defun-name)
   (smie-setup css-smie-grammar #'css-smie-rules
               :forward-token #'css-smie--forward-token
@@ -395,6 +395,10 @@ pseudo-classes, and at-rules."

 (defun css-fill-paragraph (&optional justify)
   (save-excursion
+    ;; Fill succeeding comment when invoked at the beginning of a
+    ;; multi-line comment.
+    (when (save-excursion (back-to-indentation) (looking-at "/\\*"))
+      (skip-chars-forward " \t/*"))
     (let ((ppss (syntax-ppss))
           (eol (line-end-position)))
       (cond
@@ -408,24 +412,39 @@ pseudo-classes, and at-rules."
         ;; This code is meant to be generic, so that it works not only 
for
         ;; css-mode but for all modes.
         (save-restriction
-          (narrow-to-region (nth 8 ppss) eol)
-          (comment-normalize-vars)      ;Will define comment-continue.
-          (let ((fill-paragraph-function nil)
-                (paragraph-separate
-                 (if (and comment-continue
-                          (string-match "[^ \t]" comment-continue))
-                     (concat "\\(?:[ \t]*" (regexp-quote 
comment-continue)
-                             "\\)?\\(?:" paragraph-separate "\\)")
-                   paragraph-separate))
-                (paragraph-start
-                 (if (and comment-continue
-                          (string-match "[^ \t]" comment-continue))
-                     (concat "\\(?:[ \t]*" (regexp-quote 
comment-continue)
-                             "\\)?\\(?:" paragraph-start "\\)")
-                   paragraph-start)))
-            (fill-paragraph justify)
-            ;; Don't try filling again.
-            t)))
+          ;; Ensure that multi-line variants of `comment-start' and
+          ;; `comment-end' are in use, in order to support multi-line
+          ;; comment filling in SCSS mode as well.
+          (let ((comment-start "/*")
+                (comment-end "*/"))
+            (narrow-to-region (nth 8 ppss) eol)
+            (comment-normalize-vars)    ;Will define comment-continue.
+            (let ((fill-paragraph-function nil)
+                  (paragraph-separate
+                   (if (and comment-continue
+                            (string-match "[^ \t]" comment-continue))
+                       (concat "\\(?:[ \t]*"
+                               (regexp-opt
+                                (list comment-continue comment-start
+                                      comment-end))
+                               "\\)?\\(?:" paragraph-separate "\\)")
+                     paragraph-separate))
+                  (paragraph-start
+                   (if (and comment-continue
+                            (string-match "[^ \t]" comment-continue))
+                       (concat "\\(?:[ \t]*"
+                               (regexp-quote comment-continue)
+                               "\\)?\\(?:" paragraph-start "\\)")
+                     paragraph-start))
+                  (adaptive-fill-function
+                   (lambda ()
+                     (when (looking-at "[ \t]*/\\*[ \t]*")
+                       (let ((str (match-string 0)))
+                         (and (string-match "/\\*" str)
+                              (replace-match " *" t t str)))))))
+              (fill-paragraph justify)
+              ;; Don't try filling again.
+              t))))

        ((and (null (nth 8 ppss))
              (or (nth 1 ppss)
-- 
2.1.4

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

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Stefan Monnier]

> +                  (adaptive-fill-function
> +                   (lambda ()
> +                     (when (looking-at "[ \t]*/\\*[ \t]*")
> +                       (let ((str (match-string 0)))
> +                         (and (string-match "/\\*" str)
> +                              (replace-match " *" t t str)))))))

As mentioned, I think this had better be a buffer-local setting, rather
than a let-binding.  After all, it would be good if it also works for
auto-fill-mode.

Also this patch makes css-fill-paragraph non-generic (IOW it now only
works when comment-start is "/*"), so the comment:

        ;; This code is meant to be generic, so that it works not only for
        ;; css-mode but for all modes.

needs to be updated.  Of course, even better would be to avoid
hardcoding /* and */ and use comment-start/end/continue instead.


        Stefan

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Simen Heggestøyl]

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

> As mentioned, I think this had better be a buffer-local setting, 
> rather
> than a let-binding.  After all, it would be good if it also works for
> auto-fill-mode.

Ah, thanks, I missed that. An updated patch follows. It defines
`css-adaptive-fill' buffer-locally, which makes `auto-fill-mode'
behave nice. `css-fill-paragraph' should now be general again.


 From 4e46637ceeab0a0a266cac035204f6db798fbd38 Mon Sep 17 00:00:00 2001
 From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Thu, 9 Apr 2015 19:09:04 +0200
Subject: [PATCH] css-mode.el: Support multi-line comment filling

Fixes: debbugs:20256

* lisp/textmodes/css-mode.el (css-fill-paragraph): Support multi-line
comment filling.
(css-adaptive-fill): New function.
(css-mode): Set `adaptive-fill-function'.
(scss-fill-paragraph): New function.
(scss-mode): Set `fill-paragraph-function'.
---
 lisp/textmodes/css-mode.el | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index d1893a3..b4de1a5 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -381,7 +381,8 @@ pseudo-classes, and at-rules."
   (setq-local comment-start-skip "/\\*+[ \t]*")
   (setq-local comment-end "*/")
   (setq-local comment-end-skip "[ \t]*\\*+/")
-  (setq-local fill-paragraph-function 'css-fill-paragraph)
+  (setq-local fill-paragraph-function #'css-fill-paragraph)
+  (setq-local adaptive-fill-function #'css-adaptive-fill)
   (setq-local add-log-current-defun-function #'css-current-defun-name)
   (smie-setup css-smie-grammar #'css-smie-rules
               :forward-token #'css-smie--forward-token
@@ -395,6 +396,12 @@ pseudo-classes, and at-rules."

 (defun css-fill-paragraph (&optional justify)
   (save-excursion
+    ;; Fill succeeding comment when invoked right before a multi-line
+    ;; comment.
+    (when (save-excursion
+            (back-to-indentation)
+            (looking-at (regexp-quote comment-start)))
+      (goto-char (match-end 0)))
     (let ((ppss (syntax-ppss))
           (eol (line-end-position)))
       (cond
@@ -414,7 +421,10 @@ pseudo-classes, and at-rules."
                 (paragraph-separate
                  (if (and comment-continue
                           (string-match "[^ \t]" comment-continue))
-                     (concat "\\(?:[ \t]*" (regexp-quote 
comment-continue)
+                     (concat "\\(?:[ \t]*"
+                             (regexp-opt
+                              (list comment-continue comment-start
+                                    comment-end))
                              "\\)?\\(?:" paragraph-separate "\\)")
                    paragraph-separate))
                 (paragraph-start
@@ -468,6 +478,12 @@ pseudo-classes, and at-rules."
             ;; Don't use the default filling code.
             t)))))))

+(defun css-adaptive-fill ()
+  (when (looking-at "[ \t]*/\\*[ \t]*")
+    (let ((str (match-string 0)))
+      (and (string-match "/\\*" str)
+           (replace-match " *" t t str)))))
+
 (defun css-current-defun-name ()
   "Return the name of the CSS section at point, or nil."
   (save-excursion
@@ -506,7 +522,17 @@ pseudo-classes, and at-rules."
   (setq-local comment-end "")
   (setq-local comment-start-skip "/[*/]+[ \t]*")
   (setq-local comment-end-skip "[ \t]*\\(?:\n\\|\\*+/\\)")
+  (setq-local fill-paragraph-function #'scss-fill-paragraph)
   (setq-local font-lock-defaults '(scss-font-lock-keywords nil t)))

+(defun scss-fill-paragraph (&optional justify)
+  "Call `css-fill-paragraph', but ensure that the multi-line
+   variants of `comment-start' and `comment-end' are in use, in
+   order to support multi-line comment filling in SCSS mode as
+   well."
+  (let ((comment-start "/*")
+        (comment-end "*/"))
+    (css-fill-paragraph justify)))
+
 (provide 'css-mode)
 ;;; css-mode.el ends here
-- 
2.1.4



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

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Stefan Monnier]

> Ah, thanks, I missed that. An updated patch follows. It defines
> `css-adaptive-fill' buffer-locally, which makes `auto-fill-mode'
> behave nice.

Thanks, looks good.

> `css-fill-paragraph' should now be general again.

Not quite: for example a comment starter may fail to match (regexp-quote
comment-start), in modes where comments can be of several different
forms (e.g. some modes might accept both (*...*) and /*...*/ for example).

> +    (when (save-excursion
> +            (back-to-indentation)
> +            (looking-at (regexp-quote comment-start)))

This should probably use comment-start-skip (or comment-search-forward).

> -                     (concat "\\(?:[ \t]*" (regexp-quote comment-continue)
> +                     (concat "\\(?:[ \t]*"
> +                             (regexp-opt
> +                              (list comment-continue comment-start
> +                                    comment-end))

I think this should not use comment-end if it's the empty string
(otherwise the regexp might trigger pathological behavior of our
backtracking-based matcher).
Maybe better would be to use comment-start-skip and comment-end-skip
rather than comment-start and comment-end.

> +(defun scss-fill-paragraph (&optional justify)
> +  "Call `css-fill-paragraph', but ensure that the multi-line
> +   variants of `comment-start' and `comment-end' are in use, in
> +   order to support multi-line comment filling in SCSS mode as
> +   well."
> +  (let ((comment-start "/*")
> +        (comment-end "*/"))
> +    (css-fill-paragraph justify)))

Once you fix the code to use comment-start/end-skip instead of
comment-start/end, this function should be unneeded.


        Stefan

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Simen Heggestøyl]

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

Thanks again for the feedback, Stefan! An updated patch follows.

-- Simen


 From 839810c2fc79d64634be2d27148df9f0759e1c8b Mon Sep 17 00:00:00 2001
 From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Thu, 9 Apr 2015 19:09:04 +0200
Subject: [PATCH] css-mode.el: Support multi-line comment filling

Fixes: debbugs:20256

* lisp/textmodes/css-mode.el (css-fill-paragraph): Support multi-line
comment filling.
(css-adaptive-fill): New function.
(css-mode): Set `adaptive-fill-function'.
(scss-mode): Set `comment-continue'.
---
 lisp/textmodes/css-mode.el | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index d1893a3..424cdb7 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -381,7 +381,8 @@ pseudo-classes, and at-rules."
   (setq-local comment-start-skip "/\\*+[ \t]*")
   (setq-local comment-end "*/")
   (setq-local comment-end-skip "[ \t]*\\*+/")
-  (setq-local fill-paragraph-function 'css-fill-paragraph)
+  (setq-local fill-paragraph-function #'css-fill-paragraph)
+  (setq-local adaptive-fill-function #'css-adaptive-fill)
   (setq-local add-log-current-defun-function #'css-current-defun-name)
   (smie-setup css-smie-grammar #'css-smie-rules
               :forward-token #'css-smie--forward-token
@@ -395,6 +396,12 @@ pseudo-classes, and at-rules."

 (defun css-fill-paragraph (&optional justify)
   (save-excursion
+    ;; Fill succeeding comment when invoked right before a multi-line
+    ;; comment.
+    (when (save-excursion
+            (beginning-of-line)
+            (comment-search-forward (point-at-eol) t))
+      (goto-char (match-end 0)))
     (let ((ppss (syntax-ppss))
           (eol (line-end-position)))
       (cond
@@ -414,8 +421,11 @@ pseudo-classes, and at-rules."
                 (paragraph-separate
                  (if (and comment-continue
                           (string-match "[^ \t]" comment-continue))
-                     (concat "\\(?:[ \t]*" (regexp-quote 
comment-continue)
-                             "\\)?\\(?:" paragraph-separate "\\)")
+                     (concat "\\(?:[ \t]*\\(?:"
+                             (regexp-quote comment-continue) "\\|"
+                             comment-start-skip "\\|"
+                             comment-end-skip "\\)\\)?"
+                             "\\(?:" paragraph-separate "\\)")
                    paragraph-separate))
                 (paragraph-start
                  (if (and comment-continue
@@ -468,6 +478,12 @@ pseudo-classes, and at-rules."
             ;; Don't use the default filling code.
             t)))))))

+(defun css-adaptive-fill ()
+  (when (looking-at "[ \t]*/\\*[ \t]*")
+    (let ((str (match-string 0)))
+      (and (string-match "/\\*" str)
+           (replace-match " *" t t str)))))
+
 (defun css-current-defun-name ()
   "Return the name of the CSS section at point, or nil."
   (save-excursion
@@ -504,6 +520,7 @@ pseudo-classes, and at-rules."
   "Major mode to edit \"Sassy CSS\" files."
   (setq-local comment-start "// ")
   (setq-local comment-end "")
+  (setq-local comment-continue " *")
   (setq-local comment-start-skip "/[*/]+[ \t]*")
   (setq-local comment-end-skip "[ \t]*\\(?:\n\\|\\*+/\\)")
   (setq-local font-lock-defaults '(scss-font-lock-keywords nil t)))
-- 
2.1.4

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

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Stefan Monnier]

> Thanks again for the feedback, Stefan! An updated patch follows.

Thank you.  Looks good to me.
Maybe we should even try to move this fill function to prog-mode.el.


        Stefan


> From 839810c2fc79d64634be2d27148df9f0759e1c8b Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
> Date: Thu, 9 Apr 2015 19:09:04 +0200
> Subject: [PATCH] css-mode.el: Support multi-line comment filling

> Fixes: debbugs:20256

> * lisp/textmodes/css-mode.el (css-fill-paragraph): Support multi-line
> comment filling.
> (css-adaptive-fill): New function.
> (css-mode): Set `adaptive-fill-function'.
> (scss-mode): Set `comment-continue'.
> ---
> lisp/textmodes/css-mode.el | 23 ++++++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)

> diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
> index d1893a3..424cdb7 100644
> --- a/lisp/textmodes/css-mode.el
> +++ b/lisp/textmodes/css-mode.el
> @@ -381,7 +381,8 @@ pseudo-classes, and at-rules."
>   (setq-local comment-start-skip "/\\*+[ \t]*")
>   (setq-local comment-end "*/")
>   (setq-local comment-end-skip "[ \t]*\\*+/")
> -  (setq-local fill-paragraph-function 'css-fill-paragraph)
> +  (setq-local fill-paragraph-function #'css-fill-paragraph)
> +  (setq-local adaptive-fill-function #'css-adaptive-fill)
>   (setq-local add-log-current-defun-function #'css-current-defun-name)
>   (smie-setup css-smie-grammar #'css-smie-rules
>               :forward-token #'css-smie--forward-token
> @@ -395,6 +396,12 @@ pseudo-classes, and at-rules."

> (defun css-fill-paragraph (&optional justify)
>   (save-excursion
> +    ;; Fill succeeding comment when invoked right before a multi-line
> +    ;; comment.
> +    (when (save-excursion
> +            (beginning-of-line)
> +            (comment-search-forward (point-at-eol) t))
> +      (goto-char (match-end 0)))
>     (let ((ppss (syntax-ppss))
>           (eol (line-end-position)))
>       (cond
> @@ -414,8 +421,11 @@ pseudo-classes, and at-rules."
>                 (paragraph-separate
>                  (if (and comment-continue
>                           (string-match "[^ \t]" comment-continue))
> -                     (concat "\\(?:[ \t]*" (regexp-quote comment-continue)
> -                             "\\)?\\(?:" paragraph-separate "\\)")
> +                     (concat "\\(?:[ \t]*\\(?:"
> +                             (regexp-quote comment-continue) "\\|"
> +                             comment-start-skip "\\|"
> +                             comment-end-skip "\\)\\)?"
> +                             "\\(?:" paragraph-separate "\\)")
>                    paragraph-separate))
>                 (paragraph-start
>                  (if (and comment-continue
> @@ -468,6 +478,12 @@ pseudo-classes, and at-rules."
>             ;; Don't use the default filling code.
>             t)))))))

> +(defun css-adaptive-fill ()
> +  (when (looking-at "[ \t]*/\\*[ \t]*")
> +    (let ((str (match-string 0)))
> +      (and (string-match "/\\*" str)
> +           (replace-match " *" t t str)))))
> +
> (defun css-current-defun-name ()
>   "Return the name of the CSS section at point, or nil."
>   (save-excursion
> @@ -504,6 +520,7 @@ pseudo-classes, and at-rules."
>   "Major mode to edit \"Sassy CSS\" files."
>   (setq-local comment-start "// ")
>   (setq-local comment-end "")
> +  (setq-local comment-continue " *")
>   (setq-local comment-start-skip "/[*/]+[ \t]*")
>   (setq-local comment-end-skip "[ \t]*\\(?:\n\\|\\*+/\\)")
>   (setq-local font-lock-defaults '(scss-font-lock-keywords nil t)))
> -- 
> 2.1.4

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Simen Heggestøyl]

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

On Sat, Apr 18, 2015 at 4:06 PM, Stefan Monnier 
<monnier@IRO.UMontreal.CA> wrote:
> Thank you.  Looks good to me.

Installed.

> Maybe we should even try to move this fill function to prog-mode.el.

Sounds good!

-- Simen

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

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Stefan Monnier]

>> Thank you.  Looks good to me.
> Installed.

Thanks.

>> Maybe we should even try to move this fill function to prog-mode.el.
> Sounds good!

Would you like to do it?  I think all it would take is to move the code
there, set it in prog-mode (so that modes that derive from prog-mode get
to use this new code by default) and then make a few random tests in
various major modes (mostly those that don't themselves set
fill-paragraph-function).

BTW, could add a couple tests to test/automated, to try and make
sure your use-case is remembered?


        Stefan

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Simen Heggestøyl]

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

On Mon, Apr 20, 2015 at 6:09 PM, Stefan Monnier 
<monnier@iro.umontreal.ca> wrote:
> Would you like to do it?  I think all it would take is to move the 
> code
> there, set it in prog-mode (so that modes that derive from prog-mode 
> get
> to use this new code by default) and then make a few random tests in
> various major modes (mostly those that don't themselves set
> fill-paragraph-function).
> 
> BTW, could add a couple tests to test/automated, to try and make
> sure your use-case is remembered?

Sure! Unless it's time-critical to get this done. My spare time
varies, but if I can do it in my own tempo, I can surely look into
it. :)

I've already spotted out js-mode as a candidate to make use of the new
code. It's currently borrowing comment filling code from cc-mode,
which results in bugs such as #19399.

-- Simen

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

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Stefan Monnier]

> Sure! Unless it's time-critical to get this done.  My spare time
> varies, but if I can do it in my own tempo, I can surely look into
> it. :)

It's been on my todo list for a few years already, so time-critical
it isn't.


        Stefan

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Simen Heggestøyl]

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

On Mon, Apr 20, 2015 at 6:09 PM, Stefan Monnier 
<monnier@iro.umontreal.ca> wrote:
> Would you like to do it?  I think all it would take is to move the 
> code
> there, set it in prog-mode (so that modes that derive from prog-mode 
> get
> to use this new code by default) and then make a few random tests in
> various major modes (mostly those that don't themselves set
> fill-paragraph-function).

Something along these lines?


 From 4b9c83f99c34fbe6a9b66dfbf9425f3daba2c8cd Mon Sep 17 00:00:00 2001
 From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Sat, 25 Apr 2015 19:19:08 +0200
Subject: [PATCH] Set default `fill-paragraph-function' in prog-mode

* lisp/progmodes/prog-mode.el (prog-fill-paragraph): New
function. Intended to be the default `fill-paragraph-function' for
modes that derive from prog-mode. Grown in css-mode, but now factored
out to prog-mode.
* lisp/progmodes/prog-mode.el (prog-mode): Set
`fill-paragraph-function'.
* lisp/textmodes/css-mode.el (css-fill-paragraph): Factor out code for
filling multi-line comments.
---
 lisp/progmodes/prog-mode.el |  46 ++++++++++++++++
 lisp/textmodes/css-mode.el  | 126 
++++++++++++++++----------------------------
 2 files changed, 91 insertions(+), 81 deletions(-)

diff --git a/lisp/progmodes/prog-mode.el b/lisp/progmodes/prog-mode.el
index 0d9fabd..b64a3ee 100644
--- a/lisp/progmodes/prog-mode.el
+++ b/lisp/progmodes/prog-mode.el
@@ -145,11 +145,57 @@ support it."
 (define-globalized-minor-mode global-prettify-symbols-mode
   prettify-symbols-mode turn-on-prettify-symbols-mode)

+(defvar comment-continue)
+
+(defun prog-fill-paragraph (&optional justify)
+  (save-excursion
+    ;; Fill succeeding comment when invoked right before a multi-line
+    ;; comment.
+    (when (save-excursion
+            (beginning-of-line)
+            (comment-search-forward (point-at-eol) t))
+      (goto-char (match-end 0)))
+    (let ((ppss (syntax-ppss))
+          (eol (line-end-position)))
+      (when (and (nth 4 ppss)
+                 (save-excursion
+                   (goto-char (nth 8 ppss))
+                   (forward-comment 1)
+                   (prog1 (not (bolp))
+                     (setq eol (point)))))
+        ;; Filling inside a comment whose comment-end marker is not
+        ;; \n.  This code is meant to be generic, so that it works for
+        ;; all modes.
+        (save-restriction
+          (narrow-to-region (nth 8 ppss) eol)
+          (comment-normalize-vars)     ; Will define comment-continue.
+          (let ((fill-paragraph-function nil)
+                (paragraph-separate
+                 (if (and comment-continue
+                          (string-match "[^ \t]" comment-continue))
+                     (concat "\\(?:[ \t]*\\(?:"
+                             (regexp-quote comment-continue) "\\|"
+                             comment-start-skip "\\|"
+                             comment-end-skip "\\)\\)?"
+                             "\\(?:" paragraph-separate "\\)")
+                   paragraph-separate))
+                (paragraph-start
+                 (if (and comment-continue
+                          (string-match "[^ \t]" comment-continue))
+                     (concat "\\(?:[ \t]*"
+                             (regexp-quote comment-continue)
+                             "\\)?\\(?:" paragraph-start "\\)")
+                   paragraph-start)))
+            (fill-paragraph justify)
+            ;; Don't try filling again.
+            t))))))
+
 ;;;###autoload
 (define-derived-mode prog-mode fundamental-mode "Prog"
   "Major mode for editing programming language source code."
   (setq-local require-final-newline mode-require-final-newline)
   (setq-local parse-sexp-ignore-comments t)
+  (setq-local fill-paragraph-function #'prog-fill-paragraph)
   ;; Any programming language is always written left to right.
   (setq bidi-paragraph-direction 'left-to-right))

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 424cdb7..451688a 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -393,90 +393,54 @@ pseudo-classes, and at-rules."
             #'css-completion-at-point nil 'local))

 (defvar comment-continue)
+(declare-function prog-fill-paragraph "prog-mode" (&optional justify))

 (defun css-fill-paragraph (&optional justify)
-  (save-excursion
-    ;; Fill succeeding comment when invoked right before a multi-line
-    ;; comment.
-    (when (save-excursion
-            (beginning-of-line)
-            (comment-search-forward (point-at-eol) t))
-      (goto-char (match-end 0)))
-    (let ((ppss (syntax-ppss))
-          (eol (line-end-position)))
-      (cond
-       ((and (nth 4 ppss)
-             (save-excursion
-               (goto-char (nth 8 ppss))
-               (forward-comment 1)
-               (prog1 (not (bolp))
-                 (setq eol (point)))))
-        ;; Filling inside a comment whose comment-end marker is not \n.
-        ;; This code is meant to be generic, so that it works not only 
for
-        ;; css-mode but for all modes.
-        (save-restriction
-          (narrow-to-region (nth 8 ppss) eol)
-          (comment-normalize-vars)      ;Will define comment-continue.
-          (let ((fill-paragraph-function nil)
-                (paragraph-separate
-                 (if (and comment-continue
-                          (string-match "[^ \t]" comment-continue))
-                     (concat "\\(?:[ \t]*\\(?:"
-                             (regexp-quote comment-continue) "\\|"
-                             comment-start-skip "\\|"
-                             comment-end-skip "\\)\\)?"
-                             "\\(?:" paragraph-separate "\\)")
-                   paragraph-separate))
-                (paragraph-start
-                 (if (and comment-continue
-                          (string-match "[^ \t]" comment-continue))
-                     (concat "\\(?:[ \t]*" (regexp-quote 
comment-continue)
-                             "\\)?\\(?:" paragraph-start "\\)")
-                   paragraph-start)))
-            (fill-paragraph justify)
-            ;; Don't try filling again.
-            t)))
-
-       ((and (null (nth 8 ppss))
-             (or (nth 1 ppss)
-                 (and (ignore-errors
-                        (down-list 1)
-                        (when (<= (point) eol)
-                          (setq ppss (syntax-ppss)))))))
-        (goto-char (nth 1 ppss))
-        (let ((end (save-excursion
-                     (ignore-errors (forward-sexp 1) (copy-marker 
(point) t)))))
-          (when end
-            (while (re-search-forward "[{;}]" end t)
-              (cond
-               ;; This is a false positive inside a string or comment.
-               ((nth 8 (syntax-ppss)) nil)
-               ;; This is a false positive when encountering an
-               ;; interpolated variable (bug#19751).
-               ((eq (char-before (- (point) 1)) ?#) nil)
-               ((eq (char-before) ?\})
-                (save-excursion
-                  (forward-char -1)
-                  (skip-chars-backward " \t")
-                  (when (and (not (bolp))
-                             (scss-smie--not-interpolation-p))
-                    (newline))))
-               (t
-                (while
-                    (progn
-                      (setq eol (line-end-position))
-                      (and (forward-comment 1)
-                           (> (point) eol)
-                           ;; A multi-line comment should be on its 
own line.
-                           (save-excursion (forward-comment -1)
-                                           (when (< (point) eol)
-                                             (newline)
-                                             t)))))
-                (if (< (point) eol) (newline)))))
+  (or (prog-fill-paragraph justify)
+      (save-excursion
+        (let ((ppss (syntax-ppss))
+              (eol (line-end-position)))
+          (cond
+           ((and (null (nth 8 ppss))
+                 (or (nth 1 ppss)
+                     (and (ignore-errors
+                            (down-list 1)
+                            (when (<= (point) eol)
+                              (setq ppss (syntax-ppss)))))))
             (goto-char (nth 1 ppss))
-            (indent-region (line-beginning-position 2) end)
-            ;; Don't use the default filling code.
-            t)))))))
+            (let ((end (save-excursion
+                         (ignore-errors (forward-sexp 1) (copy-marker 
(point) t)))))
+              (when end
+                (while (re-search-forward "[{;}]" end t)
+                  (cond
+                   ;; This is a false positive inside a string or 
comment.
+                   ((nth 8 (syntax-ppss)) nil)
+                   ;; This is a false positive when encountering an
+                   ;; interpolated variable (bug#19751).
+                   ((eq (char-before (- (point) 1)) ?#) nil)
+                   ((eq (char-before) ?\})
+                    (save-excursion
+                      (forward-char -1)
+                      (skip-chars-backward " \t")
+                      (when (and (not (bolp))
+                                 (scss-smie--not-interpolation-p))
+                        (newline))))
+                   (t
+                    (while
+                        (progn
+                          (setq eol (line-end-position))
+                          (and (forward-comment 1)
+                               (> (point) eol)
+                               ;; A multi-line comment should be on 
its own line.
+                               (save-excursion (forward-comment -1)
+                                               (when (< (point) eol)
+                                                 (newline)
+                                                 t)))))
+                    (if (< (point) eol) (newline)))))
+                (goto-char (nth 1 ppss))
+                (indent-region (line-beginning-position 2) end)
+                ;; Don't use the default filling code.
+                t))))))))

 (defun css-adaptive-fill ()
   (when (looking-at "[ \t]*/\\*[ \t]*")
-- 
2.1.4



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

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Stefan Monnier]

> +(defun prog-fill-paragraph (&optional justify)
[...]
> +        ;; Filling inside a comment whose comment-end marker is not
> +        ;; \n.  This code is meant to be generic, so that it works for
> +        ;; all modes.

Hmm... so the code doesn't handle the case of comments that end with \n?


        Stefan

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Simen Heggestøyl]

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

On Mon, Apr 27, 2015 at 6:11 AM, Stefan Monnier 
<monnier@iro.umontreal.ca> wrote:
>>  +(defun prog-fill-paragraph (&optional justify)
> [...]
>>  +        ;; Filling inside a comment whose comment-end marker is not
>>  +        ;; \n.  This code is meant to be generic, so that it works 
>> for
>>  +        ;; all modes.
> 
> Hmm... so the code doesn't handle the case of comments that end with 
> \n?
> 
> 
>         Stefan

No, I don't think so. When the comment-end marker is \n,
`forward-comment' will leave point at the beginning of the next line,
causing the whole filling code to be skipped. Even when tweaking the
predicate so that the code is run anyway, it doesn't fill the comment
properly. Was it ever intended to do so? I assumed from the comment
that you quoted that it wasn't.

Those kinds of comments seem to be handled well by the default
`fill-paragraph', though.

-- Simen

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

bug#20256: 25.0.50; css-mode: filling multi-line comments

[Stefan Monnier]

> No, I don't think so. When the comment-end marker is \n,
> `forward-comment' will leave point at the beginning of the next line,
> causing the whole filling code to be skipped. Even when tweaking the
> predicate so that the code is run anyway, it doesn't fill the comment
> properly. Was it ever intended to do so? I assumed from the comment
> that you quoted that it wasn't.

> Those kinds of comments seem to be handled well by the default
> `fill-paragraph', though.

Oh, indeed, fill-paragraph-handle-comment should take care of those.


        Stefan