gnus shouldn't be making general-purpose variables buffer-local

gnus shouldn't be making general-purpose variables buffer-local

[Ami Fischman]

Gnus makes 'timestamp a buffer-local variable in *Summary* buffers
(and, I suspect, any other set group parameter).  This causes buggy
interaction with non-gnus (async, usually) functions that bind
'timestamp as a function-local variable, such as emacs-jabber's
jabber-history-log-message.  For the details please see
http://thread.gmane.org/gmane.emacs.devel/105256.  Can gnus be changed
to prefix such buffer-locals with at least gnus- or, better yet,
gnus-summary-<group-name>- ?

Cheers,
-Ami

Re: gnus shouldn't be making general-purpose variables buffer-local

[Xavier Maillard]

"Ami Fischman" <ami@fischman.org> writes:

> Gnus makes 'timestamp a buffer-local variable in *Summary* buffers
> (and, I suspect, any other set group parameter).  This causes buggy
> interaction with non-gnus (async, usually) functions that bind
> 'timestamp as a function-local variable, such as emacs-jabber's
> jabber-history-log-message.  For the details please see
> http://thread.gmane.org/gmane.emacs.devel/105256.  Can gnus be changed
> to prefix such buffer-locals with at least gnus- or, better yet,
> gnus-summary-<group-name>- ?

Did you ever have a positive answer to your request ?


------------------------------------------------------------------------------
SF.Net email is Sponsored by MIX09, March 18-20, 2009 in Las Vegas, Nevada.
The future of the web can't happen without you.  Join us at MIX09 to help
pave the way to the Next Web now. Learn more and register at
http://ad.doubleclick.net/clk;208669438;13503038;i?http://2009.visitmix.com/

Re: gnus shouldn't be making general-purpose variables buffer-local

[Ami Fischman]

> > Gnus makes 'timestamp a buffer-local variable in *Summary* buffers
> > (and, I suspect, any other set group parameter).  This causes buggy
> > interaction with non-gnus (async, usually) functions that bind
> > 'timestamp as a function-local variable, such as emacs-jabber's
> > jabber-history-log-message.  For the details please see
> > http://thread.gmane.org/gmane.emacs.devel/105256.  Can gnus be changed
> > to prefix such buffer-locals with at least gnus- or, better yet,
> > gnus-summary-<group-name>- ?
>
> Did you ever have a positive answer to your request ?

Sad to say, I've gotten no replies at all.

-a

P.S. Please keep my address on the cc line of replies in future.  Thanks.

Re: gnus shouldn't be making general-purpose variables buffer-local

[Ted Zlatanov]

On Thu, 11 Dec 2008 22:42:39 -0800 "Ami Fischman" <ami@fischman.org> wrote: 

>> > Gnus makes 'timestamp a buffer-local variable in *Summary* buffers
>> > (and, I suspect, any other set group parameter).  This causes buggy
>> > interaction with non-gnus (async, usually) functions that bind
>> > 'timestamp as a function-local variable, such as emacs-jabber's
>> > jabber-history-log-message.  For the details please see
>> > http://thread.gmane.org/gmane.emacs.devel/105256.  Can gnus be changed
>> > to prefix such buffer-locals with at least gnus- or, better yet,
>> > gnus-summary-<group-name>- ?
>> 
>> Did you ever have a positive answer to your request ?

AF> Sad to say, I've gotten no replies at all.

I am not able to do this soon due to lack of time, sorry.  Perhaps
another Gnus developer is interested?  If not I'll put it on my TODO
queue but it'll be at least a few weeks.

Ted

Re: gnus shouldn't be making general-purpose variables buffer-local

[Katsumi Yamaoka]

>>>>> Ted Zlatanov wrote:
> On Thu, 11 Dec 2008 22:42:39 -0800 "Ami Fischman" <ami@fischman.org> wrote:

>>>> Gnus makes 'timestamp a buffer-local variable in *Summary* buffers
>>>> (and, I suspect, any other set group parameter).  This causes buggy
>>>> interaction with non-gnus (async, usually) functions that bind
>>>> 'timestamp as a function-local variable, such as emacs-jabber's
>>>> jabber-history-log-message.  For the details please see
>>>> http://thread.gmane.org/gmane.emacs.devel/105256.  Can gnus be changed
>>>> to prefix such buffer-locals with at least gnus- or, better yet,
>>>> gnus-summary-<group-name>- ?
>>>
>>> Did you ever have a positive answer to your request ?

