Re: [PATCH v3] Allow applying filters to summary consecutively

[Eli Zaretskii]

> From: Andrea Monaco <andrea.monaco@autistici.org>
> Cc: rms@gnu.org, rpluim@gmail.com, emacs-devel@gnu.org
> Date: Fri, 28 Oct 2022 15:26:39 +0200
> 
> This version should be final.

Famous last words ;-)

Seriously, though: having to apply 3 patches one on top of the other
is something I'd like to avoid, so in the future if you don't feel the
patch is final, please say so when you post it, to prevent me from
doing unnecessary jobs.

I have several comments to the patch, mainly on the doc strings:

> -(defcustom rmail-summary-apply-filters-consecutively nil
> -  "If non-nil, Rmail summary commands apply filtering on top existing 
> filtering.
> -When this variable is non-nil, `rmail-summary-by-*' commands work on the
> -current summary, and so their filtering can be stacked one on top of another.
> -This allows gradual narrowing of the selection of the messages."
> +(defcustom rmail-summary-intersect-consecutive-filters nil
> +  "Non-nil means that commands rmail-summary-by-* works on the
> +current summary and so can be intersected one after the other."

The first line of a doc string must be a complete sentence.  In the
original code, I made sure that was so, but your changes destroy that.

In addition, I don't think I understand what you mean by
"intersected", and neither will users who will read this doc string.
Please think of some better, clearer description, if you think the
original one was inaccurate or incorrect for some reason.

>  (defvar rmail-summary-currently-displayed-msgs nil
> -  "String made of `y' and `n'.
> -The character at position i tells wether message i is shown in the
> -summary or not.  First character is ignored.
> -Used when applying `rmail-summary-by-*' commands consecutively.  Filled
> -by `rmail-summary-fill-displayed-messages'.")
> +  "Boolean vector that for index i tells whether message i is
> +shown on the summary or not.  First element is ignored.  Used
> +when applying rmail-summary-by-* commands consecutively.  Filled
> +by rmail-summary-populate-displayed-messages.")

Same here: the first line must be a complete sentence.  Also,
references to variables and functions inside doc string should be
quoted `like this', to allow Help commands to create hyperlinks from
them.

> -(defun rmail-summary-fill-displayed-messages ()
> -  "Fill the rmail-summary-currently-displayed-msgs string."
> +(defun rmail-summary-populate-displayed-messages ()
> +  "Populate the rmail-summary-currently-displayed-msgs vector."

Quoting of variable names again.

>  (defun rmail-summary-negate ()
> -  "Toggle display of messages that match the summary and those which do not."
> +  "Negate the current summary.  That is, show the messages that
> +are not displayed, and vice versa."

The first line of a doc string must be a single complete sentence.

And I don't really understand why you changed the original doc
string.  Was it incorrect?  I think using "negate" is not a good idea,
since it's too technical and doesn't explain clearly enough what this
command does.  The original doc string attempted to clarify that in
simple terms.

> +(defun rmail-summary--exists-1 ()
> +  "Like rmail-summary-exists, but works in both main and summary buffers."

Please quote `like this' the function name mentioned in the doc
string.

Thanks.