[PATCH 2/2] emacs: Allow tagging regions in notmuch-tree

[Pierre Neidhardt]

Hi!

Thanks for your review.  I've submitted and updated patch.

> Thanks for contributing to Notmuch. Some generic comments:
>
> 1) Please consider a more comprehensive commit message [1].  The "why"
>    here is maybe obvious, but consider pointing out whether this makes
>    it more consistent with other parts of the UI (or not). Also, a (bit
>    more extended) of the changes is always helpful, both to help read
>    the diff and to warn of any potential breaking changes.

Done.

> 2) Please update doc/notmuch-emacs.rst.  You can re-use docstrings; let
>    me know if you need help doing that. We haven't been strict about
>    this is in the past, but the emacs docs are really lagging.

I've documented +/-.

> 3) We generally want at least one test for a new
>    feature. test/T460-emacs-tree.sh has the existing tree related
>    tests. Again, if you need help with the test suite, let us know.

I've added a test, but I wasn't able to run it on Guix
(https://guix.info).  It fails like this:

--8<---------------cut here---------------start------------->8---
> guix environment notmuch -- /home/ambrevar/.local/share/emacs/site-lisp/notmuch/test/T460-emacs-tree.sh
guix environment: error: execlp: No such file or directory: "/home/ambrevar/.local/share/emacs/site-lisp/notmuch/test/T460-emacs-tree.sh"
--8<---------------cut here---------------end--------------->8---

> 4) Please use notmuch-tree-- as a prefix for new private symbols that
>    users should not rely on. I'm not sure about which symbols that applies
>    to here.

I've mirrored the implementation of search-mode.
There are indeed a few functions that are only used locally, but so are
they in notmuch.el, so I guess this should be part of another patch.

>> +(defun notmuch-tree-foreach-result (beg end fn)
>
>> +  (lexical-let ((pos (notmuch-tree-result-beginning beg))
>
> As an aside, I'm curious if we can profitably start to use
> file level lexical-binding for parts of notmuch-emacs.

Absolutely, lexical bindings are heavily recommended:

- It enforces better coding practice.
- It's more performent.
- It will spot bugs! :)

I believe this should be part of a different patch set though.

>> +  (notmuch-tree-foreach-result beg end
>> +    (lambda (pos)
>> +      (save-mark-and-excursion
>
> I did wonder if notmuch-tree-foreach-result should be a macro. I'm not
> sure if the small gain in code compactness from just passing a "body" in
> the style of notmuch-show--with-currently-shown-message is worth it.

A macro could work here, but this would only save typing "(lambda".
Functions are preferable to macros if they can fit the bill, and I think
it's the case here.

Cheers!

-- 
Pierre Neidhardt
https://ambrevar.xyz/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 487 bytes
Desc: not available
URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20190514/4e395901/attachment.sig>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-emacs-Allow-tagging-regions-in-notmuch-tree.patch
Type: text/x-patch
Size: 5921 bytes
Desc: not available
URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20190514/4e395901/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-doc-emacs-document-tagging-bindings-for-notmuch-sear.patch
Type: text/x-patch
Size: 976 bytes
Desc: not available
URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20190514/4e395901/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-T460-emacs-tree-test-tagging-on-region-in-tree-view.patch
Type: text/x-patch
Size: 2311 bytes
Desc: not available
URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20190514/4e395901/attachment-0002.bin>