Re: Top posting related changes in the cvs

Re: Top posting related changes in the cvs

[Reiner Steib]

On Tue, Apr 03 2007, Gabor Z. Papp wrote:

> I have updated my cvs tree just today, earlier used a checkout from 20070223.
>
> Discovered, that something changed in lisp/message.el, related to the
> Top posting. When I'm replying to a message, in the reply buffer the
> cursor points after the citation, not like earlier at the bottom of
> the citation.

(You meant "start of the citation" in the last part?)

It has nothing to do with the top posting code.  The top posting code
is new in No Gnus, but the change that causes the new (bottom) cursor
position came from a change in the stable (v5-10) branch:

,----
| 2007-03-27  Thien-Thi Nguyen  <ttn@gnu.org>
| 
| 	* message.el (message-yank-original): Fix bug:
| 	Don't switch point and mark unnecessarily.
`----

Thien-Thi, could you please explain which bug you intended to fix?  To
put the cursor immediately after the citation line ("Whomever writes:"
line).  The user should start editing there, removing the cited
portions the he won't reply to.

When doing changes in Gnus it would be nice to dicuss such changes on
ding@gnus.org - especially WRT the upcoming Emacs release (in this
case, I didn't even see any discussion on emacs-devel or on the
pretest-list).

Unless there is a very good reason for this change, I intend to revert
it.  I certainly don't want Gnus in Emacs 22 behave this way by
default!

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Top posting related changes in the cvs

[Thien-Thi Nguyen]

() Reiner Steib <reinersteib+gmane@imap.cc>
() Tue, 03 Apr 2007 22:45:08 +0200

   ,----
   | 2007-03-27  Thien-Thi Nguyen  <ttn@gnu.org>
   | 
   | 	* message.el (message-yank-original): Fix bug:
   | 	Don't switch point and mark unnecessarily.
   `----

   Thien-Thi, could you please explain which bug you intended to fix?
   To put the cursor immediately after the citation line ("Whomever
   writes:" line).  The user should start editing there, removing the
   cited portions the he won't reply to.

before the fix, yanking would leave the cursor after the yanked
messages, contrary to the docstring.  the fix, to regain concordance w/
the docstring, involves conditionalizing a previously unconditional call
to exchange-point-and-mark.  i believe the original unconditional call
was correct (necessary) prior to another change:

2005-08-01  Katsumi Yamaoka  <yamaoka@jpl.org>

  * gnus-uu.el (gnus-uu-save-article): Use insert-buffer-substring
    instead of insert-buffer.

  * message.el (message-yank-original): Ditto; set the mark at the
    end of the yanked message.

insert-buffer-substring inserts before point, while insert-buffer
inserts after point.  my analysis is that the exchange-point-and-mark
relied on insert-buffer's behavior, and thus became incorrect after the
2005-08-01 change, since point and mark were already in the correct
order.

my fix (to conditionalize the call) removes the direct dependency so
that the desired ending position of point will be as documented for
whatever function is used (now or in the future) to do the insert, as
long as the inserted text is bracketed by point and mark.

an alternative fix that is less robust against future changes is to rely
on the behavior of insert-buffer-substring and simply remove the call to
exchange-point-and-mark.  then we can chase this bug again later for
hours of fun...

   When doing changes in Gnus it would be nice to dicuss such changes on
   ding@gnus.org - especially WRT the upcoming Emacs release (in this
   case, I didn't even see any discussion on emacs-devel or on the
   pretest-list).

   Unless there is a very good reason for this change, I intend to
   revert it.  I certainly don't want Gnus in Emacs 22 behave this way
   by default!

i think the behavior, leaving point between the citation and the yanked
text, is nice.  i was happy to read about it in the docstring.  it seems
whoever wrote the original (concording) code+docstring was thoughtful.

thi

Re: Top posting related changes in the cvs

[Reiner Steib]

On Tue, Apr 03 2007, Thien-Thi Nguyen wrote:

> before the fix, yanking would leave the cursor after the yanked
> messages, contrary to the docstring.  the fix, to regain concordance w/
> the docstring, involves conditionalizing a previously unconditional call
> to exchange-point-and-mark.  i believe the original unconditional call
> was correct (necessary) prior to another change:
>
> 2005-08-01  Katsumi Yamaoka  <yamaoka@jpl.org>
>
>   * gnus-uu.el (gnus-uu-save-article): Use insert-buffer-substring
>     instead of insert-buffer.
>
>   * message.el (message-yank-original): Ditto; set the mark at the
>     end of the yanked message.
>
> insert-buffer-substring inserts before point, while insert-buffer
> inserts after point.  my analysis is that the exchange-point-and-mark
> relied on insert-buffer's behavior, and thus became incorrect after the
> 2005-08-01 change, since point and mark were already in the correct
> order.
>
> my fix (to conditionalize the call) removes the direct dependency so
> that the desired ending position of point will be as documented for
> whatever function is used (now or in the future) to do the insert, as
> long as the inserted text is bracketed by point and mark.
>
> an alternative fix that is less robust against future changes is to rely
> on the behavior of insert-buffer-substring and simply remove the call to
> exchange-point-and-mark.  then we can chase this bug again later for
> hours of fun...
[...]
> i think the behavior, leaving point between the citation and the yanked
> text, is nice.  i was happy to read about it in the docstring.  it seems
> whoever wrote the original (concording) code+docstring was thoughtful.

I gather that you refer to an interactive call of
`message-yank-original' (C-c C-y) after doing a reply/followup without
citing the original (using `r'/`f').  However, when using `R'/`F', I
don't get this behavior anymore: Point is _below_ the quote, which is
wrong.

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Top posting related changes in the cvs

