jao <jao@gnu.org> writes:

> +(defcustom notmuch-tree-outline-visibility 'hide-others
> +  "Default state of the forest outline for `notmuch-tree-outline-mode'.
> +
> +This variable controls the state of a forest initially and after
> +a movement command.  If set to nil, all trees are displayed while
> +the symbol hide-all indicates that all trees in the forest should
> +be folded and hide-other that only the first one should be
Should be hide-others right?
> +unfolded."
> +  :type '(choice (const :tag "Show all" nil)
> +		 (const :tag "Hide others" hide-others)
> +		 (const :tag "Hide all" hide-all)))
> +
> +(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?


> +(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.

> +(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?

> +(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.

> +
> +(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?

> +		       (buffer-name notmuch-tree-message-buffer))))

At first glance, depending on the buffer name seems fragile?

> +;;;; 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

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?

d

_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org