Re: [PATCH v7 1/1] emacs: notmuch-tree-outline-mode

[jao <jao@gnu.org>]

Thanks for your comments, David.  Unfortunately, i won't be able to work
on this for quite a few months.  I am hoping someone else might perhaps
take the patch over and fix the issues you pinpoint below.  I'm adding
just a handful quick comments:

On Sun, Dec 11 2022, David Bremner wrote:

[...]

>> +(defcustom notmuch-tree-outline-open-on-next nil
>> +  "Open new messages under point if they are closed when moving to next one.
>> +
>> +When this flag is set, using the command
>> +`notmuch-tree-outline-next' with point on a header for a new
>> +message that is not shown will open its `notmuch-show' buffer
>> +instead of moving point to next matching message."
>> +  :type 'boolean)
>> +
>
> I'm curious about your choice of defaults here. At least the second
> seems like t might make more sense?

principle of least surprise: next-message in "normal" tree mode always
goes to the next message, regardless of whether the current one is on
display or not.  for personal use, i defined my own commands changing
that behaviour a long time ago, and, in that regard, yes, i agree
defaulting to t is more natural.  but i wasn't sure other users would
agree.

>> +(defun notmuch-tree-outline--set-visibility ()
>> +  (when (and notmuch-tree-outline-mode (> (point-max) (point-min)))
>> +    (cond ((eq notmuch-tree-outline-visibility 'hide-others)
>> +	   (notmuch-tree-outline-hide-others))
>> +	  ((eq notmuch-tree-outline-visibility 'hide-all)
>> +	   (outline-hide-body)))))
>
> I wouldn't insist, but cl-case (or even pcase) might be clearer here.

no opinion here (personally, i feel exactly the opposite, but i think
it's just a matter of taste or familiarity).

>> +(add-hook 'notmuch-tree-process-exit-functions #'notmuch-tree-outline--on-exit)
>
> Although it is relatively common in emacs code, I'm a bit skeptical of
> using hooks for things not intended to be customized by the user. Should
> we just be calling this directly somewhere?

i'm neutral on that.  with a hook, this package can almost be an
independent add-on (it would only need the :level property below).  but
it's true that doesn't matter if we merge in notmuch.

>> +(defsubst notmuch-tree-outline--level (&optional props)
>> +  (or (plist-get (or props (notmuch-tree-get-message-properties)) :level) 0))
>
> As mentioned in my previous reply, I'm still not 100% clear on why we
> need both depth and level.

i might be misremembering, but i think depth is just an auxiliarly
argument taken by that function to know whether it's inserting the tip
of a tree or not, not a real depth.  level is.  so a better way would be
to make 'depth' take the values 'level' is currently taking, but i
wasn't sure other code would be using depth with its old original
meaning (e.g. via and advice; i did at some point).

>> +(defsubst notmuch-tree-outline--message-open-p ()
>> +  (and (buffer-live-p notmuch-tree-message-buffer)
>> +       (get-buffer-window notmuch-tree-message-buffer)
>> +       (string-match-p (regexp-quote (or (notmuch-tree-get-message-id) ""))
>
> What's going with the 'or' here? Are we hiding errors?

just preventing errors if someone calls this function (via a user-level
command) in the "End of search" line or the blank line at the end of the
messages list.

>
>> +		       (buffer-name notmuch-tree-message-buffer))))
>
> At first glance, depending on the buffer name seems fragile?

not sure why, or how to make it more robust...

>> +;;;; Mode definition
>> +(defvar notmuch-tree-outline-mode-lighter nil
>> +  "The lighter mark for notmuch-tree-outline mode.
>> +Usually empty since outline-minor-mode's lighter will be active.")
>
> I feel like I should know what a "lighter" is in this context, but alas,
> I don't

the string in the mode line used to show a minor mode is active.  aka
"minor mode lighter"

> Finally:
>
> According to a quick test, the folding does show up in
> (visible-buffer-string) (from test/test-lib.el). So it seems feasible to
> have a few tests with folded output?

indeed.  hope someone will find a bit of time for that! (fwiw, i've 
used variations of this code for almost a year, so it's relatively well
field-tested; but of course that doesn't obviate proper tests)

cheers,
jao
-- 
More people would learn from their mistakes if they weren't so busy
denying them - Harlod J. Smith