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

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

[jao]

This is essentiall the same as the previously sent v 6, rebased over
the current master branch.  Since that previous version has not
elicited special enthusiasm, yet i use it all the time, so if you guys
think it's not worth integrating into notmuch-emacs upstream, please
just let me know and i'll create a separate ELPA package for it.

Thanks!
jao

jao (1):
  emacs: notmuch-tree-outline-mode

 doc/notmuch-emacs.rst |  23 ++++++
 emacs/notmuch-tree.el | 180 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 203 insertions(+)

--
2.38.1

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

[jao]

With this mode, one can fold trees in the notmuch-tree buffer as if
they were outlines, using all the commands provided by
outline-minor-mode.  We also define a couple of movement commands
that, optional, will ensure that only the thread around point is
unfolded.

The implementation is based on registering a :level property in the
messages p-list, that is then used by outline-minor-mode to to
recognise headers.

---
Leftover in verson 5 removed, sorry!

This version supersedes v4 (id:20220923203449.3747562-1-jao@gnu.org)
by dispensing with the use of invisible text.  Now, if desired,
notmuch-tree-outline-mode could live in its own
notmuch-tree-outline-mode.el without circular deps, but i don't see
a pressing need given that notmuch-tree.el is nicely outlined.

Signed-off-by: jao <jao@gnu.org>
---
 doc/notmuch-emacs.rst |  23 ++++++
 emacs/notmuch-tree.el | 180 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 203 insertions(+)

diff --git a/doc/notmuch-emacs.rst b/doc/notmuch-emacs.rst
index 846f5e67..b5f88a98 100644
--- a/doc/notmuch-emacs.rst
+++ b/doc/notmuch-emacs.rst
@@ -606,6 +606,29 @@ can be controlled by the variable ``notmuch-search-oldest-first``.
    See also :el:defcustom:`notmuch-search-result-format` and
    :el:defcustom:`notmuch-unthreaded-result-format`.
 
+It is also possible to enable outlines in notmuch tree buffers, via
+``notmuch-tree-outline-mode``.
+
+|docstring::notmuch-tree-outline-mode|
+
+The behaviour of this minor mode is affected by the following
+customizable variables:
+
+.. el:defcustom:: notmuch-tree-outline-enabled
+
+   |docstring::notmuch-tree-outline-enabled|
+
+.. el:defcustom:: notmuch-tree-outline-visibility
+
+   |docstring::notmuch-tree-outline-visibility|
+
+.. el:defcustom:: notmuch-tree-outline-auto-close
+
+   |docstring::notmuch-tree-outline-auto-close|
+
+.. el:defcustom:: notmuch-tree-outline-open-on-next
+
+   |docstring::notmuch-tree-outline-open-on-next|
 
 .. _notmuch-unthreaded:
 
diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
index b3c2c992..47bd17bc 100644
--- a/emacs/notmuch-tree.el
+++ b/emacs/notmuch-tree.el
@@ -1034,6 +1034,8 @@ message together with all its descendents."
     (setq msg (plist-put msg :first (and first (eq 0 depth))))
     (setq msg (plist-put msg :tree-status tree-status))
     (setq msg (plist-put msg :orig-tags (plist-get msg :tags)))
+    (setq msg (plist-put msg
+			 :level (1+ (if (and (eq 0 depth) (not first)) 1 depth))))
     (notmuch-tree-goto-and-insert-msg msg)
     (pop tree-status)
     (pop tree-status)
@@ -1278,6 +1280,184 @@ search results and that are also tagged with the given TAG."
 		  nil
 		  notmuch-search-oldest-first)))
 
