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 _______________________________________________ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