[Thien-Thi Nguyen]

() Reiner Steib <reinersteib+gmane@imap.cc>
() Wed, 04 Apr 2007 00:05:29 +0200

   I gather that you refer to an interactive call of
   `message-yank-original' (C-c C-y) after doing a reply/followup
   without citing the original (using `r'/`f').

yes.
                                         
   However, when using `R'/`F', I don't get this behavior anymore:
   Point is _below_ the quote, which is wrong.

patch below seems to fix `R'/`F' w/ light testing.
here is a ChangeLog entry:

        * gnus-msg.el (gnus-inews-yank-articles):
        Fix bug: Make sure point is at eob after yanking.

probably all callers of `message-yank-original' should be checked.
does that give good results?

thi



*** gnus-msg.el	31 Mar 2007 09:43:42 -0000	1.38
--- gnus-msg.el	3 Apr 2007 22:57:22 -0000
***************
*** 771,776 ****
--- 771,777 ----
  		 (nnheader-narrow-to-headers)
  		 (nnheader-parse-naked-head)))))
  	(message-yank-original)
+ 	(exchange-point-and-mark)       ; point to eob
  	(setq beg (or beg (mark t))))
        (when articles
  	(insert "\n")))

Re: Top posting related changes in the cvs

[Gabor Z. Papp]

* Thien-Thi Nguyen <ttn@gnuvola.org>:

|         * gnus-msg.el (gnus-inews-yank-articles):
|         Fix bug: Make sure point is at eob after yanking.

| probably all callers of `message-yank-original' should be checked.
| does that give good results?

Here yes, thanks.

Re: Top posting related changes in the cvs

[Katsumi Yamaoka]

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

Hi,

I'm sorry for the late response (I was absent because of flu).

>>>>> In <873b3hm942.fsf@ambire.localdomain> Thien-Thi Nguyen wrote:

> 2005-08-01  Katsumi Yamaoka  <yamaoka@jpl.org>

>   * gnus-uu.el (gnus-uu-save-article): Use insert-buffer-substring
>     instead of insert-buffer.

>   * message.el (message-yank-original): Ditto; set the mark at the
>     end of the yanked message.