+;;; Tree outline mode
+;;;; Custom variables
+(defcustom notmuch-tree-outline-enabled nil
+  "Whether to automatically activate `notmuch-tree-outline-mode' in tree views."
+  :type 'boolean)
+
+(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
+unfolded."
+  :type '(choice (const :tag "Show all" nil)
+		 (const :tag "Hide others" hide-others)
+		 (const :tag "Hide all" hide-all)))
+
+(defcustom notmuch-tree-outline-auto-close nil
+  "Close message and tree windows when moving past the last message."
+  :type 'boolean)
+
+(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)
+
+;;;; Helper functions
+(defsubst notmuch-tree-outline--pop-at-end (pop-at-end)
+  (if notmuch-tree-outline-auto-close (not pop-at-end) pop-at-end))
+
+(defun notmuch-tree-outline--enable-mode ()
+  (when notmuch-tree-outline-enabled (notmuch-tree-outline-mode 1)))
+
+(add-hook 'notmuch-tree-mode-hook #'notmuch-tree-outline--enable-mode)
+
+(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)))))
+
+(defun notmuch-tree-outline--on-exit (proc)
+  (when (eq (process-status proc) 'exit)
+    (notmuch-tree-outline--set-visibility)))
+
+(add-hook 'notmuch-tree-process-exit-functions #'notmuch-tree-outline--on-exit)
+
+(defsubst notmuch-tree-outline--level (&optional props)
+  (or (plist-get (or props (notmuch-tree-get-message-properties)) :level) 0))
+
+(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) ""))
+		       (buffer-name notmuch-tree-message-buffer))))
+
+(defsubst notmuch-tree-outline--at-original-match-p ()
+  (and (notmuch-tree-get-prop :match)
+       (equal (notmuch-tree-get-prop :orig-tags)
+              (notmuch-tree-get-prop :tags))))
+
+(defun notmuch-tree-outline--next (prev thread pop-at-end &optional open-new)
+  (cond (thread
+	 (notmuch-tree-thread-top)
+	 (if prev
+	     (outline-backward-same-level 1)
+	   (outline-forward-same-level 1))
+	 (when (> (notmuch-tree-outline--level) 0) (outline-show-branches))
+	 (notmuch-tree-outline--next nil nil pop-at-end t))
+	((and (or open-new notmuch-tree-outline-open-on-next)
+	      (notmuch-tree-outline--at-original-match-p)
+	      (not (notmuch-tree-outline--message-open-p)))
+	 (notmuch-tree-outline-hide-others t))
+	(t (outline-next-visible-heading (if prev -1 1))
+	   (unless (notmuch-tree-get-prop :match)
+	     (notmuch-tree-matching-message prev pop-at-end))
+	   (notmuch-tree-outline-hide-others t))))
+
+;;;; User commands
+(defun notmuch-tree-outline-hide-others (&optional and-show)
+  "Fold all threads except the one around point.
+If AND-SHOW is t, make the current message visible if it's not."
+  (interactive)
+  (save-excursion
+    (while (and (not (bobp)) (> (notmuch-tree-outline--level) 1))
+      (outline-previous-heading))
+    (outline-hide-sublevels 1))
+  (when (> (notmuch-tree-outline--level) 0)
+    (outline-show-subtree)
+    (when and-show (notmuch-tree-show-message nil))))
+
+(defun notmuch-tree-outline-next (&optional pop-at-end)
+  "Next matching message in a forest, taking care of thread visibility.
+A prefix argument reverses the meaning of `notmuch-tree-outline-auto-close'."
+  (interactive "P")
+  (let ((pop (notmuch-tree-outline--pop-at-end pop-at-end)))
+    (if (null notmuch-tree-outline-visibility)
+	(notmuch-tree-matching-message nil pop)
+      (notmuch-tree-outline--next nil nil pop))))
+
+(defun notmuch-tree-outline-previous (&optional pop-at-end)
+  "Previous matching message in forest, taking care of thread visibility.
+With prefix, quit the tree view if there is no previous message."
+  (interactive "P")
+  (if (null notmuch-tree-outline-visibility)
+      (notmuch-tree-prev-matching-message pop-at-end)
+    (notmuch-tree-outline--next t nil pop-at-end)))
+
+(defun notmuch-tree-outline-next-thread ()
+  "Next matching thread in forest, taking care of thread visibility."
+  (interactive)
+  (if (null notmuch-tree-outline-visibility)
+      (notmuch-tree-next-thread)
+    (notmuch-tree-outline--next nil t nil)))
+
+(defun notmuch-tree-outline-previous-thread ()
+  "Previous matching thread in forest, taking care of thread visibility."
+  (interactive)
+  (if (null notmuch-tree-outline-visibility)
+      (notmuch-tree-prev-thread)
+    (notmuch-tree-outline--next t t nil)))
+
+;;;; 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.")
+
+(define-minor-mode notmuch-tree-outline-mode
+  "Minor mode allowing message trees to be folded as outlines.
+
+When this mode is set, each thread and subthread in the results
+list is treated as a foldable section, with its first message as
+its header.
+
+The mode just makes available in the tree buffer all the
+keybindings in `outline-minor-mode', and binds the following
+additional keys:
+
+\\{notmuch-tree-outline-mode-map}
+
+The customizable variable `notmuch-tree-outline-visibility'
+controls how navigation in the buffer is affected by this mode:
+
+  - If it is set to nil, `notmuch-tree-outline-previous',
+    `notmuch-tree-outline-next', and their thread counterparts
+    behave just as the corresponding notmuch-tree navigation keys
+    when this mode is not enabled.
+
+  - If, on the other hand, `notmuch-tree-outline-visibility' is
+    set to a non-nil value, these commands hiding the outlines of
+    the trees you are not reading as you move to new messages.
+
+To enable notmuch-tree-outline-mode by default in all
+notmuch-tree buffers, just set
+`notmuch-tree-outline-mode-enabled' to t."
+  :lighter notmuch-tree-outline-mode-lighter
+  :keymap `((,(kbd "TAB") . outline-cycle)
+	    (,(kbd "M-TAB") . outline-cycle-buffer)
+	    ("n" . notmuch-tree-outline-next)
+	    ("p" . notmuch-tree-outline-previous)
+	    (,(kbd "M-n") . notmuch-tree-outline-next-thread)
+	    (,(kbd "M-p") . notmuch-tree-outline-previous-thread))
+  (outline-minor-mode notmuch-tree-outline-mode)
+  (unless (derived-mode-p 'notmuch-tree-mode)
+    (user-error "notmuch-tree-outline-mode is only meaningful for notmuch trees!"))
+  (if notmuch-tree-outline-mode
+      (progn (setq-local outline-regexp "^[^\n]+"
+			 outline-level #'notmuch-tree-outline--level)
+	     (notmuch-tree-outline--set-visibility))
+    (setq-local outline-regexp (default-value 'outline-regexp)
+		outline-level (default-value 'outline-level))))
+
 ;;; _
 
 (provide 'notmuch-tree)
-- 
2.38.1

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

[Jonathan Wilner]

I love this feature! I hope it could get mainstreamed. :-)
jao <jao@gnu.org> writes:

> This is essentiall the same as the previously sent v 6, rebased over
> the current master branch.  Since that previous version has not
> elicited special enthusiasm, yet i use it all the time, so if you guys
> think it's not worth integrating into notmuch-emacs upstream, please
> just let me know and i'll create a separate ELPA package for it.
>
> Thanks!
> jao
>
> jao (1):
>   emacs: notmuch-tree-outline-mode
>
>  doc/notmuch-emacs.rst |  23 ++++++
>  emacs/notmuch-tree.el | 180 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 203 insertions(+)
>
> --
> 2.38.1
> _______________________________________________
> notmuch mailing list -- notmuch@notmuchmail.org
> To unsubscribe send an email to notmuch-leave@notmuchmail.org

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

[jao]

On Fri, Nov 04 2022, jao wrote:

> This is essentiall the same as the previously sent v 6, rebased over
> the current master branch.  Since that previous version has not
> elicited special enthusiasm, yet i use it all the time, so if you guys
> think it's not worth integrating into notmuch-emacs upstream, please
> just let me know and i'll create a separate ELPA package for it.

i am actually going to take that back: my patch modifies only one line
of existing code (notmuch-tree.el:1037), to set a new message property.
tiny as the change is, it'd be quite complicated to do the same
externally, from a package (barring very ugly hacks like an :override
advice)... so i guess i'll keep using my private version of
notmuch-tree.el if this patch cannot get in :)

cheers,
jao
-- 
Lower your voice and strengthen your argument. -Lebanese proverb

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

[Matt Armstrong]

Jonathan Wilner <jonathan@teamwilner.com> writes:

> I love this feature! I hope it could get mainstreamed. :-)

Yes, seems like a nice idea to me.

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

[David Bremner]

jao <jao@gnu.org> writes:
>
> diff --git a/doc/notmuch-emacs.rst b/doc/notmuch-emacs.rst
> index 846f5e67..b5f88a98 100644
> --- a/doc/notmuch-emacs.rst
> +++ b/doc/notmuch-emacs.rst
> @@ -606,6 +606,29 @@ can be controlled by the variable ``notmuch-search-oldest-first``.
>     See also :el:defcustom:`notmuch-search-result-format` and
>     :el:defcustom:`notmuch-unthreaded-result-format`.
>  
> +It is also possible to enable outlines in notmuch tree buffers, via
> +``notmuch-tree-outline-mode``.
> +

I'm looking at the HTML output (I guess it's similar in the info output)
and I think this probably needs a sub-heading 


> +|docstring::notmuch-tree-outline-mode|

Unfortunately the more ambitious docstrings don't really transfer across
to rst. Probably it is easier to write a new blurb rather than fix the tooling.

I also felt like I wasn't really clear on what to do with the minor mode
from the docstring.

>  
> diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
> index b3c2c992..47bd17bc 100644
> --- a/emacs/notmuch-tree.el
> +++ b/emacs/notmuch-tree.el
> @@ -1034,6 +1034,8 @@ message together with all its descendents."
>      (setq msg (plist-put msg :first (and first (eq 0 depth))))
>      (setq msg (plist-put msg :tree-status tree-status))
>      (setq msg (plist-put msg :orig-tags (plist-get msg :tags)))
> +    (setq msg (plist-put msg
> +			 :level (1+ (if (and (eq 0 depth) (not first)) 1
> depth))))

Probably I'm just being lazy, but it would be nice to have a comment
here why level is needed in addition to depth.

more later, I hope...

d

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

[David Bremner]

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

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

[jao]

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

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

[David Bremner]

jao <jao@gnu.org> writes:
>>
>> 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).
>

depth is used for indentation in notmuch-show-mode, so it should be
(close to) what you want? There is already a function
notmuch-show-get-depth.

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

It depends on the buffer being named after the message-id. If I
understand the current code correctly, this depends on the default
naming in notmuch show, but that could change e.g. like it did for
notmuch-search. OTOH, I guess I don't really understand what this is
checking for, since I was using something based on
#'notmuch-tree-get-message-properties

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

[Jose A. Ortega Ruiz]

On Mon, Dec 12 2022, David Bremner wrote:

> jao <jao@gnu.org> writes:
>>>
>>> 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).
>>
>
> depth is used for indentation in notmuch-show-mode, so it should be
> (close to) what you want? There is already a function
> notmuch-show-get-depth.

that doesn't work in notmuch-tree buffers, which is where it's needed.
:depth is not even recorded as a message property there.  we can simply
rename :level to :depth as the new property there (although there's an
off-by-one difference in their values sometimes for implementation
details): since this will be in fact a brand new property in the
notmuch-tree buffer anyway, that shouldn't cause any trouble, right?

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

[Jose A. Ortega Ruiz]

On Mon, Dec 12 2022, David Bremner wrote:

>>>> +		       (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...
>
> It depends on the buffer being named after the message-id. If I
> understand the current code correctly, this depends on the default
> naming in notmuch show, but that could change e.g. like it did for
> notmuch-search. OTOH, I guess I don't really understand what this is
> checking for, since I was using something based on
> #'notmuch-tree-get-message-properties

the intent is to know whether the notmuch-tree-message-buffer is
displaying the message the point in notmuch-tree is at, so it's not
enough that there's a window showing it.  but yes, you're right that's
not a good way of checking.  Perhaps this:

     (defsubst notmuch-tree-outline--message-open-p ()
       (and (buffer-live-p notmuch-tree-message-buffer)
            (get-buffer-window notmuch-tree-message-buffer)
            (let ((id (notmuch-tree-get-message-id)))
              (with-current-buffer notmuch-tree-message-buffer
                (string= (notmuch-show-get-message-id) (or id ""))))))

?

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

[David Bremner]

"Jose A. Ortega Ruiz" <jao@gnu.org> writes:

>
> the intent is to know whether the notmuch-tree-message-buffer is
> displaying the message the point in notmuch-tree is at, so it's not
> enough that there's a window showing it.  but yes, you're right that's
> not a good way of checking.  Perhaps this:
>
>      (defsubst notmuch-tree-outline--message-open-p ()
>        (and (buffer-live-p notmuch-tree-message-buffer)
>             (get-buffer-window notmuch-tree-message-buffer)
>             (let ((id (notmuch-tree-get-message-id)))
>               (with-current-buffer notmuch-tree-message-buffer
>                 (string= (notmuch-show-get-message-id) (or id ""))))))
>
> ?

yeah, that looks better to me as a general approach

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

[Jose A. Ortega Ruiz]

On Mon, Dec 12 2022, David Bremner wrote:

> jao <jao@gnu.org> writes:
>>>
>>> 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).
>>
>
> depth is used for indentation in notmuch-show-mode, so it should be
> (close to) what you want? There is already a function
> notmuch-show-get-depth.

took a second look, and yes, it's close but not quite.  there are cases
(when the message is not the first but its depth is 0) where its outline
level doesn't match the depth. we could store the depth and then repeat
the computation everytime outline-mode requests the level, but it seems
better to compute it only once, and also calling it something other than
:depth, since they're not really the same thing.

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

[Jose A. Ortega Ruiz]

On Mon, Dec 12 2022, Jose A. Ortega Ruiz wrote:

> took a second look, and yes, it's close but not quite.  there are cases
> (when the message is not the first but its depth is 0) where its outline
> level doesn't match the depth. we could store the depth and then repeat
> the computation everytime outline-mode requests the level, but it seems
> better to compute it only once, and also calling it something other than
> :depth, since they're not really the same thing.

I've just sent a new version of the patch (v8) in a separate email,
addressing (i think) most of the low-hanging fruit in your review,
David.  The parts missing are those needing work, i am afraid: better
documentation and tests.

HTH,
jao
-- 
The effort to understand the universe is one of the very few things
which lifts human life a little above the level of farce and gives it
some of the grace of tragedy. - Steven Weinberg