Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Julien Danjou]

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

On Wed, Mar 02 2011, Ted Zlatanov wrote:

> --- a/lisp/message.el
> +++ b/lisp/message.el
> @@ -1858,7 +1858,10 @@ You must have the \"hashcash\" binary installed, see `hashcash-path'."
>  
>  (defvar	message-options nil
>    "Some saved answers when sending message.")
> -(make-variable-buffer-local 'message-options)
> +
> +(if (featurep 'xemacs)
> +    (make-local-variable 'message-options)
> +  (make-variable-buffer-local 'message-options))

I think you reverted the previous code I wrote wrong. That should be

(unless (featurep 'xemacs)
  (make-variable-buffer-local 'message-options))

Note that I'm not sure it's a good hack. At least we should add a
comment on why… And let Daiki find out if this is a real bug.

-- 
Julien Danjou
❱ http://julien.danjou.info

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Ted Zlatanov]

On Thu, 03 Mar 2011 11:44:43 +0100 Julien Danjou <julien@danjou.info> wrote: 

JD> On Wed, Mar 02 2011, Ted Zlatanov wrote:
>> --- a/lisp/message.el
>> +++ b/lisp/message.el
>> @@ -1858,7 +1858,10 @@ You must have the \"hashcash\" binary installed, see `hashcash-path'."
>> 
>> (defvar	message-options nil
>> "Some saved answers when sending message.")
>> -(make-variable-buffer-local 'message-options)
>> +
>> +(if (featurep 'xemacs)
>> +    (make-local-variable 'message-options)
>> +  (make-variable-buffer-local 'message-options))

JD> I think you reverted the previous code I wrote wrong. That should be

JD> (unless (featurep 'xemacs)
JD>   (make-variable-buffer-local 'message-options))

JD> Note that I'm not sure it's a good hack. At least we should add a
JD> comment on why… And let Daiki find out if this is a real bug.

OK, I don't know that code so it's entirely possible I did something
stupid.  I was just duplicating the behavior from before your change
(which Michael Piotrowski identified as the culprit) for XEmacs.  Let's
just keep XEmacs users functional, otherwise feel free to make any
changes you think necessary.

Ted

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Julien Danjou]

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

On Thu, Mar 03 2011, Ted Zlatanov wrote:

> OK, I don't know that code so it's entirely possible I did something
> stupid.  I was just duplicating the behavior from before your change
> (which Michael Piotrowski identified as the culprit) for XEmacs.  Let's
> just keep XEmacs users functional, otherwise feel free to make any
> changes you think necessary.

Just fixed your commit. Until someone really fixes that.

My original commit was already a fix for something else. :)

-- 
Julien Danjou
❱ http://julien.danjou.info

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Daiki Ueno]

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

Julien Danjou <julien@danjou.info> writes:

> On Thu, Mar 03 2011, Ted Zlatanov wrote:
>
>> OK, I don't know that code so it's entirely possible I did something
>> stupid.  I was just duplicating the behavior from before your change
>> (which Michael Piotrowski identified as the culprit) for XEmacs.  Let's
>> just keep XEmacs users functional, otherwise feel free to make any
>> changes you think necessary.
>
> Just fixed your commit. Until someone really fixes that.

Hm, I don't see the fix in the git.gnus.org - did you pushed it? :)

In fact, I didn't notice that the previous work around (commit by Ted)
was there and if I revert it I can finally reproduce the problem.  I
wonder why this is not the case with GNU Emacs, but anyway here is a fix:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-mml-generate-mime-Inherit-message-options-after-mm-w.patch --]
[-- Type: text/x-diff, Size: 997 bytes --]

From e3686735b34b4cfcf4752e84a24fe3ef33d8aeec Mon Sep 17 00:00:00 2001
From: Daiki Ueno <ueno@unixuser.org>
Date: Thu, 3 Mar 2011 20:50:54 +0900
Subject: [PATCH] (mml-generate-mime): Inherit message-options after mm-with-multibyte-buffer.

---
 lisp/mml.el |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/lisp/mml.el b/lisp/mml.el
index 8b196fa..2f4417f 100644
--- a/lisp/mml.el
+++ b/lisp/mml.el
@@ -461,11 +461,13 @@ If MML is non-nil, return the buffer up till the correspondent mml tag."
 (defvar mml-boundary nil)
 (defvar mml-base-boundary "-=-=")
 (defvar mml-multipart-number 0)
+(defvar message-options)
 
 (defun mml-generate-mime ()
   "Generate a MIME message based on the current MML document."
   (let ((cont (mml-parse))
-	(mml-multipart-number mml-multipart-number))
+	(mml-multipart-number mml-multipart-number)
+	(message-options (if (boundp 'message-options) message-options)))
     (if (not cont)
 	nil
       (mm-with-multibyte-buffer
-- 
1.7.2.3


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


Regards,
-- 
Daiki Ueno

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Julien Danjou]

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

On Thu, Mar 03 2011, Daiki Ueno wrote:

> Hm, I don't see the fix in the git.gnus.org - did you pushed it? :)

I guess, I forgot, sigh

> In fact, I didn't notice that the previous work around (commit by Ted)
> was there and if I revert it I can finally reproduce the problem.  I
> wonder why this is not the case with GNU Emacs, but anyway here is a fix:

I see it has been pushed. Could we revert the temporary fix from Ted
then?

-- 
Julien Danjou
❱ http://julien.danjou.info

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Daiki Ueno]

Julien Danjou <julien@danjou.info> writes:

> On Thu, Mar 03 2011, Daiki Ueno wrote:
>
>> In fact, I didn't notice that the previous work around (commit by Ted)
>> was there and if I revert it I can finally reproduce the problem.  I
>> wonder why this is not the case with GNU Emacs, but anyway here is a fix:
>
> I see it has been pushed. Could we revert the temporary fix from Ted
> then?

Reverted, but sorry it turned out that my change also does not really
fix the problem.  Anyway, I figured out the incompatibility:

(require 'message)
; => message

;; message-cross-post-old-target is another buffer-local variable

(local-variable-if-set-p 'message-cross-post-old-target (current-buffer))
; => t
message-cross-post-old-target
; => nil

(let ((message-cross-post-old-target t))
  (with-temp-buffer
    message-cross-post-old-target))
=> nil; on XEmacs / t on Emacs

It seems that let-binding loses to buffer-local variable initialization
on XEmacs and (let ((message-options message-options)) ...) does no
effect when switching between buffers.  I think it's better to use:

(unless (featurep 'xemacs)
  (make-variable-buffer-local 'message-options))

here as Julien suggested before, unless anyone knows how to address this
kind of issue.

Regards,
-- 
Daiki Ueno

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Ted Zlatanov]

On Fri, 04 Mar 2011 21:32:45 +0900 Daiki Ueno <ueno@unixuser.org> wrote: 
DU> It seems that let-binding loses to buffer-local variable initialization
DU> on XEmacs and (let ((message-options message-options)) ...) does no
DU> effect when switching between buffers.  I think it's better to use:

DU> (unless (featurep 'xemacs)
DU>   (make-variable-buffer-local 'message-options))

DU> here as Julien suggested before, unless anyone knows how to address this
DU> kind of issue.

I would ask the XEmacs developers.  But since it can't be fixed
retroactively, we're probably stuck supporting this compatibility hack
for a while no matter what they say.

Ted

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Lars Magne Ingebrigtsen]

Daiki Ueno <ueno@unixuser.org> writes:

> (let ((message-cross-post-old-target t))
>   (with-temp-buffer
>     message-cross-post-old-target))
> => nil; on XEmacs / t on Emacs
>
> It seems that let-binding loses to buffer-local variable initialization
> on XEmacs and (let ((message-options message-options)) ...) does no
> effect when switching between buffers. 

I'm not quite sure I understand the issue.  Since the variable is now
buffer-local, do we need to bind it at all?

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Julien Danjou]

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

On Sat, Mar 05 2011, Lars Magne Ingebrigtsen wrote:

> I'm not quite sure I understand the issue.  Since the variable is now
> buffer-local, do we need to bind it at all?

AFAIU, yes, if you use a temp buffer (in XEmacs).

-- 
Julien Danjou
❱ http://julien.danjou.info

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Lars Magne Ingebrigtsen]

Julien Danjou <julien@danjou.info> writes:

>> I'm not quite sure I understand the issue.  Since the variable is now
>> buffer-local, do we need to bind it at all?
>
> AFAIU, yes, if you use a temp buffer (in XEmacs).

Right.  Daiki Ueno's patch to copy over the buffer-local variables to
the temp buffer might solve that problem, though?

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Daiki Ueno]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> Julien Danjou <julien@danjou.info> writes:
>
>>> I'm not quite sure I understand the issue.  Since the variable is now
>>> buffer-local, do we need to bind it at all?
>>
>> AFAIU, yes, if you use a temp buffer (in XEmacs).
>
> Right.  Daiki Ueno's patch to copy over the buffer-local variables to
> the temp buffer might solve that problem, though?

My patch is not enough, since there are several places Gnus binds
message-options:

$ grep -n '(message-options ' *.el
gnus-art.el:4954:	      (message-options message-options)
gnus-art.el:8444:	    (let ((message-options message-options))
gnus-int.el:697:    (let ((message-options message-options))
gnus-int.el:720:    (let ((message-options message-options))
gnus-sum.el:10348:		  (message-options message-options)
message.el:1862:;; (let ((message-options message-options)) ...)
message.el:4091:	(message-options message-options))
mml.el:1469:	   (message-options message-options)

In theory, we could probably change the code like:

(let ((tmp-message-options message-options))
  (set-buffer ...)
  (setq message-options tmp-message-options))

everywhere listed above and also do this in functions called within the
let-bindings.  Too much work with little return, IMHO :)

Regards,
-- 
Daiki Ueno

Re: [gnus git] branch master updated: n0-13-65-gece58ab =2= Autoload `password-in-cache-p'. ; Try to fix XEmacs message-options buffer-local issue.

[Lars Magne Ingebrigtsen]

Daiki Ueno <ueno@unixuser.org> writes:

>> Right.  Daiki Ueno's patch to copy over the buffer-local variables to
>> the temp buffer might solve that problem, though?
>
> My patch is not enough, since there are several places Gnus binds
> message-options:
>
> $ grep -n '(message-options ' *.el
> gnus-art.el:4954:	      (message-options message-options)
> gnus-art.el:8444:	    (let ((message-options message-options))
> gnus-int.el:697:    (let ((message-options message-options))
> gnus-int.el:720:    (let ((message-options message-options))
> gnus-sum.el:10348:		  (message-options message-options)
> message.el:1862:;; (let ((message-options message-options)) ...)
> message.el:4091:	(message-options message-options))
> mml.el:1469:	   (message-options message-options)

Hm.  I wonder why Gnus binds `message-options' at all, though?  Since
it's now buffer-local, then just setting the variable (in the message
buffer) should be OK, shouldn't it?

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen