someone broke message-newline-and-reformat

someone broke message-newline-and-reformat

[Vladimir Volovich]

Hi!

recently, the message-newline-and-reformat (M-RET) function became
broken. it used to work fine with SuperCite, but it no longer works.

copy the following cited paragraph into the new mail buffer, put a
cursor in the middle of the string, and press M-RET.

 AB> test test test test test test test test test test test test
 AB> test test test test test test test test test test test test
 AB> test test test test test test test test test test test test
 AB> test test test test test test test test test test test test
 AB> test test test test test test test test test test test test

Best,
v.

Re: someone broke message-newline-and-reformat

[dme]

* vvv@vvv.vsu.ru [2000-11-10 11:39:38]
| recently, the message-newline-and-reformat (M-RET) function became
| broken. it used to work fine with SuperCite, but it no longer works.

That was me.  I'll take a look at it.

dme.

Re: someone broke message-newline-and-reformat

[dme]

* vvv@vvv.vsu.ru [2000-11-10 11:39:38]
| recently, the message-newline-and-reformat (M-RET) function became
| broken. it used to work fine with SuperCite, but it no longer works.

This one works for me.  Please could you test and confirm:

Index: message.el
===================================================================
RCS file: /usr/local/cvsroot/gnus/lisp/message.el,v
retrieving revision 6.7
diff -c -r6.7 message.el
*** message.el	2000/11/04 17:13:56	6.7
--- message.el	2000/11/10 11:57:50
***************
*** 1711,1724 ****
  (defun message-newline-and-reformat ()
    "Insert four newlines, and then reformat if inside quoted text."
    (interactive)
!   (let ((prefix "[]>»|:}+ \t]*")
  	(supercite-thing "[-._a-zA-Z0-9]*[>]+[ \t]*")
  	quoted point)
      (unless (bolp)
        (save-excursion
  	(beginning-of-line)
! 	(when (looking-at (concat prefix "\\|"
! 				  supercite-thing))
  	  (setq quoted (match-string 0))))
        (insert "\n"))
      (setq point (point))
--- 1711,1724 ----
  (defun message-newline-and-reformat ()
    "Insert four newlines, and then reformat if inside quoted text."
    (interactive)
!   (let ((prefix "[]>»\|:}+ \t]*")
  	(supercite-thing "[-._a-zA-Z0-9]*[>]+[ \t]*")
  	quoted point)
      (unless (bolp)
        (save-excursion
  	(beginning-of-line)
! 	(when (or (looking-at supercite-thing)
! 		  (looking-at prefix))
  	  (setq quoted (match-string 0))))
        (insert "\n"))
      (setq point (point))

dme.

Re: someone broke message-newline-and-reformat

[Vladimir Volovich]

> This one works for me.  Please could you test and confirm:
> 
> *** message.el	2000/11/04 17:13:56	6.7
> --- message.el	2000/11/10 11:57:50
> ***************
> *** 1711,1724 ****
>   (defun message-newline-and-reformat ()
>     "Insert four newlines, and then reformat if inside quoted text."
>     (interactive)
> !   (let ((prefix "[]>»|:}+ \t]*")
>   	(supercite-thing "[-._a-zA-Z0-9]*[>]+[ \t]*")
>   	quoted point)
>       (unless (bolp)
>         (save-excursion
>   	(beginning-of-line)
> ! 	(when (looking-at (concat prefix "\\|"
> ! 				  supercite-thing))
>   	  (setq quoted (match-string 0))))
>         (insert "\n"))
>       (setq point (point))
> --- 1711,1724 ----
>   (defun message-newline-and-reformat ()
>     "Insert four newlines, and then reformat if inside quoted text."
>     (interactive)
> !   (let ((prefix "[]>»\|:}+ \t]*")
>   	(supercite-thing "[-._a-zA-Z0-9]*[>]+[ \t]*")
>   	quoted point)
>       (unless (bolp)
>         (save-excursion
>   	(beginning-of-line)
> ! 	(when (or (looking-at supercite-thing)
> ! 		  (looking-at prefix))
>   	  (setq quoted (match-string 0))))
>         (insert "\n"))
>       (setq point (point))

i have some questions/suggestions:

1) you changed
  (let ((prefix "[]>»|:}+ \t]*")
to
  (let ((prefix "[]>»\|:}+ \t]*")

But this seems to be not needed (i.e. a backslash before | is not
needed):

(string-equal
  "[]>»|:}+ \t]*"
  "[]>»\|:}+ \t]*")
t

2) (looking-at prefix) will ALWAYS return t because ANY string matches
   a regexp of form [something]*
   so the check for (looking-at prefix) is broken.

   i think that this regexp could be changed to something like
   []>»|:}+]+[ \t]*

   BTW, the broken regexp for prefix was THE reason why your patch
   broke message-newline-and-reformat: the resulting regexp

   <value of prefix>\\|<value of supercite-thing>

   gave a null string match.

3) (supercite-thing "[-._a-zA-Z0-9]*[>]+[ \t]*")
   could be changed (simplified) to
   (supercite-thing "[-._a-zA-Z0-9]*>+[ \t]*")

4) the trick of changing 
  (looking-at (concat prefix "\\|" supercite-thing))
to
  (or (looking-at supercite-thing) (looking-at prefix))
seems to me like a hack.

  of course the original form (the one which you added some time ago)
  should work PROVIDED that prefix is sensibly defined AND
  supercite-thing comes before prefix (because otherwise prefix could
  find a smaller match)

5) the whole concat thing (and assigning prefix and supercite-thing
   variables) is not necessary, and you could just put a ready-to-use
   regexp in looking-at's argument.

6) looking only for latin letters (a-zA-Z) is not necessary (e.g. i
   often need to answer letters written by people with cyrillic
   initials), so it is better to change it to \w.

7) here is a resulting message-newline-and-reformat which seems to
   work much better for me:

(defun message-newline-and-reformat ()
  "Insert four newlines, and then reformat if inside quoted text."
  (interactive)
  (let (quoted point)
    (unless (bolp)
      (save-excursion
        (beginning-of-line)
        (when (looking-at " *\\(\\w\\|[-._]\\)*>+[ \t]*\\|[]>»|:}+ \t]*")
          (setq quoted (match-string 0))))
      (insert "\n"))
    (setq point (point))
    (insert "\n\n\n")
    (delete-region (point) (re-search-forward "[ \t]*"))
    (when quoted
      (insert quoted))
    (fill-paragraph nil)
    (goto-char point)
    (forward-line 1)))

Best,
v.

Re: someone broke message-newline-and-reformat

[dme]

* vvv@vvv.vsu.ru [2000-11-10 15:16:44]
| 7) here is a resulting message-newline-and-reformat which seems to
|    work much better for me:

This works for all of the tests that I tried (didn't include any
non-ascii), so I'm happy to have that committed.

dme.

Re: someone broke message-newline-and-reformat

[Kai Großjohann]

On 10 Nov 2000, Vladimir Volovich wrote:

>         (when (looking-at " *\\(\\w\\|[-._]\\)*>+[ \t]*\\|[]>»|:}+ \t]*")

I'd be glad if these regexes weren't hard-coded.  There are variables
gnus-cite-prefix-regexp and gnus-supercite-regexp, as well as
gnus-supercite-secondary-regexp.  Can you make use of them in some
way?  Maybe one of these needs to be changed so that
message-newline-and-reformat works correctly -- then the required
change is probably also useful for the other applications.

Another idea is that we might need to move gnus-cite-prefix-regexp and
gnus-supercite-regexp to message-* variables since message.el is
supposed to be usable without Gnus (but not vice versa).

Right now, regexes which are supposed to match citation prefixes are
scattered about the Gnus source code in various places -- this isn't a
healthy situation, imvho.

Thoughts?

kai
-- 
The arms should be held in a natural and unaffected way and never
be conspicuous. -- Revised Technique of Latin American Dancing

Re: someone broke message-newline-and-reformat

[dme]

* Kai.Grossjohann@CS.Uni-Dortmund.DE [2000-11-10 16:52:02]
| On 10 Nov 2000, Vladimir Volovich wrote:
| >         (when (looking-at " *\\(\\w\\|[-._]\\)*>+[ \t]*\\|[]>»|:}+ \t]*")
| 
| I'd be glad if these regexes weren't hard-coded.  There are variables
| gnus-cite-prefix-regexp and gnus-supercite-regexp, as well as
| gnus-supercite-secondary-regexp.

gnus-cite-prefix-regexp is the relevant one (the others match
supercite attribution lines rather than body citation).  With two
small additional changes (append space when inserting and use \w
rather than . (as per Vladimir)) this appears to work satisfactorily
for both supercite and `normal' citation prefixes.

| Another idea is that we might need to move gnus-cite-prefix-regexp and
| gnus-supercite-regexp to message-* variables since message.el is
| supposed to be usable without Gnus (but not vice versa).

I've done that and added (require 'message) to gnus-cite.el - is there
a better way to do this ?  I'll post a diff after some more testing.

The documentation also changes slightly.

dme.

Re: someone broke message-newline-and-reformat

[Kai Großjohann]

On 11 Nov 2000, dme@dme.org wrote:
> * Kai.Grossjohann@CS.Uni-Dortmund.DE [2000-11-10 16:52:02]
> 
>| Another idea is that we might need to move gnus-cite-prefix-regexp
>| and gnus-supercite-regexp to message-* variables since message.el
>| is supposed to be usable without Gnus (but not vice versa).
> 
> I've done that and added (require 'message) to gnus-cite.el - is
> there a better way to do this ?

That seems to be the right way.  Thanks a lot!

kai
-- 
The arms should be held in a natural and unaffected way and never
be conspicuous. -- Revised Technique of Latin American Dancing

Re: someone broke message-newline-and-reformat

[dme]

Here's the patch mentioned.

I'd be obliged of a couple of people could test this before it is
committed - particularly those who use or see non-us-ascii citation
leaders (as indicated by Vladimir).

Index: lisp/ChangeLog
===================================================================
RCS file: /usr/local/cvsroot/gnus/lisp/ChangeLog,v
retrieving revision 6.91
diff -u -r6.91 ChangeLog
--- lisp/ChangeLog	2000/11/10 11:06:52	6.91
+++ lisp/ChangeLog	2000/11/12 09:40:13
@@ -1,3 +1,15 @@
+2000-11-12  David Edmondson  <dme@dme.org>
+
+	* message.el (message-cite-prefix-regexp): moved from gnus-cite.el
+	and replace `.' with `\w' to allow for different syntax tables
+	(from Vladimir Volovich).
+	* message.el (message-newline-and-reformat): use
+	`message-cite-prefix-regexp'.
+	* gnus-cite.el (gnus-supercite-regexp): use
+	`message-cite-prefix-regexp'.
+	* gnus-cite.el (gnus-cite-parse): use
+	`message-cite-prefix-regexp'.
+
 2000-11-07  Kai Großjohann  <Kai.Grossjohann@CS.Uni-Dortmund.DE>
 
 	* gnus-sum.el (gnus-move-split-methods): Say that
Index: lisp/gnus-cite.el
===================================================================
RCS file: /usr/local/cvsroot/gnus/lisp/gnus-cite.el,v
retrieving revision 6.0
diff -u -r6.0 gnus-cite.el
--- lisp/gnus-cite.el	2000/10/27 23:11:23	6.0
+++ lisp/gnus-cite.el	2000/11/12 09:40:22
@@ -26,6 +26,7 @@
 (require 'gnus)
 (require 'gnus-art)
 (require 'gnus-range)
+(require 'message)	; for message-cite-prefix-regexp
 
 ;;; Customization:
 
@@ -74,19 +75,13 @@
   :type '(choice (const :tag "all" nil)
 		 integer))
 
-(defcustom gnus-cite-prefix-regexp
-  "^[]>»|:}+ ]*[]>»|:}+]\\(.*>»\\)?\\|^.*>"
-  "*Regexp matching the longest possible citation prefix on a line."
-  :group 'gnus-cite
-  :type 'regexp)
-
 (defcustom gnus-cite-max-prefix 20
   "Maximum possible length for a citation prefix."
   :group 'gnus-cite
   :type 'integer)
 
 (defcustom gnus-supercite-regexp
-  (concat "^\\(" gnus-cite-prefix-regexp "\\)? *"
+  (concat "^\\(" message-cite-prefix-regexp "\\)? *"
 	  ">>>>> +\"\\([^\"\n]+\\)\" +==")
   "*Regexp matching normal Supercite attribution lines.
 The first grouping must match prefixes added by other packages."
@@ -306,7 +301,7 @@
 corresponding citation merged with `gnus-cite-attribution-face'.
 
 Text is considered cited if at least `gnus-cite-minimum-match-count'
-lines matches `gnus-cite-prefix-regexp' with the same prefix.
+lines matches `message-cite-prefix-regexp' with the same prefix.
 
 Lines matching `gnus-cite-attribution-suffix' and perhaps
 `gnus-cite-attribution-prefix' are considered attribution lines."
@@ -679,7 +674,7 @@
       ;; Ignore very long prefixes.
       (when (> end (+ (point) gnus-cite-max-prefix))
 	(setq end (+ (point) gnus-cite-max-prefix)))
-      (while (re-search-forward gnus-cite-prefix-regexp (1- end) t)
+      (while (re-search-forward message-cite-prefix-regexp (1- end) t)
 	;; Each prefix.
 	(setq end (match-end 0)
 	      prefix (buffer-substring begin end))
Index: lisp/message.el
===================================================================
RCS file: /usr/local/cvsroot/gnus/lisp/message.el,v
retrieving revision 6.7
diff -u -r6.7 message.el
--- lisp/message.el	2000/11/04 17:13:56	6.7
+++ lisp/message.el	2000/11/12 09:41:07
@@ -331,6 +331,12 @@
   :group 'message-insertion
   :type 'regexp)
 
+(defcustom message-cite-prefix-regexp
+  "^[]>»|:}+ ]*[]>»|:}+]\\(\\w*>»\\)?\\|^\\w*>"
+  "*Regexp matching the longest possible citation prefix on a line."
+  :group 'message-insertion
+  :type 'regexp)
+
 (defcustom message-cancel-message "I am canceling my own article.\n"
   "Message to be inserted in the cancel message."
   :group 'message-interface
@@ -1711,21 +1717,18 @@
 (defun message-newline-and-reformat ()
   "Insert four newlines, and then reformat if inside quoted text."
   (interactive)
-  (let ((prefix "[]>»|:}+ \t]*")
-	(supercite-thing "[-._a-zA-Z0-9]*[>]+[ \t]*")
-	quoted point)
+  (let (quoted point)
     (unless (bolp)
       (save-excursion
-	(beginning-of-line)
-	(when (looking-at (concat prefix "\\|"
-				  supercite-thing))
-	  (setq quoted (match-string 0))))
+        (beginning-of-line)
+	(when (looking-at message-cite-prefix-regexp)
+          (setq quoted (match-string 0))))
       (insert "\n"))
     (setq point (point))
     (insert "\n\n\n")
     (delete-region (point) (re-search-forward "[ \t]*"))
     (when quoted
-      (insert quoted))
+      (insert quoted " "))
     (fill-paragraph nil)
     (goto-char point)
     (forward-line 1)))
Index: texi/ChangeLog
===================================================================
RCS file: /usr/local/cvsroot/gnus/texi/ChangeLog,v
retrieving revision 6.8
diff -u -r6.8 ChangeLog
--- texi/ChangeLog	2000/11/10 11:06:46	6.8
+++ texi/ChangeLog	2000/11/12 09:41:15
@@ -1,3 +1,10 @@
+2000-11-12  David Edmondson  <dme@dme.org>
+
+	* gnus.texi: remove `gnus-cite-prefix-regexp'.
+
+	* message.texi (Insertion): move `gnus-cite-prefix-regexp' from
+	gnus.texi to here and rename to `message-cite-prefix-regexp'.
+
 2000-11-07  Kai Großjohann  <Kai.Grossjohann@CS.Uni-Dortmund.DE>
 
 	* gnus.texi (Mail Group Commands): `gnus-move-split-methods' uses
Index: texi/gnus.texi
===================================================================
RCS file: /usr/local/cvsroot/gnus/texi/gnus.texi,v
retrieving revision 6.10
diff -u -r6.10 gnus.texi
--- texi/gnus.texi	2000/11/10 11:06:42	6.10
+++ texi/gnus.texi	2000/11/12 09:44:48
@@ -6995,10 +6995,6 @@
 If the article size if bigger than this variable (which is 25000 by
 default), no citation highlighting will be performed.
 
-@item gnus-cite-prefix-regexp
-@vindex gnus-cite-prefix-regexp
-Regexp matching the longest possible citation prefix on a line.
-
 @item gnus-cite-max-prefix
 @vindex gnus-cite-max-prefix
 Maximum possible length for a citation prefix (default 20).
Index: texi/message.texi
===================================================================
RCS file: /usr/local/cvsroot/gnus/texi/message.texi,v
retrieving revision 6.0
diff -u -r6.0 message.texi
--- texi/message.texi	2000/10/27 23:12:30	6.0
+++ texi/message.texi	2000/11/12 09:45:00
@@ -464,6 +464,10 @@
 messages.  The default is @samp{.}, which means that all headers will be
 removed.
 
+@item message-cite-prefix-regexp
+@vindex message-cite-prefix-regexp
+Regexp matching the longest possible citation prefix on a line.
+
 @item message-citation-line-function
 @vindex message-citation-line-function
 Function called to insert the citation line.  The default is

dme.

Re: someone broke message-newline-and-reformat

[Vladimir Volovich]

David,

your recent changes to gnus DID NOT fix the problem.
M-RET still does NOT work.

did you ever test your patch before committing it?!

please test with the following two paragraphs (try to put a cursor
somewhere in the middle of line between words, and press M-RET), and
see that your patch completely messes up the citation!

 AB> test test test test test test test test test test test test
 AB> test test test test test test test test test test test test
 AB> test test test test test test test test test test test test
 AB> test test test test test test test test test test test test
 AB> test test test test test test test test test test test test

 АБ> тест тест тест тест тест тест тест тест тест тест тест тест
 АБ> тест тест тест тест тест тест тест тест тест тест тест тест
 АБ> тест тест тест тест тест тест тест тест тест тест тест тест
 АБ> тест тест тест тест тест тест тест тест тест тест тест тест
 АБ> тест тест тест тест тест тест тест тест тест тест тест тест

i see two problems with your patch:

1) a hack in message-newline-and-reformat that should be removed
   (reverted):

-      (insert quoted))
+      (insert quoted " "))

2) the new value of message-cite-prefix-regexp is the reason why
   message-newline-and-reformat still does not work. i propose the
   following:

(setq message-cite-prefix-regexp "[ \t]*\\w*>+[ \t]*\\|[ \t]*[]>»|:}+ ]*[]>»|:}+][ \t]*")

or

(setq message-cite-prefix-regexp "[ \t]*\\(\\w\\|[-._]\\)*>+[ \t]*\\|[ \t]*[]>»|:}+ ]*[]>»|:}+][ \t]*")

Best,
v.

Re: someone broke message-newline-and-reformat

[dme]

* vvv@vvv.vsu.ru [2000-11-13 10:49:07]
| did you ever test your patch before committing it?!

Obviously not sufficiently ! The change isn't committed yet.

| 1) a hack in message-newline-and-reformat that should be removed
|    (reverted):
| 
| -      (insert quoted))
| +      (insert quoted " "))

The regexp I used didn't consume the space(s) at the end of the
citation in the tests, so this was required.

| 2) the new value of message-cite-prefix-regexp is the reason why
|    message-newline-and-reformat still does not work. i propose the
|    following:
| 
| (setq message-cite-prefix-regexp "[ \t]*\\w*>+[ \t]*\\|[
| \t]*[]>»|:}+ ]*[]>»|:}+][ \t]*")

This works for m-n-a-f, but it has a bad effect if used for citation
highlighting in message-mode (something else I've been looking at).
Sigh.

| (setq message-cite-prefix-regexp "[ \t]*\\(\\w\\|[-._]\\)*>+[
| \t]*\\|[ \t]*[]>»|:}+ ]*[]>»|:}+][ \t]*")

I'll play with the two new regexps and post another patch later.

dme.

Re: someone broke message-newline-and-reformat

[Vladimir Volovich]

 > i propose the following:
 > (setq message-cite-prefix-regexp "[ \t]*\\w*>+[ \t]*\\|[ \t]*[]>»|:}+ ]*[]>»|:}+][ \t]*")
 > or
 > (setq message-cite-prefix-regexp "[ \t]*\\(\\w\\|[-._]\\)*>+[ \t]*\\|[ \t]*[]>»|:}+ ]*[]>»|:}+][ \t]*")

please change `*' after \\w to `+':

(setq message-cite-prefix-regexp "[ \t]*\\w+>+[ \t]*\\|[ \t]*[]>»|:}+ ]*[]>»|:}+][ \t]*")
or
(setq message-cite-prefix-regexp "[ \t]*\\(\\w\\|[-._]\\)+>+[ \t]*\\|[ \t]*[]>»|:}+ ]*[]>»|:}+][ \t]*")

(the second form allows characters `-', `.', `_' in citation names as
well as `word' characters).

Best,
v.

someone broke message-newline-and-reformat

[Vladimir Volovich]

Hi!

 > recently, the message-newline-and-reformat (M-RET) function became
 > broken. it used to work fine with SuperCite, but it no longer works.

the bug was introduced by the following patch:

2000-09-29  David Edmondson  <dme@thus.net>

        One-line patch.
        
        * message.el (message-newline-and-reformat): Typo.

$ cvs diff -u -r6.5 -r6.6 message.el 
Index: message.el
===================================================================
RCS file: /usr/local/cvsroot/gnus/lisp/message.el,v
retrieving revision 6.5
retrieving revision 6.6
diff -u -r6.5 -r6.6
--- message.el  2000/11/04 13:35:32     6.5
+++ message.el  2000/11/04 14:21:04     6.6
@@ -1716,7 +1716,7 @@
     (unless (bolp)
       (save-excursion
        (beginning-of-line)
-       (when (looking-at (concat prefix
+       (when (looking-at (concat prefix "\\|"
                                  supercite-thing))
          (setq quoted (match-string 0))))
       (insert "\n"))

Could you please revert the buggy patch?

Best,
v.

Re: someone broke message-newline-and-reformat

[Vladimir Volovich]

Hi,

message-newline-and-reformat still behaves not good: it removes any
spaces betseen attribution and cited text after reciting.

well, is it really that hard to fix message-newline-and-reformat once
and for all? :-)

if you cannot make the regexp variables sharable between
message-newline-and-reformat and other purposes, then just do not
share them: i've already posted a regexp for
message-newline-and-reformat which works nice.

Best,
v.

Re: someone broke message-newline-and-reformat

[dme]

* vvv@vvv.vsu.ru [2000-11-17 11:42:58]
> message-newline-and-reformat still behaves not good: it removes any
> spaces betseen attribution and cited text after reciting.

ShengHou seems to have deliberately disabled this:

2000-11-16 17:00  ShengHuo ZHU  <zsh@cs.rochester.edu>

	* message.el (message-cite-prefix-regexp): Prefix should not end
	at space.

but at the moment I don't really know why.

dme.

Re: someone broke message-newline-and-reformat

[ShengHuo ZHU]

dme@dme.org writes:

> * vvv@vvv.vsu.ru [2000-11-17 11:42:58]
> > message-newline-and-reformat still behaves not good: it removes any
> > spaces betseen attribution and cited text after reciting.
> 
> ShengHou seems to have deliberately disabled this:
> 
> 2000-11-16 17:00  ShengHuo ZHU  <zsh@cs.rochester.edu>
> 
> 	* message.el (message-cite-prefix-regexp): Prefix should not end
> 	at space.
> 
> but at the moment I don't really know why.

Otherwise, "> " and ">   " are treated as different citations.

ShengHuo

Re: someone broke message-newline-and-reformat

[ShengHuo ZHU]

Vladimir Volovich <vvv@vvv.vsu.ru> writes:

> Hi,
> 
> message-newline-and-reformat still behaves not good: it removes any
> spaces betseen attribution and cited text after reciting.
> 
> well, is it really that hard to fix message-newline-and-reformat once
> and for all? :-)
> 
> if you cannot make the regexp variables sharable between
> message-newline-and-reformat and other purposes, then just do not
> share them: i've already posted a regexp for
> message-newline-and-reformat which works nice.

Fixed.

ShengHuo