adaptive-fill-mode and auto-fill-mode

adaptive-fill-mode and auto-fill-mode

[martin rudalics]

To reproduce with Emacs -Q insert the following text ...

(auto-fill-mode 1)
(setq fill-column 60)

;; x
;; x
(defun foo (from to)
  (yes-or-no-p (format "..... should we foo from %s to %s?" from to
		       ;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx yy
;;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx zz
		       )))

... in *scratch* and evaluate *scratch*.


(1) Typing SPC at the end of the `yes-or-no-p' line gets me

(defun foo (from to)
  (yes-or-no-p (format "..... should we foo from %s to %s?"
;; from to 
		       ;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx yy
;;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx zz
		       )))


(2) Typing SPC after yy breaks the comment as

(defun foo (from to)
  (yes-or-no-p (format "..... should we foo from %s to %s?" from to
		       ;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
;; yy 
;;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx zz
		       )))


(3) Typing SPC after zz breaks the comment as

(defun foo (from to)
  (yes-or-no-p (format "..... should we foo from %s to %s?" from to
		       ;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx yy
;;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
;; zz 
		       )))


(4) Changing the definition of foo to

(defun foo (from to)
  (yes-or-no-p (format "....... should we foo from %s to %s?" from to
		       ;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx yy
;;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx zz
		       )))

and typing SPC at the end of the `yes-or-no-p' line gets me

(defun foo (from to)
  (yes-or-no-p (format "....... should we foo from %s to
;; %s?" from to 
		       ;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx yy
;;; xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx zz
		       )))


The patch below is an attempt to resolve these problems with minimal
invasiveness (but for the removal of the `fill-nobreak-predicate' let
binding in `fill-move-to-break-point' which, IMHO, breaks the purpose of
`fill-nobreak-predicate' anyway).


*** textmodes/fill.el.~1.191.~	Fri Sep  8 18:07:28 2006
--- textmodes/fill.el	Fri Oct  6 19:42:22 2006
***************
*** 520,526 ****
        ;; Ok, skip at least one word or one \c| character.
        ;; Meanwhile, don't stop at a period followed by one space.
        (let ((to (line-end-position))
- 	    (fill-nobreak-predicate nil) ;to break sooner.
  	    (first t))
  	(goto-char linebeg)
  	(while (and (< (point) to) (or first (fill-nobreak-p)))
--- 520,525 ----


*** newcomment.el.~1.96.~	Mon Aug 21 14:35:24 2006
--- newcomment.el	Fri Oct  6 19:49:14 2006
***************
*** 1182,1187 ****
--- 1182,1202 ----
  	 ;; If there's an adaptive prefix, use it unless we're inside
  	 ;; a comment and the prefix is not a comment starter.
  	 ((and fill-prefix
+ 	       ;; Don't use the adaptive prefix if we are not in a comment and
+ 	       ;; the adaptive prefix matches `comment-start'.
+ 	       (or compos (null comment-start)
+ 		   (not (string-match
+ 			 (regexp-quote comment-start) fill-prefix)))
+ 	       ;; Don't use the adaptive prefix if we are in an end-of-line
+ 	       ;; comment that doesn't start at the left margin or whose start
+ 	       ;; sequence doesn't match the prefix.
+ 	       (or (not compos) (not (string-equal comment-end ""))
+ 		   (and (<= compos
+ 			    (+ (line-beginning-position 0)
+ 			       (if (numberp left-margin) left-margin 0)))
+ 			(save-excursion
+ 			  (goto-char compos)
+ 			  (looking-at (regexp-quote fill-prefix)))))
  	       (or (not compos)
  		   (comment-valid-prefix-p fill-prefix compos)))
  	  (indent-to-left-margin)