I made those changes to obey the doc string of `insert-buffer':

#v+
This function is meant for the user to run interactively.
Don't call it from programs: use `insert-buffer-substring' instead!
#v-

> insert-buffer-substring inserts before point, while insert-buffer
> inserts after point.

Sure.  Therefore, I've made it behave like `insert-buffer' as
follows:

#v+
      (push-mark (save-excursion
		   (insert-buffer-substring message-reply-buffer)
		   (point)))
#v-

(This is just what `insert-buffer' defined in simple.el does.)

> my analysis is that the exchange-point-and-mark relied on
> insert-buffer's behavior, and thus became incorrect after the
> 2005-08-01 change, since point and mark were already in the correct
> order.

Anyway, I verified that, before this change, there has already
been the problem that happens when `message-yank-original' is
called interactively.  So, I'm not a culprit. ;-)

>>>>> In <87ps6ljc3q.fsf@ambire.localdomain> Thien-Thi Nguyen wrote:

> () Reiner Steib <reinersteib+gmane@imap.cc>
> () Wed, 04 Apr 2007 00:05:29 +0200

>    I gather that you refer to an interactive call of
>    `message-yank-original' (C-c C-y) after doing a reply/followup
>    without citing the original (using `r'/`f').

> yes.

>    However, when using `R'/`F', I don't get this behavior anymore:
>    Point is _below_ the quote, which is wrong.

> patch below seems to fix `R'/`F' w/ light testing.
> here is a ChangeLog entry:

>         * gnus-msg.el (gnus-inews-yank-articles):
>         Fix bug: Make sure point is at eob after yanking.

> probably all callers of `message-yank-original' should be checked.
> does that give good results?

I think the recent change in `message-yank-original' and this
fix are the right solutions for Gnus v5.11.  However, for No
Gnus, it doesn't solve the problem in the case where
`message-cite-reply-above' is non-nil and `message-yank-original'
is called interactively.  How about this one?

	* message.el (message-yank-original): Don't exchange point and mark.

