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

[Robert Pluim]

Hi Andrea, some comments below

>>>>> On Wed, 19 Oct 2022 16:23:13 +0200, Andrea Monaco 
>>>>> <andrea.monaco@autistici.org> said:

    Andrea> +(defcustom rmail-summary-apply-filters-consecutively nil
    Andrea> +  "Non-nil means that commands rmail-summary-by-* works
    Andrea> on the

Iʼd call it `rmail-summary-stack-filters' and say

"Whether rmail-summary-by-* commands stack on each other."

    Andrea> +current summary and so can be stacked one after the other."
    Andrea> +  :type 'boolean
    Andrea> +  :group 'rmail-summary)

This needs a :version "29.1"

    Andrea> +(defvar rmail-summary-currently-displayed-msgs nil
    Andrea> +  "String made of 'y' and 'n'.  At index i it tells
    Andrea> wether

"whether"

    Andrea> +message i is shown on the summary or not.  First character is
    Andrea> +ignored.  Used when applying rmail-summary-by-* commands
    Andrea> +consecutively.  Filled by
    Andrea> +rmail-summary-fill-displayed-messages.")
    Andrea> +(put 'rmail-summary-currently-displayed-msgs 'permanent-local t)
    Andrea> +

Itʼs a string that you treat as a vector, so why not just make it a
vector? Also 'fill' in Emacs normally means wrapping paragraphs of
text, so perhaps `rmail-summary-populate-displayed-messages' or
similar is better.

    Andrea>  (defvar rmail-summary-font-lock-keywords
    Andrea>    '(("^ *[0-9]+D.*" . font-lock-string-face)                       
; Deleted.
    Andrea>      ("^ *[0-9]+-.*" . font-lock-type-face)                 ; 
Unread.
    Andrea> @@ -267,6 +281,35 @@ rmail-summary-mode-map
    Andrea>  (defun rmail-update-summary (&rest _)
    Andrea>    (apply (car rmail-summary-redo) (cdr rmail-summary-redo)))
 
    Andrea> +(defun rmail-summary-fill-displayed-messages ()
    Andrea> +  "Fill the rmail-summary-currently-displayed-msgs string."
    Andrea> +  (with-current-buffer rmail-buffer
    Andrea> +    (with-current-buffer rmail-summary-buffer
    Andrea> +      (setq rmail-summary-currently-displayed-msgs
    Andrea> +       (make-string (1+ rmail-total-messages) ?n))
    Andrea> +      (goto-char (point-min))
    Andrea> +      (while (not (eobp))
    Andrea> +   (aset rmail-summary-currently-displayed-msgs
    Andrea> +         (string-to-number (thing-at-point 'line))
    Andrea> +         ?y)
    Andrea> +   (forward-line 1)))))

I donʼt remember the details of rmailʼs summary line format, but I
think youʼd be better served with

(thing-at-point 'number)

(that also avoids the `string-to-number')

    Andrea> +
    Andrea> +(defun rmail-summary-negate ()
    Andrea> +  "Negate the current summary.  That is, show the messages that
    Andrea> +are not displayed, and vice versa."

The first line of the docstring should be a single complete sentence
on a single line.

    Andrea> +  (interactive)
    Andrea> +  (rmail-summary-fill-displayed-messages)
    Andrea> +  (rmail-new-summary "Negate"
    Andrea> +                '(rmail-summary-by-regexp ".*")
    Andrea> +                (lambda (msg)
    Andrea> +                  (if
    Andrea> +                      (= (aref 
rmail-summary-currently-displayed-msgs msg)
    Andrea> +                         ?n)
    Andrea> +                      (progn
    Andrea> +                        (aset 
rmail-summary-currently-displayed-msgs msg ?y) t)
    Andrea> +                    (progn
    Andrea> +                      (aset rmail-summary-currently-displayed-msgs 
msg ?n) nil)))))
    Andrea> +

If you switch `rmail-summary-currently-displayed-msgs' to a vector,
you can store `nil' and `t' there directly and avoid the prognʼs (aset
returns the new value)

    Andrea>  ;;;###autoload
    Andrea>  (defun rmail-summary ()
    Andrea>    "Display a summary of all messages, one line per message."
    Andrea> @@ -282,9 +325,16 @@ rmail-summary-by-labels
    Andrea>        (setq labels (or rmail-last-multi-labels
    Andrea>                    (error "No label specified"))))
    Andrea>    (setq rmail-last-multi-labels labels)
    Andrea> +  (if rmail-summary-apply-filters-consecutively
    Andrea> +      (rmail-summary-fill-displayed-messages))
    Andrea>    (rmail-new-summary (concat "labels " labels)
    Andrea>                  (list 'rmail-summary-by-labels labels)
    Andrea> -                'rmail-message-labels-p
    Andrea> +                (if rmail-summary-apply-filters-consecutively
    Andrea> +                    (lambda (msg l)
    Andrea> +                      (and (= (aref 
rmail-summary-currently-displayed-msgs msg)
    Andrea> +                              ?y)
    Andrea> +                           (rmail-message-labels-p msg l)))
    Andrea> +                  'rmail-message-labels-p)
    Andrea>                  (concat " \\("
    Andrea>                          (mail-comma-list-regexp labels)
    Andrea>                          "\\)\\(,\\|\\'\\)")))

I think you have this same lambda about 4 times in the code, perhaps
`defun' it (using "rmail-summary--" as a prefix to indicate that itʼs
intended for internal usage)

Robert
--