Re: [PATCH] Summary by thread in rmail

[Eli Zaretskii <eliz@gnu.org>]

> From: Andrea Monaco <andrea.monaco@autistici.org>
> Date: Wed, 05 Oct 2022 23:57:49 +0200
> 
> I had some code ready.  This only looks at the Subject field.  You can
> invoke it with rmail-thread-summary and it creates a buffer called
> e.g. RMAIL-thread-summary that is independent of RMAIL-summary.  There's
> still no code to update the summary after getting new mail.  It's basic,
> but it works.

Thanks.  Please see some comments below.

Richard, I'd appreciate your review as well.

> --- /dev/null
> +++ b/lisp/mail/rmailthread.el

Please add this to rmailsum.el, instead of making a new file.

> +(defun rmail-get-create-thread-summary-buffer ()
> +  "Return the Rmail thread summary buffer.
> +If necessary, it is created and undo is disabled."
> +  (if (and rmail-thread-summary-buffer (buffer-name rmail-thread-summary-buffer))
                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

We nowadays have the buffer-live-p function to make such tests.

> +    (let ((buff (generate-new-buffer (concat (buffer-name) "-thread-summary"))))

I think "-summary-by-thread" is a better name.  "Thread-summary" hints
that it is a summary of a single thread, which is not the case here.

> +(defun rmail-new-thread-summary (desc redo function &rest args)
> +  "Create a summary of selected messages by thread."

The "selected" part here is misleading, I think.  The truth is, the
messages can be "selected" by the FUNCTION argument, but what kind of
FUNCTION can make sense for this summary?  And in any case, if we keep
the FUNCTION arg, the doc string should explain how messages are
"selected" for the summary.

> +    (with-current-buffer rmail-buffer
> +      (setq rmail-thread-summary-buffer (rmail-new-thread-summary-1 desc redo function args)
> +	    ;; r-s-b is buffer-local.

What is "r-s-b" here?  Please don't use abbreviated symbol names, they
make reading the code harder.

> +      ;; This is how rmail makes the summary buffer reappear.
> +      ;; We do this here to make the window the proper size.
> +      ;(rmail-select-summary nil)
> +					;(set-buffer sumbuf))

Is this commented-out code necessary?

> +    ;(rmail-summary-goto-msg mesg t t)
> +    ;(rmail-summary-construct-io-menu)

And this?

> +			  (if thread
> +			      (setcdr thread
> +				      (cons (cons msgnum (rmail-get-summary msgnum)) (cdr thread)))
> +			    (progn
> +			      (let* ((newthread (list subject (cons msgnum (rmail-get-summary msgnum))))
> +				     (newcell (cons newthread cell)))
> +				(setq cell newcell)
> +				(puthash subject newcell rmail-thread-hash-table)
> +				(setq ordered-threads (cons newthread ordered-threads)))))

Is progn really necessary here?

> +				(setq ordered-threads (cons newthread ordered-threads)))))

Maybe using 'push' here will make the code more concise and readable?

> +			  (setq msgnum (1+ msgnum))
> +			  (if (and (not (zerop msgnum))
> +				   (zerop (% msgnum 10)))
> +			      (message "Computing thread summary lines... %d"
> +				       msgnum))))))

"Computing by-thread summary lines... %d" is a better message, I
think.

> +	      (while thread-message
> +		(let* ((suml (cdar thread-message))
> +		       (newsuml (concat (substring suml 0 42) "  " (substring suml 42))))

Why are you truncating the subject at the 42nd character? and why is
42 hard-coded?

This:

> +	(setq-local minor-mode-alist (list (list t (concat ": " description))))

together with this:

> +  (rmail-new-thread-summary "All" '(rmail-thread-summary) nil))

Will AFAIU display "RMAIL Summary: All" in the mode line.  However, I
think the mode line should in this case show something like this
instead:

  RMAIL Summary: By-Thread

Thanks again for working on this.