AF> Sad to say, I've gotten no replies at all.

> I am not able to do this soon due to lack of time, sorry.  Perhaps
> another Gnus developer is interested?  If not I'll put it on my TODO
> queue but it'll be at least a few weeks.

Well, how can I get `timestamp' as a buffer-local variable in
the summary buffer?  So far I found neither such a variable nor
a way to make Gnus make it.

C-h v timestamp RET
 => [No match]

Regards,

Re: gnus shouldn't be making general-purpose variables buffer-local

[David Engster]

Katsumi Yamaoka <yamaoka@jpl.org> writes:
>> On Thu, 11 Dec 2008 22:42:39 -0800 "Ami Fischman" <ami@fischman.org> wrote:
>>>>> Gnus makes 'timestamp a buffer-local variable in *Summary* buffers
>>>>> (and, I suspect, any other set group parameter).  This causes buggy
>>>>> interaction with non-gnus (async, usually) functions that bind
>>>>> 'timestamp as a function-local variable, such as emacs-jabber's
>>>>> jabber-history-log-message.  For the details please see
>>>>> http://thread.gmane.org/gmane.emacs.devel/105256.  Can gnus be changed
>>>>> to prefix such buffer-locals with at least gnus- or, better yet,
>>>>> gnus-summary-<group-name>- ?

There are indeed lots of possible group parameters which turn to
buffer-local variables. I am surely not innocent here, with nnmairix
having group parameters like `query', `folder' and `threads'. The
problem I see is that changing those is incompatible with existing
groups... not sure how to deal with that...

> Well, how can I get `timestamp' as a buffer-local variable in
> the summary buffer?  

I have some nnimap groups with `timestamp' in their group parameters,
but don't know which feature put it there. You can search for
`timestamp' in your .newsrc.eld.

> So far I found neither such a variable nor
> a way to make Gnus make it.
>
> C-h v timestamp RET
>  => [No match]

You would have to bind it globally first to see that it's buffer-local.

Regards,
David

Re: gnus shouldn't be making general-purpose variables buffer-local

[Katsumi Yamaoka]

>>>>> David Engster wrote:
> Katsumi Yamaoka <yamaoka@jpl.org> writes:
>>> On Thu, 11 Dec 2008 22:42:39 -0800 "Ami Fischman" <ami@fischman.org> wrote:
>>>>>> Gnus makes 'timestamp a buffer-local variable in *Summary* buffers
>>>>>> (and, I suspect, any other set group parameter).  This causes buggy
>>>>>> interaction with non-gnus (async, usually) functions that bind
>>>>>> 'timestamp as a function-local variable, such as emacs-jabber's
>>>>>> jabber-history-log-message.  For the details please see
>>>>>> http://thread.gmane.org/gmane.emacs.devel/105256.  Can gnus be changed
>>>>>> to prefix such buffer-locals with at least gnus- or, better yet,
>>>>>> gnus-summary-<group-name>- ?

> There are indeed lots of possible group parameters which turn to
> buffer-local variables. I am surely not innocent here, with nnmairix
> having group parameters like `query', `folder' and `threads'. The
> problem I see is that changing those is incompatible with existing
> groups... not sure how to deal with that...

>> Well, how can I get `timestamp' as a buffer-local variable in
>> the summary buffer?

> I have some nnimap groups with `timestamp' in their group parameters,
> but don't know which feature put it there. You can search for
> `timestamp' in your .newsrc.eld.

Yes, there are a lot of `(timestamp 18760 46339)' things in my
.newsrc.eld file, but I believe it will never be bound as a variable.
Those forms are accessed using `gnus-group-set-timestamp' and
`gnus-group-timestamp', that don't seem to bind it as a variable.

>> So far I found neither such a variable nor
>> a way to make Gnus make it.
>>
>> C-h v timestamp RET
>>  => [No match]

> You would have to bind it globally first to see that it's buffer-local.

I see, oh!  Interesting!

Re: gnus shouldn't be making general-purpose variables buffer-local

[Katsumi Yamaoka]

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

