From: Mark Walters <markwalters1009@gmail.com>
To: Damien Cassou <damien.cassou@gmail.com>,
	notmuch mailing list <notmuch@notmuchmail.org>
Subject: Re: [PATCH v2] emacs: display tags in notmuch-show with links
In-Reply-To: <1352565719-12397-1-git-send-email-damien.cassou@gmail.com>
References: <1352565719-12397-1-git-send-email-damien.cassou@gmail.com>
User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1
	(i486-pc-linux-gnu)
Date: Wed, 14 Nov 2012 12:41:44 +0000
Message-ID: <878va4xson.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Hi

I have read this patch and am broadly happy with it. In particular I do
not think I have any "design concerns" with this version: I think
picking the tags from the visible messages is fine and this version
avoids the query and the thread-id stuff.

On the loading library issues I defer to Adam and Tomi.

I do have a couple of comments on the current version
though. First the patch is very big. I would prefer it split into
several pieces something like:

1) Add tags to the headerline but not clickable 
2) Add the header-button library if appropriate
3) Add notmuch-tagger and change headerline to buttonize the tags
4) Add the tests.

The first would be small and uncontroversial I think, for the second it
would not really be code review just "do we want to do it this way" as
is being discussed in the other thread. The third obviously has the meat
of the series and 4 is relatively innocuous.

Possibly you could also split the add clickable buttons to tags in the
buffer and add clickable buttons to headerline tags as two separate
steps.

One advantage of the split is that it would be easy to see who had
reviewed which bits. For example I have not read the
header-button-library bit or the tests but have looked at the rest.


My other comment is on your comments in notmuch-tagger

> +(defun notmuch-tagger-present-tags (tags &optional headerline)
> +  "Return a property list which nicely presents all TAGS.
> +
> +If HEADERLINE is non-nil the returned list will be ready for
> +inclusion in the buffer's header-line. HEADERLINE must be nil in
> +all other cases."

I find it odd to say what it returns if HEADERLINE is non-nil and then
say otherwise HEADERLINE must be nil. Could you say what it returns in
the nil case? Something along them lines of

"if HEADERLINE is non-nil the returned will be ready for inclusion in
the buffer's header-line (i.e., will use header-buttons if
available). Otherwise it returns a list?? ready for inclusion in a
buffer."

Best wishes

Mark