Re: [PATCH 2/7] emacs: tag: allow default case in notmuch-tag-formats

[Mark Walters <markwalters1009@gmail.com>]

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?

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.

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

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