I also removed the `(unless (< (point) (mark-marker)) ...)' test
since I think it will never return nil.


[-- Attachment #2: patch for Gnus v5.11 --]
[-- Type: text/plain, Size: 1044 bytes --]

*** message.el~	Tue Mar 27 21:43:24 2007
--- message.el	Mon Apr  9 06:35:11 2007
***************
*** 3346,3358 ****
        (delete-windows-on message-reply-buffer t)
        (push-mark (save-excursion
  		   (insert-buffer-substring message-reply-buffer)
  		   (point)))
        (unless arg
! 	(funcall message-cite-function))
!       (unless (< (point) (mark-marker))
!         (message-exchange-point-and-mark))
!       (unless (bolp)
! 	(insert ?\n))
        (unless modified
  	(setq message-checksum (message-checksum))))))
  
--- 3346,3361 ----
        (delete-windows-on message-reply-buffer t)
        (push-mark (save-excursion
  		   (insert-buffer-substring message-reply-buffer)
+ 		   (unless (bolp)
+ 		     (insert ?\n))
  		   (point)))
        (unless arg
! 	(funcall message-cite-function)
! 	(unless (eq (char-before (mark t)) ?\n)
! 	  (let ((pt (point)))
! 	    (goto-char (mark t))
! 	    (insert-before-markers ?\n)
! 	    (goto-char pt))))
        (unless modified
  	(setq message-checksum (message-checksum))))))
  

[-- Attachment #3: patch for No Gnus --]
[-- Type: text/plain, Size: 1828 bytes --]

*** gnus-msg.el~	Wed Mar 28 02:31:28 2007
--- gnus-msg.el	Mon Apr  9 06:35:11 2007
***************
*** 789,794 ****
--- 789,795 ----
  		 (nnheader-narrow-to-headers)
  		 (nnheader-parse-naked-head)))))
  	(message-yank-original)
+ 	(exchange-point-and-mark)
  	(setq beg (or beg (mark t))))
        (when articles
  	(insert "\n")))
*** message.el~	Sun Apr  1 21:54:14 2007
--- message.el	Mon Apr  9 06:35:11 2007
***************
*** 3550,3569 ****
        (delete-windows-on message-reply-buffer t)
        (push-mark (save-excursion
  		   (insert-buffer-substring message-reply-buffer)
  		   (point)))
        (unless arg
! 	(funcall message-cite-function))
!       (if message-cite-reply-above
! 	  (progn
! 	    (message-goto-body)
! 	    (insert body-text)
! 	    (newline)
! 	    (message-goto-body)
! 	    (message-exchange-point-and-mark))
! 	(unless (< (point) (mark-marker))
! 	  (message-exchange-point-and-mark)))
!       (unless (bolp)
! 	(insert ?\n))
        ;; Add a `message-setup-very-last-hook' here?
        ;; Add `gnus-article-highlight-citation' here?
        (unless modified
--- 3550,3572 ----
        (delete-windows-on message-reply-buffer t)
        (push-mark (save-excursion
  		   (insert-buffer-substring message-reply-buffer)
+ 		   (unless (bolp)
+ 		     (insert ?\n))
  		   (point)))
        (unless arg
! 	(funcall message-cite-function)
! 	(unless (eq (char-before (mark t)) ?\n)
! 	  (let ((pt (point)))
! 	    (goto-char (mark t))
! 	    (insert-before-markers ?\n)
! 	    (goto-char pt))))
!       (when message-cite-reply-above
! 	(message-goto-body)
! 	(insert body-text)
! 	(unless (bolp)
! 	  (insert ?\n))
! 	(newline)
! 	(message-goto-body))
        ;; Add a `message-setup-very-last-hook' here?
        ;; Add `gnus-article-highlight-citation' here?
        (unless modified

[-- Attachment #4: Type: text/plain, Size: 10 bytes --]


Regards,

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

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

Re: Top posting related changes in the cvs

[Thien-Thi Nguyen]

() Katsumi Yamaoka <yamaoka@jpl.org>
() Mon, 09 Apr 2007 15:40:40 +0900

   Anyway, I verified that, before this change, there has already
   been the problem that happens when `message-yank-original' is
   called interactively.  So, I'm not a culprit. ;-)

my bad.  i apologize for the mis-analysis.  i guess the point is
that it was a long-standing discrepency masked by a long-standing
discrepency workaround.

   >         * gnus-msg.el (gnus-inews-yank-articles):
   >         Fix bug: Make sure point is at eob after yanking.

   I think the recent change in `message-yank-original' and this
   fix are the right solutions for Gnus v5.11.  However, for No
   Gnus, it doesn't solve the problem in the case where
   `message-cite-reply-above' is non-nil and `message-yank-original'
   is called interactively.  How about this one?

           * message.el (message-yank-original):
           Don't exchange point and mark.

   I also removed the `(unless (< (point) (mark-marker)) ...)' test
   since I think it will never return nil.

i have no opinion on No Gnus, just on the Gnus distributed w/ Emacs
(that i can play around with (and break, apparently)).

thi

Re: Top posting related changes in the cvs

[Reiner Steib]

On Mon, Apr 09 2007, Katsumi Yamaoka wrote:

>>>>>> In <873b3hm942.fsf@ambire.localdomain> Thien-Thi Nguyen wrote:
>>         * gnus-msg.el (gnus-inews-yank-articles):
>>         Fix bug: Make sure point is at eob after yanking.
>
>> probably all callers of `message-yank-original' should be checked.
>> does that give good results?
>
> I think the recent change in `message-yank-original' and this
> fix are the right solutions for Gnus v5.11.
[...]
> [6. patch for Gnus v5.11 --- text/plain; patch-for-Gnus-v5_11.txt]

I'm not sure if I understand correctly.  Are you saying we need this
patch in addition to the current code in Emacs (i.e. Gnus v5.11;
lisp/gnus/message.el rev. 1.117)?

If so, we should install it ASAP so that it gets more testing before
and during the next (final?) pretest.

Are you sure about the use of `(insert ?\n)' vs. `(newline)',
i.e. soft or hard newline?  (I'm not sure; that's why I ask. :-))