>>>>> Katsumi Yamaoka wrote:
>>>>>> David Engster wrote:
>>> Well, how can I get `timestamp' as a buffer-local variable in
>>> the summary buffer?
[...]
>> You would have to bind it globally first to see that it's buffer-local.
[...]
> I see, oh!  Interesting!

At least for `timestamp', the attached patch will solve (note
that you need to reload the patched gnus-group.elc because of
`defsubst').  At the first time you enter to a group, the buffer-
local variable `timestamp' is still alive, but it will be renamed
to `gnus-timestamp' when exiting the group.  Maybe the change
will not slow Gnus.

For other local variables of which the names are too generic, I
tried (setq result (mapcar 'car (buffer-local-variables))) in
a nnml group, a nntp group, a nnrss group and a nnshimbun group,
and found no such one.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 825 bytes --]

--- gnus-group.el~	2008-10-03 05:47:11 +0000
+++ gnus-group.el	2008-12-17 10:18:31 +0000
@@ -4608,11 +4608,13 @@
   (when gnus-newsgroup-name
     (let ((time (current-time)))
       (setcdr (cdr time) nil)
-      (gnus-group-set-parameter gnus-newsgroup-name 'timestamp time))))
+      (gnus-group-set-parameter gnus-newsgroup-name 'gnus-timestamp time)
+      (gnus-group-remove-parameter gnus-newsgroup-name 'timestamp))))
 
 (defsubst gnus-group-timestamp (group)
   "Return the timestamp for GROUP."
-  (gnus-group-get-parameter group 'timestamp t))
+  (or (gnus-group-get-parameter group 'gnus-timestamp t)
+      (gnus-group-get-parameter group 'timestamp t)))
 
 (defun gnus-group-timestamp-delta (group)
   "Return the offset in seconds from the timestamp for GROUP to the current time, as a floating point number."

Re: gnus shouldn't be making general-purpose variables buffer-local

[Ted Zlatanov]

On Wed, 17 Dec 2008 19:21:01 +0900 Katsumi Yamaoka <yamaoka@jpl.org> wrote: 

KY> At least for `timestamp', the attached patch will solve (note
KY> that you need to reload the patched gnus-group.elc because of
KY> `defsubst').  At the first time you enter to a group, the buffer-
KY> local variable `timestamp' is still alive, but it will be renamed
KY> to `gnus-timestamp' when exiting the group.  Maybe the change
KY> will not slow Gnus.

The patch looks OK to me.  It's probably better to do it this way now,
as a bug fix that can go into Gnus and Emacs right away, and do a
thorough fix (as David Engster proposes, IIUC) later.

Thank you for looking into this.

Ted

Re: gnus shouldn't be making general-purpose variables buffer-local

[Reiner Steib]

[ Adding Ami Fischman, emacs-devel and emacs-jabber (again).
  (See
  <http://thread.gmane.org/gmane.emacs.gnus.general/67886/focus=67943>,
  <http://thread.gmane.org/gmane.emacs.devel/105256> also for the
  complete threads.) ]

On Wed, Dec 17 2008, Katsumi Yamaoka wrote:

> At least for `timestamp', the attached patch will solve (note
> that you need to reload the patched gnus-group.elc because of
> `defsubst').  At the first time you enter to a group, the buffer-
> local variable `timestamp' is still alive, but it will be renamed
> to `gnus-timestamp' when exiting the group.  Maybe the change
> will not slow Gnus.
[...]
> --- gnus-group.el~	2008-10-03 05:47:11 +0000
> +++ gnus-group.el	2008-12-17 10:18:31 +0000
> @@ -4608,11 +4608,13 @@
>    (when gnus-newsgroup-name
>      (let ((time (current-time)))
>        (setcdr (cdr time) nil)
> -      (gnus-group-set-parameter gnus-newsgroup-name 'timestamp time))))
> +      (gnus-group-set-parameter gnus-newsgroup-name 'gnus-timestamp time)
> +      (gnus-group-remove-parameter gnus-newsgroup-name 'timestamp))))
>
>  (defsubst gnus-group-timestamp (group)
>    "Return the timestamp for GROUP."
> -  (gnus-group-get-parameter group 'timestamp t))
> +  (or (gnus-group-get-parameter group 'gnus-timestamp t)
> +      (gnus-group-get-parameter group 'timestamp t)))

After the initial report on emacs-devel, I wrote this quite similar
patch:

--8<---------------cut here---------------start------------->8---
--- gnus-group.el	10 Sep 2008 03:28:52 +0200	1.77
+++ gnus-group.el	03 Nov 2008 00:17:44 +0100	
@@ -4608,11 +4608,17 @@
   (when gnus-newsgroup-name
     (let ((time (current-time)))
       (setcdr (cdr time) nil)
-      (gnus-group-set-parameter gnus-newsgroup-name 'timestamp time))))
+      ;; Remove obsolete `timestamp' (used until 2008-11-03) if present.
+      ;; Note: This breaks down-grading.
+      (when (gnus-group-get-parameter group 'timestamp t)
+	(gnus-group-remove-parameter  group 'timestamp))
+      (gnus-group-set-parameter gnus-newsgroup-name 'gnus-group-timestamp time))))
 
 (defsubst gnus-group-timestamp (group)
   "Return the timestamp for GROUP."
-  (gnus-group-get-parameter group 'timestamp t))
+  (or (gnus-group-get-parameter group 'gnus-group-timestamp t)
+      ;; For compatibility, check `timestamp' (used until 2008-11-03) as well.
+      (gnus-group-get-parameter group 'timestamp t)))
 
 (defun gnus-group-timestamp-delta (group)
   "Return the offset in seconds from the timestamp for GROUP to the current time, as a floating point number."
--8<---------------cut here---------------end--------------->8---

Beside the other name, it removes the old parameter `timestamp'.

However, I wonder if the more general patch suggested by David Engster
is better.  Does anyone see a problem with it?

Ami, does David's patch solve your problem?

--8<---------------cut here---------------start------------->8---
--- a/lisp/gnus-sum.el
+++ b/lisp/gnus-sum.el
@@ -3831,6 +3831,7 @@ This function is intended to be used in
       (and (consp elem)			; Has to be a cons.
 	   (consp (cdr elem))		; The cdr has to be a list.
 	   (symbolp (car elem))		; Has to be a symbol in there.
+	   (boundp (car elem))		; Has to be already bound
 	   (not (memq (car elem) vars))
 	   (ignore-errors		; So we set it.
 	     (push (car elem) vars)
--8<---------------cut here---------------end--------------->8---

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

Re: gnus shouldn't be making general-purpose variables buffer-local

[Ami Fischman]

Thanks for adding me back to the thread, Reiner.

David's patch does solve my problem.

In case it's helpful for others who don't want to edit their copy of
gnus-sum.el, until now I'd been making do with:
(add-hook 'gnus-select-group-hook
          (lambda ()
            (kill-local-variable 'timestamp)))
with no apparent ill-effect.

-a

On Mon, Dec 22, 2008 at 10:55 AM, Reiner Steib
<reinersteib+gmane@imap.cc> wrote:
> [ Adding Ami Fischman, emacs-devel and emacs-jabber (again).
>  (See
>  <http://thread.gmane.org/gmane.emacs.gnus.general/67886/focus=67943>,
>  <http://thread.gmane.org/gmane.emacs.devel/105256> also for the
>  complete threads.) ]
>
> On Wed, Dec 17 2008, Katsumi Yamaoka wrote:
>
>> At least for `timestamp', the attached patch will solve (note
>> that you need to reload the patched gnus-group.elc because of
>> `defsubst').  At the first time you enter to a group, the buffer-
>> local variable `timestamp' is still alive, but it will be renamed
>> to `gnus-timestamp' when exiting the group.  Maybe the change
>> will not slow Gnus.
> [...]
>> --- gnus-group.el~    2008-10-03 05:47:11 +0000
>> +++ gnus-group.el     2008-12-17 10:18:31 +0000
>> @@ -4608,11 +4608,13 @@
>>    (when gnus-newsgroup-name
>>      (let ((time (current-time)))
>>        (setcdr (cdr time) nil)
>> -      (gnus-group-set-parameter gnus-newsgroup-name 'timestamp time))))
>> +      (gnus-group-set-parameter gnus-newsgroup-name 'gnus-timestamp time)
>> +      (gnus-group-remove-parameter gnus-newsgroup-name 'timestamp))))
>>
>>  (defsubst gnus-group-timestamp (group)
>>    "Return the timestamp for GROUP."
>> -  (gnus-group-get-parameter group 'timestamp t))
>> +  (or (gnus-group-get-parameter group 'gnus-timestamp t)
>> +      (gnus-group-get-parameter group 'timestamp t)))
>
> After the initial report on emacs-devel, I wrote this quite similar
> patch:
>
> --8<---------------cut here---------------start------------->8---
> --- gnus-group.el       10 Sep 2008 03:28:52 +0200      1.77
> +++ gnus-group.el       03 Nov 2008 00:17:44 +0100
> @@ -4608,11 +4608,17 @@
>   (when gnus-newsgroup-name
>     (let ((time (current-time)))
>       (setcdr (cdr time) nil)
> -      (gnus-group-set-parameter gnus-newsgroup-name 'timestamp time))))
> +      ;; Remove obsolete `timestamp' (used until 2008-11-03) if present.
> +      ;; Note: This breaks down-grading.
> +      (when (gnus-group-get-parameter group 'timestamp t)
> +       (gnus-group-remove-parameter  group 'timestamp))
> +      (gnus-group-set-parameter gnus-newsgroup-name 'gnus-group-timestamp time))))
>
>  (defsubst gnus-group-timestamp (group)
>   "Return the timestamp for GROUP."
> -  (gnus-group-get-parameter group 'timestamp t))
> +  (or (gnus-group-get-parameter group 'gnus-group-timestamp t)
> +      ;; For compatibility, check `timestamp' (used until 2008-11-03) as well.
> +      (gnus-group-get-parameter group 'timestamp t)))
>
>  (defun gnus-group-timestamp-delta (group)
>   "Return the offset in seconds from the timestamp for GROUP to the current time, as a floating point number."
> --8<---------------cut here---------------end--------------->8---
>
> Beside the other name, it removes the old parameter `timestamp'.
>
> However, I wonder if the more general patch suggested by David Engster
> is better.  Does anyone see a problem with it?
>
> Ami, does David's patch solve your problem?
>
> --8<---------------cut here---------------start------------->8---
> --- a/lisp/gnus-sum.el
> +++ b/lisp/gnus-sum.el
> @@ -3831,6 +3831,7 @@ This function is intended to be used in
>       (and (consp elem)                        ; Has to be a cons.
>           (consp (cdr elem))           ; The cdr has to be a list.
>           (symbolp (car elem))         ; Has to be a symbol in there.
> +          (boundp (car elem))          ; Has to be already bound
>           (not (memq (car elem) vars))
>           (ignore-errors               ; So we set it.
>             (push (car elem) vars)
> --8<---------------cut here---------------end--------------->8---
>
> Bye, Reiner.
> --
>       ,,,
>      (o o)
> ---ooO-(_)-Ooo---  |  PGP key available  |  http://rsteib.home.pages.de/
>

------------------------------------------------------------------------------

Re: gnus shouldn't be making general-purpose variables buffer-local

[Katsumi Yamaoka]

>>>>> Reiner Steib wrote:
> However, I wonder if the more general patch suggested by David Engster
> is better.  Does anyone see a problem with it?

> Ami, does David's patch solve your problem?

> --- a/lisp/gnus-sum.el
> +++ b/lisp/gnus-sum.el
> @@ -3831,6 +3831,7 @@ This function is intended to be used in
>        (and (consp elem)			; Has to be a cons.
>  	   (consp (cdr elem))		; The cdr has to be a list.
>  	   (symbolp (car elem))		; Has to be a symbol in there.
> +	   (boundp (car elem))		; Has to be already bound
>  	   (not (memq (car elem) vars))
>  	   (ignore-errors		; So we set it.
>  	     (push (car elem) vars)

>>>>> In <b4mfxkms7yl.fsf@jpl.org> Katsumi Yamaoka wrote:
> Cool!  But I agree not to use it. :)

But I found no evil with that patch so far.  Variables like gnus-*
globally bound need to get to be buffer-local but it's harmless.
Moreover, those parameters have been to be set as (VAR VAL), not
(VAR . VAL).  OTOH, parameters used as just parameters, e.g.
`timestamp', should not need to be bound; those are set in the
`gnus-parameters' variable or the newsrc database.  Only one anxiety
is the case that a user or some program binds such a variable, but
the fault will lie with the user or what should be complained will
be the program.  So, I've installed David Engster's patch with a
comment: http://article.gmane.org/gmane.emacs.gnus.commits/6091

Regards,

Re: gnus shouldn't be making general-purpose variables buffer-local

[David Engster]

Katsumi Yamaoka <yamaoka@jpl.org> writes:
>>>>>> David Engster wrote:
>> Katsumi Yamaoka <yamaoka@jpl.org> writes:
>>>> On Thu, 11 Dec 2008 22:42:39 -0800 "Ami Fischman" <ami@fischman.org> wrote:
>>>>>>> Gnus makes 'timestamp a buffer-local variable in *Summary* buffers
>>>>>>> (and, I suspect, any other set group parameter).  This causes buggy
>>>>>>> interaction with non-gnus (async, usually) functions that bind
>>>>>>> 'timestamp as a function-local variable, such as emacs-jabber's
>>>>>>> jabber-history-log-message.  For the details please see
>>>>>>> http://thread.gmane.org/gmane.emacs.devel/105256.  Can gnus be changed
>>>>>>> to prefix such buffer-locals with at least gnus- or, better yet,
>>>>>>> gnus-summary-<group-name>- ?
>
>> There are indeed lots of possible group parameters which turn to
>> buffer-local variables. I am surely not innocent here, with nnmairix
>> having group parameters like `query', `folder' and `threads'. The
>> problem I see is that changing those is incompatible with existing
>> groups... not sure how to deal with that...
>
>>> Well, how can I get `timestamp' as a buffer-local variable in
>>> the summary buffer?
>
>> I have some nnimap groups with `timestamp' in their group parameters,
>> but don't know which feature put it there. You can search for
>> `timestamp' in your .newsrc.eld.
>
> Yes, there are a lot of `(timestamp 18760 46339)' things in my
> .newsrc.eld file, but I believe it will never be bound as a variable.

`gnus-summary-set-local-parameters' will set *all* group/topic
parameters to buffer-local variables. This includes `timestamp', but
also stuff like `visible', `uidvadility', `expiry-wait',
`registry-ignore', and so on.

So maybe, we should change `gnus-summary-set-local-parameters' to only
set those variables buffer-local that begin with a proper prefix, like
"gnus-" and  "mm-" ?

> Those forms are accessed using `gnus-group-set-timestamp' and
> `gnus-group-timestamp', that don't seem to bind it as a variable.

Actually, gnus-group-set-timestamp is used nowhere in the whole
trunk. :-) I guess it's mostly accessed through
gnus-group-get/set-parameter or directly from the info.

Regards,
David

Re: gnus shouldn't be making general-purpose variables buffer-local

[Katsumi Yamaoka]

>>>>> David Engster wrote:
> `gnus-summary-set-local-parameters' will set *all* group/topic
> parameters to buffer-local variables. This includes `timestamp', but
> also stuff like `visible', `uidvadility', `expiry-wait',
> `registry-ignore', and so on.

I see.

> So maybe, we should change `gnus-summary-set-local-parameters' to only
> set those variables buffer-local that begin with a proper prefix, like
> "gnus-" and  "mm-" ?

But it will disable the existing features concerning the parameters
of which the names don't begin with "gnus-" and  "mm-".  The best
way seems to be to add "gnus-" or  "mm-" to all the parameters.
However, it will take time for coding and verifying.  Maybe, it
should do after the Emacs 23.1 release.

>> Those forms are accessed using `gnus-group-set-timestamp' and
>> `gnus-group-timestamp', that don't seem to bind it as a variable.

> Actually, gnus-group-set-timestamp is used nowhere in the whole
> trunk. :-) I guess it's mostly accessed through
> gnus-group-get/set-parameter or directly from the info.

Oops, I use `gnus-group-set-timestamp' as `gnus-select-group-hook'.
I don't recall why I added it to the hook, though.

Regards,

Re: gnus shouldn't be making general-purpose variables buffer-local

[David Engster]

Katsumi Yamaoka <yamaoka@jpl.org> writes:
>>>>>> David Engster wrote:
>> So maybe, we should change `gnus-summary-set-local-parameters' to only
>> set those variables buffer-local that begin with a proper prefix, like
>> "gnus-" and  "mm-" ?
>
> But it will disable the existing features concerning the parameters
> of which the names don't begin with "gnus-" and  "mm-". 

That shouldn't happen, since those parameters are usually retrieved
through `gnus-group-get-parameter', `gnus-group-fast-parameter', or
`gnus-group-find-parameter' (see e.g. how nnimap uses the uidvalidity
parameter). If an existing feature breaks because of that change, I
would argue that it is broken, since it shouldn't depend on group
parameters being buffer-local variables.

In short: Making a group parameter buffer-local should only be done for
overriding a global value.

That gives me an idea: in `gnus-summary-set-local-parameters', instead
of defining fixed prefixes like "gnus-" or "mm-", we could simply make
only those parameters buffer-local which are globally bound?

> The best way seems to be to add "gnus-" or "mm-" to all the
> parameters.  However, it will take time for coding and verifying.

The main problem I see is that this is an incompatible change,
i.e. parameters from existing groups would have to be updated.

> Maybe, it should do after the Emacs 23.1 release.

I agree in any case.

Regards,
David

Re: gnus shouldn't be making general-purpose variables buffer-local

[David Engster]

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

David Engster <deng@randomsample.de> writes:
> In short: Making a group parameter buffer-local should only be done for
> overriding a global value.
>
> That gives me an idea: in `gnus-summary-set-local-parameters', instead
> of defining fixed prefixes like "gnus-" or "mm-", we could simply make
> only those parameters buffer-local which are globally bound?

The attached one-line patch does this. So far everything is still
working fine here. But don't count on it. :-)

-David


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gnus-sum-patch.diff --]
[-- Type: text/x-diff, Size: 498 bytes --]

diff --git a/lisp/gnus-sum.el b/lisp/gnus-sum.el
index 4f6a565..1be5949 100644
--- a/lisp/gnus-sum.el
+++ b/lisp/gnus-sum.el
@@ -3831,6 +3831,7 @@ This function is intended to be used in
       (and (consp elem)			; Has to be a cons.
 	   (consp (cdr elem))		; The cdr has to be a list.
 	   (symbolp (car elem))		; Has to be a symbol in there.
+	   (boundp (car elem))		; Has to be already bound
 	   (not (memq (car elem) vars))
 	   (ignore-errors		; So we set it.
 	     (push (car elem) vars)

Re: gnus shouldn't be making general-purpose variables buffer-local

[Katsumi Yamaoka]

>>>>> David Engster wrote:
>>> So maybe, we should change `gnus-summary-set-local-parameters' to only
>>> set those variables buffer-local that begin with a proper prefix, like
>>> "gnus-" and  "mm-" ?

> Katsumi Yamaoka <yamaoka@jpl.org> writes:
>> But it will disable the existing features concerning the parameters
>> of which the names don't begin with "gnus-" and  "mm-".

>>>>> David Engster wrote:
> That shouldn't happen, since those parameters are usually retrieved
> through `gnus-group-get-parameter', `gnus-group-fast-parameter', or
> `gnus-group-find-parameter' (see e.g. how nnimap uses the uidvalidity
> parameter). If an existing feature breaks because of that change, I
> would argue that it is broken, since it shouldn't depend on group
> parameters being buffer-local variables.

I agree.  I recalled that

(setq gnus-parameters '((".*" (parameter . value)))) and
(setq gnus-parameters '((".*" (parameter value))))

are different.  The former is for only setting the parameter and
the value, does not bind the parameter as a variable, but the later
does.  Ok.  I'm going to try some tests for some time.

>> In short: Making a group parameter buffer-local should only be done for
>> overriding a global value.
>>
>> That gives me an idea: in `gnus-summary-set-local-parameters', instead
>> of defining fixed prefixes like "gnus-" or "mm-", we could simply make
>> only those parameters buffer-local which are globally bound?

> The attached one-line patch does this. So far everything is still
> working fine here. But don't count on it. :-)

Cool!  But I agree not to use it. :)

Regards,

Re: gnus shouldn't be making general-purpose variables buffer-local

[David Engster]

Katsumi Yamaoka <yamaoka@jpl.org> writes:
> I recalled that
>
> (setq gnus-parameters '((".*" (parameter . value)))) and
> (setq gnus-parameters '((".*" (parameter value))))
>
> are different.  The former is for only setting the parameter and
> the value, does not bind the parameter as a variable, but the later
> does.

Ah! Yes, I overlooked that. If you set a parameter with a single value
through gnus-group-set-parameter, it will not become a buffer local
variable, since it is set as a single cons cell (as in the first line
you've given).

The problem exists only for group parameters which have lists as values,
like `timestamp'. Unfortunately, nnmairix has also two of those: `query'
and `numcorr'... :-/

>> The attached one-line patch does this. So far everything is still
>> working fine here. But don't count on it. :-)

>Cool!  But I agree not to use it. :)

OK. But it's still working fine so far. :-)

-David