From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/7] emacs: tag: allow default case in notmuch-tag-formats
In-Reply-To: <87ob2e0wyp.fsf@qmul.ac.uk>
References: <1390087855-26194-1-git-send-email-markwalters1009@gmail.com>
	<1390087855-26194-3-git-send-email-markwalters1009@gmail.com>
	<87txc6n84q.fsf@awakening.csail.mit.edu>
	<87ob2e0wyp.fsf@qmul.ac.uk>
User-Agent: Notmuch/0.17~rc2+14~g06f47e0 (http://notmuchmail.org) Emacs/23.4.1
	(i486-pc-linux-gnu)
Date: Tue, 11 Feb 2014 17:57:51 -0500
Message-ID: <87r479mf4g.fsf@awakening.csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Tue, 11 Feb 2014, Mark Walters <markwalters1009@gmail.com> wrote:
> Thanks for the review.
>
> On Mon, 10 Feb 2014, Austin Clements <amdragon@MIT.EDU> wrote:
>> On Sat, 18 Jan 2014, Mark Walters <markwalters1009@gmail.com> wrote:
>>> Allow an empty string in notmuch-tag-formats which matches all tags
>>> except those matched explicitly matched. This allows the user to tell
>>
>> Typo.
>
> Will fix.
>
>>> notmuch to hide all tags except those specified.
>>>
>>> This will be useful once formatting for deleted/added tags is added
>>> later in the series: a user might want to hide all deleted tags for
>>> example.
>>> ---
>>>  emacs/notmuch-tag.el |   20 +++++++++++---------
>>>  1 files changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
>>> index 2153068..92c1249 100644
>>> --- a/emacs/notmuch-tag.el
>>> +++ b/emacs/notmuch-tag.el
>>> @@ -65,14 +65,15 @@
>>>  This gives a list that maps from tag names to lists of formatting
>>>  expressions.  The car of each element gives a tag name and the
>>>  cdr gives a list of Elisp expressions that modify the tag.  If
>>> -the list is empty, the tag will simply be hidden.  Otherwise,
>>> -each expression will be evaluated in order: for the first
>>> -expression, the variable `tag' will be bound to the tag name; for
>>> -each later expression, the variable `tag' will be bound to the
>>> -result of the previous expression.  In this way, each expression
>>> -can build on the formatting performed by the previous expression.
>>> -The result of the last expression will displayed in place of the
>>> -tag.
>>> +the car is an empty string it matches all tags that do not have
>>> +an explicit match.  If the list is empty, the tag will simply be
>>
>> Hmm.  I'm not sure I like overloading of the meanings of strings.  Could
>> we instead use a symbol to represent this case?  For example, `t' would
>> parallel the fall-through case of `cond' and `case', or `_' would
>> parallel `pcase' [1].  Or even a separate variable like
>> notmuch-tag-default-format?
>
> I would prefer not to have a separate variable as I want the default
> case for added/deleted tags too (see next patch), so it would need to be
> 3 separate variables. But maybe that makes things clearer and is worth
> doing?

Ah, I see.  In that case I agree this shouldn't be a separate variable
(that idea was just a workaround anyway).

> One other possibility that would solve the customize problem would be to
> allow regexps for the matches. The regexp would have to match the
> complete tag and only the first match would be used. This has advantages
> (the user can highlight all notmuch::.* tags or can hide all X-
> tags). The downside is that finding/testing for a regexp match is about
> 20 times slower than using assoc, primarily because assoc is written in
> C and the regexp match bit in lisp.

I really like the idea of using regexps for this.  I wouldn't worry
about the performance.  We can almost certainly eliminate that problem
with a sprinkle of caching (which would probably be even faster than
assoc), and that can be added separately.

>> The former would require some tweaking of the customize widget, but that
>> should probably happen anyway to support this special case.
>> Unfortunately, we may need a custom alist widget variant to do that.  (I
>> tried and failed to tweak it in a way that both worked and looked
>> decent.)  Hence my suggestion of a separate variable, which would only
>> require pulling out the :value-type into its own define-widget.
>
> If we go this route I may need some help getting this to work: pulling
> out value-type didn't work on my first attempt.
>
>> I'm also slightly bothered that this would introduce a second way to
>> control the default formatting of tags in addition to notmuch-tag-face,
>> but only slightly.
>
> Yes that slightly bothered me but I didn't see a solution.
>
>> [1] It's unfortunate that pcase wasn't introduced until Emacs 24.  I've
>> been tempted to backport it for notmuch multiple times now.  Then we
>> could just treat notmuch-tag-formats as a list of pcase conditions.
>
> Would that still involve a lisp loop so would be comparable to the
> regexp bit above? I haven't looked at pcase enough to work out its
> details.

pcase compiles into a decision tree, which could in principle reduce
this to a fast lookup when possible, though I don't know if the
implementation is actually that smart.  At any rate, I like the idea of
using regexps for this better anyway.

> Best wishes
>
> Mark
>
>
>>
>>> +hidden.  Otherwise, each expression will be evaluated in order:
>>> +for the first expression, the variable `tag' will be bound to the
>>> +tag name; for each later expression, the variable `tag' will be
>>> +bound to the result of the previous expression.  In this way,
>>> +each expression can build on the formatting performed by the
>>> +previous expression.  The result of the last expression will
>>> +displayed in place of the tag.
>>>  
>>>  For example, to replace a tag with another string, simply use
>>>  that string as a formatting expression.  To change the foreground
>>> @@ -140,7 +141,8 @@ This can be used with `notmuch-tag-format-image-data'."
>>>  
>>>  (defun notmuch-tag-format-tag (tag)
>>>    "Format TAG by looking into `notmuch-tag-formats'."
>>> -  (let ((formats (assoc tag notmuch-tag-formats)))
>>> +  (let ((formats (or (assoc tag notmuch-tag-formats)
>>> +		     (assoc "" notmuch-tag-formats))))
>>>      (cond
>>>       ((null formats)		;; - Tag not in `notmuch-tag-formats',
>>>        tag)			;;   the format is the tag itself.
>>> -- 
>>> 1.7.9.1
>>>
>>> _______________________________________________
>>> notmuch mailing list
>>> notmuch@notmuchmail.org
>>> http://notmuchmail.org/mailman/listinfo/notmuch