,----[ <f1> f newline RET ]
| newline is an interactive compiled Lisp function in `simple'.
| It is bound to RET.
| (newline &optional arg)
| 
| Insert a newline, and move to left margin of the new line if it's blank.
| If `use-hard-newlines' is non-nil, the newline is marked with the
| text-property `hard'.
| With arg, insert that many newlines.
| Call `auto-fill-function' if the current column number is greater
| than the value of `fill-column' and arg is nil.
`----

Bye, Reiner.
-- 
       ,,,
      (o o)
---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/

Re: Top posting related changes in the cvs

[Katsumi Yamaoka]

>>>>> In <873b39bdti.fsf@ambire.localdomain> Thien-Thi Nguyen wrote:
> () Katsumi Yamaoka <yamaoka@jpl.org>
> () Mon, 09 Apr 2007 15:40:40 +0900

>            * message.el (message-yank-original):
>            Don't exchange point and mark.

>    I also removed the `(unless (< (point) (mark-marker)) ...)' test
>    since I think it will never return nil.

> i have no opinion on No Gnus, just on the Gnus distributed w/ Emacs
> (that i can play around with (and break, apparently)).

Thank you for leaving it to me.

>>>>> In <v9mz1hmiov.fsf@marauder.physik.uni-ulm.de>
>>>>>	Reiner Steib wrote:
> On Mon, Apr 09 2007, Katsumi Yamaoka wrote:

> I'm not sure if I understand correctly.  Are you saying we need this
> patch in addition to the current code in Emacs (i.e. Gnus v5.11;
> lisp/gnus/message.el rev. 1.117)?

The code I modified is for making sure that the yanked text ends
with a newline.  The present one indeed has the code

      (unless (bolp)
	(insert ?\n))

, which works only if the point is at the end of yanked text by
performing `message-exchange-point-and-mark'.  However, Thien-Thi
Nguyen's change made `message-exchange-point-and-mark' become not
to be used usually (so I'm going to remove it).

> If so, we should install it ASAP so that it gets more testing before
> and during the next (final?) pretest.

> Are you sure about the use of `(insert ?\n)' vs. `(newline)',
> i.e. soft or hard newline?  (I'm not sure; that's why I ask. :-))

> ,----[ <f1> f newline RET ]
>| newline is an interactive compiled Lisp function in `simple'.
>| It is bound to RET.
>| (newline &optional arg)
>|
>| Insert a newline, and move to left margin of the new line if it's blank.
>| If `use-hard-newlines' is non-nil, the newline is marked with the
>| text-property `hard'.
>| With arg, insert that many newlines.
>| Call `auto-fill-function' if the current column number is greater
>| than the value of `fill-column' and arg is nil.
> `----

Thanks for clarifying it.  Though I'm not still sure why `newline',
not `(insert ?\n)', is used in the No Gnus version.

      (if message-cite-reply-above
	  (progn
	    (message-goto-body)
	    (insert body-text)
	    (newline)
	    (message-goto-body)

I think it is worth to use `newline' if it is the last function
in a Lisp command to determine the cursor position and it is in
the middle of a paragraph.  However, `newline' used there does
not seem to apply to such a case.  So, I'll modify the following
part of my patch

      (when message-cite-reply-above
	(message-goto-body)
	(insert body-text)
	(unless (bolp)
	  (insert ?\n))
	(newline)
	(message-goto-body))

into:

      (when message-cite-reply-above
	(message-goto-body)
	(insert body-text)
	(insert (if (bolp) "\n" "\n\n"))
	(message-goto-body))

Anyway, we have plentiful time to test it because it is in only
No Gnus. :)

Regards,