*** emacs-lisp/lisp-mode.el.~1.194.~	Tue Aug 15 10:00:52 2006
--- emacs-lisp/lisp-mode.el	Fri Oct  6 12:21:32 2006
***************
*** 210,215 ****
--- 210,222 ----
    ;; because lisp-fill-paragraph should do the job.
    ;;  I believe that newcomment's auto-fill code properly deals with it  -stef
    ;;(set (make-local-variable 'adaptive-fill-mode) nil)
+   (set (make-local-variable 'fill-nobreak-predicate)
+        (lambda ()
+ 	 (and (eq (get-text-property (point) 'face)
+ 		  'font-lock-string-face)
+ 	      (or (= (point) (point-min))
+ 		  (eq (get-text-property (1- (point)) 'face)
+ 		      'font-lock-string-face)))))
    (make-local-variable 'indent-line-function)
    (setq indent-line-function 'lisp-indent-line)
    (make-local-variable 'indent-region-function)

Re: adaptive-fill-mode and auto-fill-mode

[Stefan Monnier]

> *** newcomment.el.~1.96.~	Mon Aug 21 14:35:24 2006
> --- newcomment.el	Fri Oct  6 19:49:14 2006
> ***************
> *** 1182,1187 ****
> --- 1182,1202 ----
>   	 ;; If there's an adaptive prefix, use it unless we're inside
>   	 ;; a comment and the prefix is not a comment starter.
>   	 ((and fill-prefix
> + 	       ;; Don't use the adaptive prefix if we are not in a comment and
> + 	       ;; the adaptive prefix matches `comment-start'.
> + 	       (or compos (null comment-start)
> + 		   (not (string-match
> + 			 (regexp-quote comment-start) fill-prefix)))

Why not use comment-start-skip instead?

> + 	       ;; Don't use the adaptive prefix if we are in an end-of-line
> + 	       ;; comment that doesn't start at the left margin or whose start
> + 	       ;; sequence doesn't match the prefix.
> + 	       (or (not compos) (not (string-equal comment-end ""))
> + 		   (and (<= compos
> + 			    (+ (line-beginning-position 0)
> + 			       (if (numberp left-margin) left-margin 0)))
> + 			(save-excursion
> + 			  (goto-char compos)
> + 			  (looking-at (regexp-quote fill-prefix)))))

I disagree with the "at left margin" thingy.  I'm not 100% sure what it's
trying to fix, tho, so please explain which scenario it fixes.

As for the rest of the code above, please merge it with the two subsequent
lines which already do something like that.  I.e. move the code to
comment-valid-prefix-p.

>   	       (or (not compos)
>   		   (comment-valid-prefix-p fill-prefix compos)))

And finally, rather than having

   (and foo
        (or bar ...)
        (or (null bar) ...))

better use

   (and foo
        (if bar
            ...
          ...))

Also, please add a note that this is mostly trying to work around a bug in
auto-fill-mode where (for|back)ward-paragraph (used to find the relevant
boundaries of the paragraph, then used to find the adaptive prefix) doesn't
pay attention to comments.


        Stefan

Re: adaptive-fill-mode and auto-fill-mode

[martin rudalics]

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

> Why not use comment-start-skip instead?

OK

> I disagree with the "at left margin" thingy.  I'm not 100% sure what it's
> trying to fix, tho, so please explain which scenario it fixes.

I agree with your disagreement.

> As for the rest of the code above, please merge it with the two subsequent
> lines which already do something like that.  I.e. move the code to
> comment-valid-prefix-p.

Please look at the attached patch.

BTW is the fill.el change harmless?  In my opinion the `fill-nobreak-predicate'
stuff was broken.  Did anyone ever use that?

martin

[-- Attachment #2: auto-fill.patch --]
[-- Type: text/plain, Size: 4719 bytes --]

*** textmodes/fill.el.~1.191.~	Fri Sep  8 18:07:28 2006
--- textmodes/fill.el	Fri Oct  6 19:42:22 2006
***************
*** 520,526 ****
        ;; Ok, skip at least one word or one \c| character.
        ;; Meanwhile, don't stop at a period followed by one space.
        (let ((to (line-end-position))
- 	    (fill-nobreak-predicate nil) ;to break sooner.
  	    (first t))
  	(goto-char linebeg)
  	(while (and (< (point) to) (or first (fill-nobreak-p)))
--- 520,525 ----


*** newcomment.el.~1.96.~	Mon Aug 21 14:35:24 2006
--- newcomment.el	Sat Oct  7 20:19:18 2006
***************
*** 238,244 ****
  (defcustom comment-empty-lines nil
    "If nil, `comment-region' does not comment out empty lines.
  If t, it always comments out empty lines.
! if `eol' it only comments out empty lines if comments are
  terminated by the end of line (i.e. `comment-end' is empty)."
    :type '(choice (const :tag "Never" nil)
  	  (const :tag "Always" t)
--- 238,244 ----
  (defcustom comment-empty-lines nil
    "If nil, `comment-region' does not comment out empty lines.
  If t, it always comments out empty lines.
! If `eol' it only comments out empty lines if comments are
  terminated by the end of line (i.e. `comment-end' is empty)."
    :type '(choice (const :tag "Never" nil)
  	  (const :tag "Always" t)
***************
*** 1124,1135 ****
    :group 'comment)

  (defun comment-valid-prefix-p (prefix compos)
!   (or
!    ;; Accept any prefix if the current comment is not EOL-terminated.
!    (save-excursion (goto-char compos) (comment-forward) (not (bolp)))
!    ;; Accept any prefix that starts with a comment-start marker.
!    (string-match (concat "\\`[ \t]*\\(?:" comment-start-skip "\\)")
! 		 prefix)))

  ;;;###autoload
  (defun comment-indent-new-line (&optional soft)
--- 1124,1152 ----
    :group 'comment)

  (defun comment-valid-prefix-p (prefix compos)
!   (if compos
!       (and
!        (or
! 	;; Accept any prefix if the current comment is not EOL-terminated.
! 	(save-excursion (goto-char compos) (comment-forward) (not (bolp)))
! 	;; Accept any prefix that starts with a comment-start marker.
! 	(string-match (concat "\\`[ \t]*\\(?:" comment-start-skip "\\)")
! 		      prefix))
!        ;; Don't accept a prefix in an end-of-line comment that doesn't start at
!        ;; line beginning or whose start sequence doesn't match the prefix.
!        ;; This should work around a bug where `do-auto-fill' determines the
!        ;; prefix from the beginning of the paragraph but doesn't pay attention
!        ;; to comments.
!        (or (not (string-equal comment-end ""))
! 	   (and (<= compos (line-beginning-position 0))
! 		(save-excursion
! 		  (goto-char compos)
! 		  (looking-at (regexp-quote prefix))))))
!     ;; Don't accept a prefix if we are not in a comment and the adaptive prefix
!     ;; matches `comment-start-skip'.  This should work around the `do-auto-fill'
!     ;; bug cited above which may cause code put inadvertently inside a comment.
!     (or (null comment-start)
! 	(not (string-match comment-start-skip prefix)))))

  ;;;###autoload
  (defun comment-indent-new-line (&optional soft)
***************
*** 1179,1189 ****
  	    (setq comin (point))))

  	(cond
! 	 ;; If there's an adaptive prefix, use it unless we're inside
! 	 ;; a comment and the prefix is not a comment starter.
! 	 ((and fill-prefix
! 	       (or (not compos)
! 		   (comment-valid-prefix-p fill-prefix compos)))
  	  (indent-to-left-margin)
  	  (insert-and-inherit fill-prefix))
  	 ;; If we're not inside a comment, just try to indent.
--- 1196,1203 ----
  	    (setq comin (point))))

  	(cond
! 	 ;; If there's an adaptive prefix, use it provided it's valid.
! 	 ((and fill-prefix (comment-valid-prefix-p fill-prefix compos))
  	  (indent-to-left-margin)
  	  (insert-and-inherit fill-prefix))
  	 ;; If we're not inside a comment, just try to indent.


*** emacs-lisp/lisp-mode.el.~1.194.~	Tue Aug 15 10:00:52 2006
--- emacs-lisp/lisp-mode.el	Sat Oct  7 20:30:42 2006
***************
*** 210,215 ****
--- 210,223 ----
    ;; because lisp-fill-paragraph should do the job.
    ;;  I believe that newcomment's auto-fill code properly deals with it  -stef
    ;;(set (make-local-variable 'adaptive-fill-mode) nil)
+   (set (make-local-variable 'fill-nobreak-predicate)
+        ;; Try to avoid that auto-fill breaks strings.
+        (lambda ()
+ 	 (and (eq (get-text-property (point) 'face)
+ 		  'font-lock-string-face)
+ 	      (or (= (point) (point-min))
+ 		  (eq (get-text-property (1- (point)) 'face)
+ 		      'font-lock-string-face)))))
    (make-local-variable 'indent-line-function)
    (setq indent-line-function 'lisp-indent-line)
    (make-local-variable 'indent-region-function)

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

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: adaptive-fill-mode and auto-fill-mode

[Stefan Monnier]

>> I disagree with the "at left margin" thingy.  I'm not 100% sure what it's
>> trying to fix, tho, so please explain which scenario it fixes.
> I agree with your disagreement.

I'm not sure what you mean: isn't the test still present in your
latest patch?

> BTW is the fill.el change harmless?  In my opinion the
> `fill-nobreak-predicate' stuff was broken.  Did anyone ever use that?

I think it doesn't matter either way.

> !   (if compos
> !       (and
> !        (or
> ! 	;; Accept any prefix if the current comment is not EOL-terminated.
> ! 	(save-excursion (goto-char compos) (comment-forward) (not (bolp)))
> ! 	;; Accept any prefix that starts with a comment-start marker.
> ! 	(string-match (concat "\\`[ \t]*\\(?:" comment-start-skip "\\)")
> ! 		      prefix))

OK.

> !        ;; Don't accept a prefix in an end-of-line comment that doesn't start at
> !        ;; line beginning or whose start sequence doesn't match the prefix.
> !        ;; This should work around a bug where `do-auto-fill' determines the
> !        ;; prefix from the beginning of the paragraph but doesn't pay attention
> !        ;; to comments.
> !        (or (not (string-equal comment-end ""))
> ! 	   (and (<= compos (line-beginning-position 0))
> ! 		(save-excursion
> ! 		  (goto-char compos)
> ! 		  (looking-at (regexp-quote prefix))))))

I don't quite understand this.  I feel like this block repeats the previous
one.  I.e. the first part (comment-end check) seems like an inferior
alternative to the "comment-forward + not bolp" check above, and the second
part seems to be a stricter form of the second part above (it checks not
just that prefix matches comment-start-skip but that it specifically
matches the prefix used in the current comment).

Note that the stricter match is sometimes too strict.  I often have comments
where the adaptive-prefix correctly includes extra chars compared to the
initial comment start.  E.g.

        ;; - here is the beginning of the paragraph
        ;;   where the fill-prefix will include 3 spaces after ";;"
        ;;   even though the first line doesn't match it.

It could even be things like

        ;; foo wrote:
        ;; > where the fill-prefix will include 3 spaces after ";;"
        ;; > even though the first line doesn't match it.

I.e. we should probably use `comment-start-skip' on `prefix' to extract the
comment-marker part of the prefix, then strip whitespace, then check that
the same match&strip at compos returns the same thing.

> +   (set (make-local-variable 'fill-nobreak-predicate)
> +        ;; Try to avoid that auto-fill breaks strings.
> +        (lambda ()
> + 	 (and (eq (get-text-property (point) 'face)
> + 		  'font-lock-string-face)
> + 	      (or (= (point) (point-min))
> + 		  (eq (get-text-property (1- (point)) 'face)
> + 		      'font-lock-string-face)))))

Do we really want that?  It seems at best to be a personal preference.


        Stefan

Re: adaptive-fill-mode and auto-fill-mode

[Stefan Monnier]

> + 	      (or (= (point) (point-min))

Aka (bobp)


        Stefan

Re: adaptive-fill-mode and auto-fill-mode

[martin rudalics]

 >>>I disagree with the "at left margin" thingy.  I'm not 100% sure what it's
 >>>trying to fix, tho, so please explain which scenario it fixes.
 >>
 >>I agree with your disagreement.
 >
 >
 > I'm not sure what you mean: isn't the test still present in your
 > latest patch?

I removed the "at left margin" thingy.  If you refer to the

(<= compos (line-beginning-position 0)

thingy then I need that to avoid the `indent-to-left-margin' in
`comment-indent-new-line' as in bug(2) of my first mail.  If we agree to
use `indent-according-to-mode' here, that check is not needed.  Note
that bug(2) is quite nasty since with longer defuns you usually won't
guess why it happened in the first place.  And defuns preceded by two
";; " comment lines are quite frequent in the Elisp source.

 >>BTW is the fill.el change harmless?  In my opinion the
 >>`fill-nobreak-predicate' stuff was broken.  Did anyone ever use that?
 >
 >
 > I think it doesn't matter either way.

Suppose you write a long Elisp regexp that you don't want to break
before you finished writing.  Auto fill will break it without mercy.

 >>!        ;; Don't accept a prefix in an end-of-line comment that doesn't start at
 >>!        ;; line beginning or whose start sequence doesn't match the prefix.
 >>!        ;; This should work around a bug where `do-auto-fill' determines the
 >>!        ;; prefix from the beginning of the paragraph but doesn't pay attention
 >>!        ;; to comments.
 >>!        (or (not (string-equal comment-end ""))
 >>! 	   (and (<= compos (line-beginning-position 0))
 >>! 		(save-excursion
 >>! 		  (goto-char compos)
 >>! 		  (looking-at (regexp-quote prefix))))))
 >
 >
 > I don't quite understand this.  I feel like this block repeats the previous
 > one.  I.e. the first part (comment-end check) seems like an inferior
 > alternative to the "comment-forward + not bolp" check above,

Indeed.  The reason is that I don't want to affect multiline comment
environments when the comment starting at compos is a single line
comment.  Hence I check whether this is a "single line environment" -
exemplified by `comment-end' equalling the empty string - and proceed
only in that case.  If you think that multiline comment modes may be
affected as well ...

 > and the second
 > part seems to be a stricter form of the second part above (it checks not
 > just that prefix matches comment-start-skip but that it specifically
 > matches the prefix used in the current comment).
 >
 > Note that the stricter match is sometimes too strict.  I often have comments
 > where the adaptive-prefix correctly includes extra chars compared to the
 > initial comment start.  E.g.
 >
 >         ;; - here is the beginning of the paragraph
 >         ;;   where the fill-prefix will include 3 spaces after ";;"
 >         ;;   even though the first line doesn't match it.
 >
 > It could even be things like
 >
 >         ;; foo wrote:
 >         ;; > where the fill-prefix will include 3 spaces after ";;"
 >         ;; > even though the first line doesn't match it.

Do you think my patch would be responsible for not handling these (at
line-beginning)?  If the prefix passed to `comment-valid-prefix-p'
matches the string at compos there shouldn't be any problems.  I think
the second example doesn't work because `fill-common-string-prefix'
determines the common prefix of ";; " and ";; > " as ";; " long before
I lay my hands at this.

 > I.e. we should probably use `comment-start-skip' on `prefix' to extract the
 > comment-marker part of the prefix, then strip whitespace, then check that
 > the same match&strip at compos returns the same thing.

I don't quite follow you here.  Where do you want to strip whitespace,
before the comment marker, between comment marker and body?  Would it be
reliable to do that?

 >>+   (set (make-local-variable 'fill-nobreak-predicate)
 >>+        ;; Try to avoid that auto-fill breaks strings.
 >>+        (lambda ()
 >>+ 	 (and (eq (get-text-property (point) 'face)
 >>+ 		  'font-lock-string-face)
 >>+ 	      (or (= (point) (point-min))
 >>+ 		  (eq (get-text-property (1- (point)) 'face)
 >>+ 		      'font-lock-string-face)))))
 >
 >
 > Do we really want that?  It seems at best to be a personal preference.

Personally, I don't care at all.  My auto-fill-function uses syntax-ppss
and doesn't fill normal strings.  But, as stated above, I don't think
that auto-fill should be allowed to break non-doc-strings in Elisp.

Re: adaptive-fill-mode and auto-fill-mode

[Stefan Monnier]

>>>> I disagree with the "at left margin" thingy.  I'm not 100% sure what it's
>>>> trying to fix, tho, so please explain which scenario it fixes.
>>> I agree with your disagreement.
>> I'm not sure what you mean: isn't the test still present in your
>> latest patch?

> I removed the "at left margin" thingy.  If you refer to the

> (<= compos (line-beginning-position 0)

> thingy then I need that to avoid the `indent-to-left-margin' in
> `comment-indent-new-line' as in bug(2) of my first mail.  If we agree to
> use `indent-according-to-mode' here, that check is not needed.  Note
> that bug(2) is quite nasty since with longer defuns you usually won't
> guess why it happened in the first place.  And defuns preceded by two
> ";; " comment lines are quite frequent in the Elisp source.

I guess what I don't understand is in what way this is a fix for
the problem.  Yes, it may work for your specific case, but what about in
cases such as:

  (defun foo ()
    "docstring"

    ;; The empty line above is important.
    ;; This second line as well.
    (let ((toto titi))
      ;; some comment klsdhfkshgkslfkhgsjkhgskhgjfdhgd yy

the same problem will happen, although not at BOL.  Maybe a better fix would
be (when compos is set and `prefix' matches
(concat "\\`[ \t]*" comment-start-skip)) to go to compos, and call
comment-forward until we reach point.  If we bump into non-comment before
reaching point, then the prefix was set based on some other unrelated
comment and should be ignored.

>>> BTW is the fill.el change harmless?  In my opinion the
>>> `fill-nobreak-predicate' stuff was broken.  Did anyone ever use that?
>> I think it doesn't matter either way.
> Suppose you write a long Elisp regexp that you don't want to break
> before you finished writing.  Auto fill will break it without mercy.

It's not that it doesn't make a difference, but that it only comes into play
in fairly unusual circumstances and that which behavior is right and which
is wrong depends on the specifics, so you can find supporting evidence
either way.  In this context I won't oppose your patch, especially since it
simplifies the code.

>>> !        ;; Don't accept a prefix in an end-of-line comment that doesn't start at
>>> !        ;; line beginning or whose start sequence doesn't match the prefix.
>>> !        ;; This should work around a bug where `do-auto-fill' determines the
>>> !        ;; prefix from the beginning of the paragraph but doesn't pay attention
>>> !        ;; to comments.
>>> !        (or (not (string-equal comment-end ""))
>>> ! 	   (and (<= compos (line-beginning-position 0))
>>> ! 		(save-excursion
>>> ! 		  (goto-char compos)
>>> ! 		  (looking-at (regexp-quote prefix))))))
>> 
>> 
>> I don't quite understand this.  I feel like this block repeats the previous
>> one.  I.e. the first part (comment-end check) seems like an inferior
>> alternative to the "comment-forward + not bolp" check above,

> Indeed.  The reason is that I don't want to affect multiline comment
> environments when the comment starting at compos is a single line
> comment.

But in what is checking comment-end better than using "comment-forward + not
bolp"?

> Hence I check whether this is a "single line environment" -
> exemplified by `comment-end' equalling the empty string - and proceed
> only in that case.  If you think that multiline comment modes may be
> affected as well ...

*Many* modes allow both single-line and multi-line comments and
comment-start/comment-end doesn't restrict the comment style that we may
encounter in the file.  I.e. in C++ mode comment-end may be "" but /*..*/
comments are pretty common.

>> ;; foo wrote:
>> ;; > where the fill-prefix will include 3 spaces after ";;"
>> ;; > even though the first line doesn't match it.

> Do you think my patch would be responsible for not handling these

The prefix would be ";; >" which doesn't match the string at compos, so your
code would reject it.

> (at line-beginning)?

These can happen at any indentation.

> If the prefix passed to `comment-valid-prefix-p'
> matches the string at compos there shouldn't be any problems.

Of course, but what I'm saying is that it may not match, even in cases where
it's perfectly correct.

> I think the second example doesn't work because
> `fill-common-string-prefix' determines the common prefix of ";; " and
> ";; > " as ";; " long before I lay my hands at this.

That depends on adaptive-fill-function.

>> I.e. we should probably use `comment-start-skip' on `prefix' to extract the
>> comment-marker part of the prefix, then strip whitespace, then check that
>> the same match&strip at compos returns the same thing.

> I don't quite follow you here.  Where do you want to strip whitespace,
> before the comment marker, between comment marker and body?  Would it be
> reliable to do that?

Why not?

>>> +   (set (make-local-variable 'fill-nobreak-predicate)
>>> +        ;; Try to avoid that auto-fill breaks strings.
>>> +        (lambda ()
>>> + 	 (and (eq (get-text-property (point) 'face)
>>> + 		  'font-lock-string-face)
>>> + 	      (or (= (point) (point-min))
>>> + 		  (eq (get-text-property (1- (point)) 'face)
>>> + 		      'font-lock-string-face)))))
>> 
>> 
>> Do we really want that?  It seems at best to be a personal preference.

> Personally, I don't care at all.  My auto-fill-function uses syntax-ppss
> and doesn't fill normal strings.  But, as stated above, I don't think
> that auto-fill should be allowed to break non-doc-strings in Elisp.

I completely understand your point of view, but it's still far from
a bug-fix and it's not the only way to go about it.  Have you tried
comment-auto-fill-only-comments?


        Stefan

Re: adaptive-fill-mode and auto-fill-mode

[martin rudalics]

 > I guess what I don't understand is in what way this is a fix for
 > the problem.  Yes, it may work for your specific case, but what about in
 > cases such as:
 >
 >   (defun foo ()
 >     "docstring"
 >
 >     ;; The empty line above is important.
 >     ;; This second line as well.
 >     (let ((toto titi))
 >       ;; some comment klsdhfkshgkslfkhgsjkhgskhgjfdhgd yy
 >
 > the same problem will happen, although not at BOL.

I don't accept the prefix when _compos_ is not at BOL (in your example
compos is before ";; some comment ...").  `comment-indent-new-line' has
no idea where the fill-prefix comes from (otherwise solving all this
would be trivial).  I'm afraid my code is unreadable.

 >  Maybe a better fix would
 > be (when compos is set and `prefix' matches
 > (concat "\\`[ \t]*" comment-start-skip)) to go to compos, and call
 > comment-forward until we reach point.  If we bump into non-comment before
 > reaching point, then the prefix was set based on some other unrelated
 > comment and should be ignored.

Why?  compos is at the beginning of the current comment provided `point'
is within a comment.  The problem that `comment-indent-new-line' used
the fill-prefix in the "not compos" case is trivially handled in the
"else" part of `comment-valid-prefix-p'.  The problem you refer to is
with the "then" part.

 >>Indeed.  The reason is that I don't want to affect multiline comment
 >>environments when the comment starting at compos is a single line
 >>comment.
 >
 >
 > But in what is checking comment-end better than using "comment-forward + not
 > bolp"?

Well, checking comment-end style is cheaper than doing comment-forward.
Combine these before doing the string-match?

 >>>;; foo wrote:
 >>>;; > where the fill-prefix will include 3 spaces after ";;"
 >>>;; > even though the first line doesn't match it.
 >
 >
 >>Do you think my patch would be responsible for not handling these
 >
 >
 > The prefix would be ";; >" which doesn't match the string at compos, so your
 > code would reject it.

... but the string at compos is ";; >" ...

 >>(at line-beginning)?
 >
 >
 > These can happen at any indentation.

... if compos is not at line beginning the prefix is rejected.

 >>If the prefix passed to `comment-valid-prefix-p'
 >>matches the string at compos there shouldn't be any problems.
 >
 >
 > Of course, but what I'm saying is that it may not match, even in cases where
 > it's perfectly correct.

The only cases rejected are when the string is not at line beginning or
does not match the prefix.  In your second example the prefix is ";; ",
at least with emacs -Q, because that's the common prefix of ";; " and
";; >".

 >>I think the second example doesn't work because
 >>`fill-common-string-prefix' determines the common prefix of ";; " and
 >>";; > " as ";; " long before I lay my hands at this.
 >
 >
 > That depends on adaptive-fill-function.

I fail to understand you here.  If the comment at compos doesn't match
the prefix why should I want to insert the prefix on the next line?

 >>Where do you want to strip whitespace,
 >>before the comment marker, between comment marker and body?  Would it be
 >>reliable to do that?
 >
 >
 > Why not?

Because, as I suppose, the number of spaces is significant for the
fill-prefix.

 >>Personally, I don't care at all.  My auto-fill-function uses syntax-ppss
 >>and doesn't fill normal strings.  But, as stated above, I don't think
 >>that auto-fill should be allowed to break non-doc-strings in Elisp.
 >
 >
 > I completely understand your point of view, but it's still far from
 > a bug-fix and it's not the only way to go about it.

It fixes that better than I expected.  Note also that you never had a
chance to handle this _before_ the introduction of the doc-string face.
But I admit that emacs was always breaking lisp strings this way, hence
let's drop it (which means I don't need to change fill.el either).

 > Have you tried
 > comment-auto-fill-only-comments?

It doesn't fill doc-strings.

Re: adaptive-fill-mode and auto-fill-mode

[Stefan Monnier]

> Why?  compos is at the beginning of the current comment provided `point'
> is within a comment.

Oh right, I'm not making sense.  Then how 'bout the 100% guaranteed untested
patch below.

> The problem that `comment-indent-new-line' used
> the fill-prefix in the "not compos" case is trivially handled in the
> "else" part of `comment-valid-prefix-p'.  The problem you refer to is
> with the "then" part.

>>> Indeed.  The reason is that I don't want to affect multiline comment
>>> environments when the comment starting at compos is a single line
>>> comment.
>> 
>> 
>> But in what is checking comment-end better than using "comment-forward + not
>> bolp"?

> Well, checking comment-end style is cheaper than doing comment-forward.
> Combine these before doing the string-match?

But the code *already* does the comment-forward check.  As I pointed out
your code does the same thing as mine, just slightly differently.  This part
of the different seems to be just "worse".  The other part OTOH seems to be
"partly better".
Please merge them.

>>>> ;; foo wrote:
>>>> ;; > where the fill-prefix will include 3 spaces after ";;"
>>>> ;; > even though the first line doesn't match it.
>> 
>> 
>>> Do you think my patch would be responsible for not handling these
>> 
>> 
>> The prefix would be ";; >" which doesn't match the string at compos, so your
>> code would reject it.

> ... but the string at compos is ";; >" ...

Not necessarily.  I may compute this in adaptive-fill-function straight from
the first line, (when using auto-fill on the first line).

>>> (at line-beginning)?
>> These can happen at any indentation.
> ... if compos is not at line beginning the prefix is rejected.

Where?  Why?

> The only cases rejected are when the string is not at line beginning or
> does not match the prefix.  In your second example the prefix is ";; ",
> at least with emacs -Q, because that's the common prefix of ";; " and
> ";; >".

But your code will be used even if "-Q" is not passed.

>>> I think the second example doesn't work because
>>> `fill-common-string-prefix' determines the common prefix of ";; " and
>>> ";; > " as ";; " long before I lay my hands at this.
>> 
>> That depends on adaptive-fill-function.

> I fail to understand you here.  If the comment at compos doesn't match
> the prefix why should I want to insert the prefix on the next line?

Why not?

>>> Personally, I don't care at all.  My auto-fill-function uses syntax-ppss
>>> and doesn't fill normal strings.  But, as stated above, I don't think
>>> that auto-fill should be allowed to break non-doc-strings in Elisp.
>> I completely understand your point of view, but it's still far from
>> a bug-fix and it's not the only way to go about it.

> It fixes that better than I expected.  Note also that you never had a
> chance to handle this _before_ the introduction of the doc-string face.
> But I admit that emacs was always breaking lisp strings this way, hence
> let's drop it (which means I don't need to change fill.el either).

>> Have you tried
>> comment-auto-fill-only-comments?

> It doesn't fill doc-strings.

But maybe it's the right place to introduce such a feature.
Basically extend this var so you can say "fill in FOO" where FOO is the list
of possible contexts, such as `comment', `string', `doc', `code'?


        Stefan


--- newcomment.el	24 aoû 2006 14:41:55 -0400	1.96
+++ newcomment.el	08 oct 2006 21:33:15 -0400	
@@ -1124,12 +1124,44 @@
   :group 'comment)
 
 (defun comment-valid-prefix-p (prefix compos)
+  "Check that the adaptive-fill-prefix is consistent with the context.
+PREFIX is the prefix (presumably guessed by `adaptive-fill-mode').
+COMPOS is the position of the beginning of the comment we're in,
+or nil if we're not inside a comment."
+  ;; This consistency checking is mostly needed to workaround the limitation
+  ;; of auto-fill-mode whose paragraph-determination doesn't pay attention
+  ;; to comment boundaries.
+  (if (null compos)
+      ;; We're not inside a comment: the prefix shouldn't match
+      ;; a comment-starter.
+      (not (and comment-start comment-start-skip
+                (string-match comment-start-skip prefix)))
   (or
    ;; Accept any prefix if the current comment is not EOL-terminated.
    (save-excursion (goto-char compos) (comment-forward) (not (bolp)))
-   ;; Accept any prefix that starts with a comment-start marker.
-   (string-match (concat "\\`[ \t]*\\(?:" comment-start-skip "\\)")
-		 prefix)))
+     ;; Accept any prefix that starts with the same comment-start marker
+     ;; as the current one.
+     (when (string-match (concat "\\`[ \t]*\\(?:" comment-start-skip "\\)")
+                         prefix)
+       (let ((prefix-com (comment-string-strip (match-string 0) nil t)))
+         (string-match "\\`[ \t]*" prefix-com)
+         (let* ((prefix-space (match-string 0))
+                (prefix-indent (string-width prefix-space))
+                (prefix-comstart (substring prefix-com (match-end 0))))
+           (save-excursion
+             (goto-char compos)
+             ;; The comstart marker is the same.
+             (and (looking-at (regexp-quote prefix-comstart))
+                  ;; The indentation as well.
+                  (or (= prefix-indent
+                         (- (current-column) (current-left-margin)))
+                      ;; Check the indentation in two different ways, just
+                      ;; to try and avoid most of the potential funny cases.
+                      (equal prefix-space
+                             (buffer-substring (point)
+                                               (progn (move-to-left-margin)
+                                                      (point)))))))))))))
+                    
 
 ;;;###autoload
 (defun comment-indent-new-line (&optional soft)
@@ -1182,8 +1214,7 @@
 	 ;; If there's an adaptive prefix, use it unless we're inside
 	 ;; a comment and the prefix is not a comment starter.
 	 ((and fill-prefix
-	       (or (not compos)
-		   (comment-valid-prefix-p fill-prefix compos)))
+               (comment-valid-prefix-p fill-prefix compos))
 	  (indent-to-left-margin)
 	  (insert-and-inherit fill-prefix))
 	 ;; If we're not inside a comment, just try to indent.

Re: adaptive-fill-mode and auto-fill-mode

[martin rudalics]

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

 > Then how 'bout the 100% guaranteed untested
 > patch below.

Neat.  I attached a 50% unguaranteed tested revision.

 >>... but the string at compos is ";; >" ...
 >
 >
 > Not necessarily.  I may compute this in adaptive-fill-function straight from
 > the first line, (when using auto-fill on the first line).

... no matter what you compute in `adaptive-fill-function' the string at
compos is the string at compos is the ...

 >>... if compos is not at line beginning the prefix is rejected.
 >
 >
 > Where?  Why?

In my first patch.  The current one doesn't reject it any more.

 >>I fail to understand you here.  If the comment at compos doesn't match
 >>the prefix why should I want to insert the prefix on the next line?
 >
 >
 > Why not?

Your own patch might impede you.  If you manually change the
comment-start sequence at compos or the whitespace preceding it, the
prefix won't match the comment at compos any more and hence won't be
used.

 >>>Have you tried
 >>>comment-auto-fill-only-comments?
 >
 >
 >>It doesn't fill doc-strings.
 >
 >
 > But maybe it's the right place to introduce such a feature.
 > Basically extend this var so you can say "fill in FOO" where FOO is the list
 > of possible contexts, such as `comment', `string', `doc', `code'?

If someone is interested I'll write it.

[-- Attachment #2: auto-fill.patch --]
[-- Type: text/plain, Size: 4412 bytes --]

*** newcomment.el.~1.96.~	Mon Aug 21 14:35:24 2006
--- newcomment.el	Mon Oct  9 11:23:40 2006
***************
*** 238,244 ****
  (defcustom comment-empty-lines nil
    "If nil, `comment-region' does not comment out empty lines.
  If t, it always comments out empty lines.
! if `eol' it only comments out empty lines if comments are
  terminated by the end of line (i.e. `comment-end' is empty)."
    :type '(choice (const :tag "Never" nil)
  	  (const :tag "Always" t)
--- 238,244 ----
  (defcustom comment-empty-lines nil
    "If nil, `comment-region' does not comment out empty lines.
  If t, it always comments out empty lines.
! If `eol' it only comments out empty lines if comments are
  terminated by the end of line (i.e. `comment-end' is empty)."
    :type '(choice (const :tag "Never" nil)
  	  (const :tag "Always" t)
***************
*** 1124,1135 ****
    :group 'comment)

  (defun comment-valid-prefix-p (prefix compos)
!   (or
!    ;; Accept any prefix if the current comment is not EOL-terminated.
!    (save-excursion (goto-char compos) (comment-forward) (not (bolp)))
!    ;; Accept any prefix that starts with a comment-start marker.
!    (string-match (concat "\\`[ \t]*\\(?:" comment-start-skip "\\)")
! 		 prefix)))

  ;;;###autoload
  (defun comment-indent-new-line (&optional soft)
--- 1124,1169 ----
    :group 'comment)

  (defun comment-valid-prefix-p (prefix compos)
!     "Check that the adaptive-fill-prefix is consistent with the context.
! PREFIX is the prefix (presumably guessed by `adaptive-fill-mode').
! COMPOS is the position of the beginning of the comment we're in, or nil
! if we're not inside a comment."
!   ;; This consistency checking is mostly needed to workaround the limitation
!   ;; of auto-fill-mode whose paragraph-determination doesn't pay attention
!   ;; to comment boundaries.
!   (if (null compos)
!       ;; We're not inside a comment: the prefix shouldn't match
!       ;; a comment-starter.
!       (not (and comment-start comment-start-skip
!                 (string-match comment-start-skip prefix)))
!     (or
!      ;; Accept any prefix if the current comment is not EOL-terminated.
!      (save-excursion (goto-char compos) (comment-forward) (not (bolp)))
!      ;; Accept any prefix that starts with the same comment-start marker
!      ;; as the current one.
!      (when (string-match (concat "\\`[ \t]*\\(?:" comment-start-skip "\\)")
!                          prefix)
!        (let ((prefix-com (comment-string-strip (match-string 0 prefix) nil t)))
!          (string-match "\\`[ \t]*" prefix-com)
!          (let* ((prefix-space (match-string 0 prefix-com))
!                 (prefix-indent (string-width prefix-space))
!                 (prefix-comstart (substring prefix-com (match-end 0))))
!            (save-excursion
!              (goto-char compos)
!              ;; The comstart marker is the same.
!              (and (looking-at comment-start-skip)
! 		  (string-equal
! 		   prefix-comstart
! 		   (comment-string-strip (match-string 0) nil t))
!                   ;; The indentation as well.
!                   (or (= prefix-indent
!                          (- (current-column) (current-left-margin)))
!                       ;; Check the indentation in two different ways, just
!                       ;; to try and avoid most of the potential funny cases.
!                       (equal prefix-space
!                              (buffer-substring (point)
!                                                (progn (move-to-left-margin)
!                                                       (point)))))))))))))

  ;;;###autoload
  (defun comment-indent-new-line (&optional soft)
***************
*** 1179,1189 ****
  	    (setq comin (point))))

  	(cond
! 	 ;; If there's an adaptive prefix, use it unless we're inside
! 	 ;; a comment and the prefix is not a comment starter.
! 	 ((and fill-prefix
! 	       (or (not compos)
! 		   (comment-valid-prefix-p fill-prefix compos)))
  	  (indent-to-left-margin)
  	  (insert-and-inherit fill-prefix))
  	 ;; If we're not inside a comment, just try to indent.
--- 1213,1220 ----
  	    (setq comin (point))))

  	(cond
! 	 ;; If there's an adaptive prefix, use it provided it's valid.
! 	 ((and fill-prefix (comment-valid-prefix-p fill-prefix compos))
  	  (indent-to-left-margin)
  	  (insert-and-inherit fill-prefix))
  	 ;; If we're not inside a comment, just try to indent.

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

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: adaptive-fill-mode and auto-fill-mode

[Stefan Monnier]

>>> I fail to understand you here.  If the comment at compos doesn't match
>>> the prefix why should I want to insert the prefix on the next line?
>> Why not?

> Your own patch might impede you.  If you manually change the
> comment-start sequence at compos or the whitespace preceding it, the
> prefix won't match the comment at compos any more and hence won't be
> used.

I'm not sure what you're referring to.  Of course, if the comment-start is
different, I want to reject the prefix, and if the leading whitespace is of
different length, I also want to reject it.  Both of these are pretty clear
signs that the prefix was built from some other unrelated comment.
Of course, one can never be sure, but one has to guess.

> !            (save-excursion
> !              (goto-char compos)
> !              ;; The comstart marker is the same.
> !              (and (looking-at comment-start-skip)
> ! 		  (string-equal
> ! 		   prefix-comstart
> ! 		   (comment-string-strip (match-string 0) nil t))

AFAICT this is the only spot where you changed my suggested code.
What was the scenario where this change is needed?

BTW comment-start-skip can't be used like that in `looking-at' because it
may need to match some chars *before* the actual comment start.  This is
typically the case when it starts with
"\\(\\(^\\|[^\\\\\n]\\)\\(\\\\\\\\\\)*\\)" as in elisp.  So you need to
either narrow so as to pretend that compos is at BOL, or match from BOL and
prepend something like ".*".  Instead I just did (looking-at (regexp-quote
prefix-comstart)) which I thought should work just as well.


        Stefan

Re: adaptive-fill-mode and auto-fill-mode

[martin rudalics]

 >>>>I fail to understand you here.  If the comment at compos doesn't match
 >>>>the prefix why should I want to insert the prefix on the next line?
 >>>
 >>>Why not?
 >
 >
 >>Your own patch might impede you.  If you manually change the
 >>comment-start sequence at compos or the whitespace preceding it, the
 >>prefix won't match the comment at compos any more and hence won't be
 >>used.
 >
 >
 > I'm not sure what you're referring to.  Of course, if the comment-start is
 > different, I want to reject the prefix, and if the leading whitespace is of
 > different length, I also want to reject it.  Both of these are pretty clear
 > signs that the prefix was built from some other unrelated comment.
 > Of course, one can never be sure, but one has to guess.

You said it all here.  If the stripped comment-start doesn't match the
prefix it will be rejected, as in your patch.

 >>!            (save-excursion
 >>!              (goto-char compos)
 >>!              ;; The comstart marker is the same.
 >>!              (and (looking-at comment-start-skip)
 >>! 		  (string-equal
 >>! 		   prefix-comstart
 >>! 		   (comment-string-strip (match-string 0) nil t))
 >
 >
 > AFAICT this is the only spot

I also added arguments to the `string-match's, please check 'em.

 > where you changed my suggested code.
 > What was the scenario where this change is needed?

Scenarios where the stripped comment-start sequence is longer than the
prefix.  Otherwise, with

;; x
;; x
;;; xxxxxx .....

breaking the third comment would give something like

;; x
;; x
;;; xxxxxx .....
;; xxx

and breaking the third comment in

;; x
;;; x
;;; xxxxxx .....

something like

;; x
;;; x
;;; xxxxxx .....
;;xxxxxx

 > BTW comment-start-skip can't be used like that in `looking-at' because it
 > may need to match some chars *before* the actual comment start.  This is
 > typically the case when it starts with
 > "\\(\\(^\\|[^\\\\\n]\\)\\(\\\\\\\\\\)*\\)" as in elisp.  So you need to
 > either narrow so as to pretend that compos is at BOL, or match from BOL and
 > prepend something like ".*".  Instead I just did (looking-at (regexp-quote
 > prefix-comstart)) which I thought should work just as well.

The `looking-at' is a preparatory kludge to (a) make sure that we really
are at a valid comment-start and (b) produce the match-string for the
next conjunct.  The real test is obviously the following `string-equal'
(where I `comment-string-strip' trailing whitespace - newcomment.el is a
pocketful of miracles).  I don't care about leading whitespace because
compos is beyond that already.  Maybe you find a better solution.

Re: adaptive-fill-mode and auto-fill-mode

[Stefan Monnier]

>>> !            (save-excursion
>>> !              (goto-char compos)
>>> !              ;; The comstart marker is the same.
>>> !              (and (looking-at comment-start-skip)
>>> ! 		  (string-equal
>>> ! 		   prefix-comstart
>>> ! 		   (comment-string-strip (match-string 0) nil t))
>> 
>> 
>> AFAICT this is the only spot

> I also added arguments to the `string-match's, please check 'em.

I can't find any difference: I see 3 calls to string-match and they seem to
all be unchanged.  Oh I see, you meant match-string.  Thanks.

>> where you changed my suggested code.
>> What was the scenario where this change is needed?

> Scenarios where the stripped comment-start sequence is longer than the
> prefix.  Otherwise, with

> ;; x
> ;; x
> ;;; xxxxxx .....

> breaking the third comment would give something like

> ;; x
> ;; x
> ;;; xxxxxx .....
> ;; xxx

> and breaking the third comment in

> ;; x
> ;;; x
> ;;; xxxxxx .....

> something like

> ;; x
> ;;; x
> ;;; xxxxxx .....
> ;;xxxxxx

How serious are these?

>> BTW comment-start-skip can't be used like that in `looking-at' because it
>> may need to match some chars *before* the actual comment start.  This is
>> typically the case when it starts with
>> "\\(\\(^\\|[^\\\\\n]\\)\\(\\\\\\\\\\)*\\)" as in elisp.  So you need to
>> either narrow so as to pretend that compos is at BOL, or match from BOL and
>> prepend something like ".*".  Instead I just did (looking-at (regexp-quote
>> prefix-comstart)) which I thought should work just as well.

> The `looking-at' is a preparatory kludge to (a) make sure that we really
> are at a valid comment-start and (b) produce the match-string for the
> next conjunct.

I understand, but what I mean is that it may not do what you expect: e.g. it
may fail to match, even though you are indeed looking at a comment-starter.
The main problem with it, is that in modes where it fails, it'll tend to
fail every time :-(.
E.g. in elisp, go to the semi-colon in:

     (foo bar) ; Comment

and type M-: (looking-at-comment-start-skip) RET and you'll that it fails.
Luckily, the match succeeds (by accident) when you double the semi-colon.

> The real test is obviously the following `string-equal'
> (where I `comment-string-strip' trailing whitespace - newcomment.el is a
> pocketful of miracles).  I don't care about leading whitespace because
> compos is beyond that already.  Maybe you find a better solution.

Well, I prefer my (looking-at (regexp-quote prefix-comstart)), because even
though it's suboptimal, I believe that annoyance is small.  And it has the
benefit of being a lot simpler.

I installed the change.  Thanks for all your help,


        Stefan