GNU bug report logs - #33263
27.0.50; Tidying up Gnus modes

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 33263 in the body.
You can then email your comments to 33263 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; Tidying up Gnus modes
Date: Sun, 04 Nov 2018 18:53:41 -0800

[Message part 1 (text/plain, inline)]

The attached patch does a couple of things to clean up Gnus's various
major modes:

- Creates a do-nothing `gnus-mode', derived from `special-mode', to
  serve as parent mode for all the other major modes. This isn't so
  important, but sometimes it would be nice to do (derived-mode-p
  'gnus-mode).
- Derives everything from `gnus-mode', and turns a couple of modes from
  functions that manually set up modes into proper calls to
  `define-derived-mode'.
- Removes code like "(setq buffer-read-only t)", as that's automatically
  done by `special-mode'.
- Where appropriate, deletes custom definitions for gnus-*-mode-hook, as
  these hooks are already automatically created by the define-mode
  calls. (Maybe this removal isn't worth doing?)

This patch probably needs more testing, but I'm floating it first for
any comments.

Eric

In GNU Emacs 27.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.24.1)
 of 2018-10-31 built on slip

[0001-Provide-new-gnus-mode-derive-all-gnus-major-modes-fr.patch (text/x-patch, attachment)]

Message #8 received at 33263 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 33263 <at> debbugs.gnu.org
Subject: Re: bug#33263: 27.0.50; Tidying up Gnus modes
Date: Mon, 05 Nov 2018 09:14:25 -0500

Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:

> +(defvar gnus-browse-mode-map (make-keymap))
>  
>  (unless gnus-browse-mode-map
> -  (setq gnus-browse-mode-map (make-keymap))
> +  (defvar gnus-browse-mode-map (make-keymap))

This doesn't look right.

> -(defun gnus-summary-mode (&optional group)
> +(define-derived-mode gnus-summary-mode gnus-mode "Summary"
>    "Major mode for reading articles.

> -      (gnus-summary-mode group)
> +      (setq gnus-newsgroup-name group)
> +      (gnus-summary-mode)
>        (when (gnus-group-quit-config group)

Where does GROUP come from now?

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#33263: 27.0.50; Tidying up Gnus modes
Date: Mon, 05 Nov 2018 09:51:22 -0800

Noam Postavsky <npostavs <at> gmail.com> writes:

> Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:
>
>> +(defvar gnus-browse-mode-map (make-keymap))
>>
>>  (unless gnus-browse-mode-map
>> -  (setq gnus-browse-mode-map (make-keymap))
>> +  (defvar gnus-browse-mode-map (make-keymap))
>
> This doesn't look right.

Hmm, you're right, I think I got confused there. TBH I don't really know
why these things are wrapped in `unless', but I think what should have
happened is that `gnus-browse-mode-map' is first defvar'ed to nil, and
later setq'ed to (make-keymap).

>> -(defun gnus-summary-mode (&optional group)
>> +(define-derived-mode gnus-summary-mode gnus-mode "Summary"
>>    "Major mode for reading articles.
>
>> -      (gnus-summary-mode group)
>> +      (setq gnus-newsgroup-name group)
>> +      (gnus-summary-mode)
>>        (when (gnus-group-quit-config group)
>
> Where does GROUP come from now?

GROUP has always been passed down through gnus-group-read-group (which
picks it up from the group under point) => gnus-summary-read-group =>
gnus-summary-read-group-1 => gnus-summary-setup-buffer =>
gnus-summary-mode.

When gnus-summary-mode was a function it was passed GROUP as an argument
and set it as the (buffer-local) gnus-newsgroup-name variable. I
couldn't see anything in the rest of gnus-summary-mode that would need
that variable set, but you never know if Gnus is checking something four
levels deep. So I pulled that up and set it in
gnus-summary-setup-buffer, before the call to the major mode, forgetting
that calling gnus-summary-mode would of course clear out buffer-local
variables. *And* failing to notice that gnus-summary-setup-buffer sets
the variable *again* later on. So it was originally set twice.

I've been testing for a while, and don't think anything called from the
gnus-summary-mode code needs to be aware of gnus-newsgroup-name, but I
will look a little closer. Anything that *does* rely on knowing which
group we're opening should definitely be hoisted up to
gnus-summary-setup-buffer. Then I'll just delete the first setting of
gnus-newsgroup-name and leave the later one.

Thanks for looking at this.

Message #14 received at 33263 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 33263 <at> debbugs.gnu.org
Subject: Re: bug#33263: 27.0.50; Tidying up Gnus modes
Date: Tue, 06 Nov 2018 08:27:56 -0500

Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:

> Noam Postavsky <npostavs <at> gmail.com> writes:
>
>> Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:
>>
>>> +(defvar gnus-browse-mode-map (make-keymap))
>>>
>>>  (unless gnus-browse-mode-map
>>> -  (setq gnus-browse-mode-map (make-keymap))
>>> +  (defvar gnus-browse-mode-map (make-keymap))
>>
>> This doesn't look right.
>
> Hmm, you're right, I think I got confused there. TBH I don't really know
> why these things are wrapped in `unless', but I think what should have
> happened is that `gnus-browse-mode-map' is first defvar'ed to nil, and
> later setq'ed to (make-keymap).

The usual idiom for this is

    (defvar gnus-browse-mode-map
      (let ((map (make-sparse-keymap)))
        (define-key map ...)
        ...
        map))

It seems that (the expansion of) gnus-define-keys relies on dynamic
binding of the map variable (i.e., it must be called after the defvar,
not inside), so that's the reason to split in two parts.

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#33263: 27.0.50; Tidying up Gnus modes
Date: Tue, 06 Nov 2018 08:21:31 -0800

Noam Postavsky <npostavs <at> gmail.com> writes:

> Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:
>
>> Noam Postavsky <npostavs <at> gmail.com> writes:
>>
>>> Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:
>>>
>>>> +(defvar gnus-browse-mode-map (make-keymap))
>>>>
>>>>  (unless gnus-browse-mode-map
>>>> -  (setq gnus-browse-mode-map (make-keymap))
>>>> +  (defvar gnus-browse-mode-map (make-keymap))
>>>
>>> This doesn't look right.
>>
>> Hmm, you're right, I think I got confused there. TBH I don't really know
>> why these things are wrapped in `unless', but I think what should have
>> happened is that `gnus-browse-mode-map' is first defvar'ed to nil, and
>> later setq'ed to (make-keymap).
>
> The usual idiom for this is
>
>     (defvar gnus-browse-mode-map
>       (let ((map (make-sparse-keymap)))
>         (define-key map ...)
>         ...
>         map))
>
> It seems that (the expansion of) gnus-define-keys relies on dynamic
> binding of the map variable (i.e., it must be called after the defvar,
> not inside), so that's the reason to split in two parts.

Right, I got that much, I was mostly wondering why the `unless' -- it's
unlikely these files would be loaded multiple times, and even if they
were, that wouldn't break anything, would it?

Message #20 received at 33263 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 33263 <at> debbugs.gnu.org
Subject: Re: bug#33263: 27.0.50; Tidying up Gnus modes
Date: Tue, 6 Nov 2018 11:28:17 -0500

On Tue, 6 Nov 2018 at 11:22, Eric Abrahamsen <eric <at> ericabrahamsen.net> wrote:

> >>>> +(defvar gnus-browse-mode-map (make-keymap))
> >>>>
> >>>>  (unless gnus-browse-mode-map
> >>>> -  (setq gnus-browse-mode-map (make-keymap))
> >>>> +  (defvar gnus-browse-mode-map (make-keymap))

> Right, I got that much, I was mostly wondering why the `unless' -- it's
> unlikely these files would be loaded multiple times, and even if they
> were, that wouldn't break anything, would it?

Without the `unless', if the user had made changes to
gnus-browse-mode-map they would be lost upon reload (as you say, it's
not especially likely to happen, but I guess the original author
wanted to preserve the semantics of the single defvar form).

Message #23 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#33263: 27.0.50; Tidying up Gnus modes
Date: Tue, 06 Nov 2018 09:37:59 -0800

Noam Postavsky <npostavs <at> gmail.com> writes:

> On Tue, 6 Nov 2018 at 11:22, Eric Abrahamsen <eric <at> ericabrahamsen.net> wrote:
>
>> >>>> +(defvar gnus-browse-mode-map (make-keymap))
>> >>>>
>> >>>>  (unless gnus-browse-mode-map
>> >>>> -  (setq gnus-browse-mode-map (make-keymap))
>> >>>> +  (defvar gnus-browse-mode-map (make-keymap))
>
>> Right, I got that much, I was mostly wondering why the `unless' -- it's
>> unlikely these files would be loaded multiple times, and even if they
>> were, that wouldn't break anything, would it?
>
> Without the `unless', if the user had made changes to
> gnus-browse-mode-map they would be lost upon reload (as you say, it's
> not especially likely to happen, but I guess the original author
> wanted to preserve the semantics of the single defvar form).

Got it, thanks for the explanation. I'll fix this, and look more closely
at the "group" argument to `gnus-summary-mode'.

Eric

Message #26 received at 33263 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: 33263 <at> debbugs.gnu.org
Cc: Eric Abrahamsen <eric <at> ericabrahamsen.net>,
 Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#33263: 27.0.50;
 Tidying up Gnus modes, [PATCH] Provide new gnus-mode, derive all gnus
 major modes from this
Date: Fri, 09 Nov 2018 23:01:46 -0500

[Message part 1 (text/plain, inline)]

[forwarding to list]

[Message part 2 (message/rfc822, inline)]

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Noam Postavsky <npostavs <at> gmail.com>
Subject: Re: bug#33263: 27.0.50; Tidying up Gnus modes
Date: Fri, 09 Nov 2018 12:54:53 -0800

[Message part 3 (text/plain, inline)]

Here's a new version of the patch. I fixed the keymap thing, and looked
carefully at the code called from `gnus-summary-mode'.

`turn-on-mailing-list-mode' clearly belongs in
`gnus-summary-setup-buffer', so I moved it up there. The format spec and
mark position stuff doesn't vary depending on the group, but I have
dreams of a day when it will, so I moved that up, too.

I added more docs and comments making the separation of concerns clear:
`gnus-summary-setup-buffer' handles group-dependent setup,
`gnus-summary-mode' handles group-independent stuff. That's also the
motivation for moving the three `make-local-variables', which would
otherwise be a pointless change.

I hope this is acceptable (I'm cc'ing Lars in case he has an opinion).
I'll run it locally for a week or so, then push to master.

Eric

[0001-Provide-new-gnus-mode-derive-all-gnus-major-modes-fr.patch (text/x-patch, attachment)]

Message #29 received at 33263 <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 33263 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#33263: 27.0.50;
 Tidying up Gnus modes, [PATCH] Provide new gnus-mode, derive all gnus
 major modes from this
Date: Fri, 09 Nov 2018 20:55:47 -0800

Noam Postavsky <npostavs <at> gmail.com> writes:

> [forwarding to list]

[...]

Bah, sorry about that.

And it turns out that the mark position stuff does depend on the group!
In mysterious ways that aren't clear to me yet. So I'll continue poking
at this patch until I've got a better sense of how stuff interacts.

Eric

Message #32 received at 33263 <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Noam Postavsky <npostavs <at> gmail.com>
Cc: 33263 <at> debbugs.gnu.org, Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: Re: bug#33263: 27.0.50;
 Tidying up Gnus modes, [PATCH] Provide new gnus-mode, derive all gnus
 major modes from this
Date: Sat, 10 Nov 2018 12:18:35 -0800

[Message part 1 (text/plain, inline)]

Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:

> Noam Postavsky <npostavs <at> gmail.com> writes:
>
>> [forwarding to list]
>
> [...]
>
> Bah, sorry about that.
>
> And it turns out that the mark position stuff does depend on the group!
> In mysterious ways that aren't clear to me yet. So I'll continue poking
> at this patch until I've got a better sense of how stuff interacts.

Aaaand... It wasn't that mysterious after all, I'd transposed two
function calls. Here's the patch that I'm dog-fooding, and I will be
quiet for a while.

Eric

[0001-Provide-new-gnus-mode-derive-all-gnus-major-modes-fr.patch (text/x-patch, attachment)]

Message #37 received at 33263-done <at> debbugs.gnu.org (full text, mbox):

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: 33263-done <at> debbugs.gnu.org
Subject: Re: bug#33263: 27.0.50; Tidying up Gnus modes
Date: Fri, 23 Nov 2018 09:45:24 -0800

Okay, I've been running this for more than a week and it seems fine so
far. I'll push it, and watch for repercussions.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.