From: Daniel Schoepe <daniel.schoepe@googlemail.com>
To: Michal Sojka <sojkam1@fel.cvut.cz>, notmuch@notmuchmail.org
Subject: Re: [PATCH v3 1/2] emacs: User-defined sections in notmuch-hello
In-Reply-To: <87fwmjabii.fsf@steelpick.2x.cz>
References: <1309379221-5617-1-git-send-email-daniel.schoepe@googlemail.com>
	<1309883030-28899-1-git-send-email-daniel.schoepe@googlemail.com>
	<1309883030-28899-2-git-send-email-daniel.schoepe@googlemail.com>
	<87fwmjabii.fsf@steelpick.2x.cz>
User-Agent: Notmuch/0.5-321-g7d7ebd8 (http://notmuchmail.org) Emacs/23.3.1
	(i486-pc-linux-gnu)
Date: Wed, 06 Jul 2011 14:41:09 +0200
Message-ID: <87oc17r38a.fsf@tredergarh.home.box>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: list

--=-=-=
Content-Transfer-Encoding: quoted-printable

On Wed, 06 Jul 2011 13:34:13 +0200, Michal Sojka <sojkam1@fel.cvut.cz> wrot=
e:
> > -(defcustom notmuch-hello-tag-list-make-query nil
> > -  "Function or string to generate queries for the all tags list.
>=20
> I'm not sure it is a good thing to remove customizations that are
> included in a released version. Some users may configure it and their
> configuration disappear when they install a new version. Is there a way
> to migrate this settings to a new, section-based one?

Agreed, I think I only removed it because at the time I first
implemented user-defined sections, my patch, which introduced
notmuch-hello-tag-list-make-query, hadn't been applied yet.

> This is IMHO still hard to understand only from looking at the
> customization widget. Ideally, I'd like to see somewhere a paragraph
> like this: "This displays a list of all tags, optionally hiding some of
> them. It is also possible to filter the list of messages matching each
> tag by an additional filter query. Similarly, the count of messages
> displayed next to the buttons can be generated by applying another
> filter to the tag query."
>=20
> Actually, when reading the above paragraph, it still seems a way too
> complex. What about the following: I do not think it has sense to list
> custom tags with zero counts - I think everybody would use
> :hide-empty-tags all the time. Then, the result of listing all tags with
> 'notmuch search-tags', filtering them with 'filter' and throwing away
> empty tags, would be the same as running 'notmuch search-tags filter'
> and then finding the counts by 'notmuch count tag:TAG and filter'. One
> of the advantage of this approach is that it would probably be faster to
> generate the section, because you won't query the counts of all tags if
> your filter matches only a few of them.
>=20
> Additionally, as I didn't read carefully your previous discussions about
> the additional filter for counts, I do not see much use for it. If you
> only want to see which tags has unread messages, you can simply add a
> new section with (:make-query "tag:unread") and you would get what you
> want. You can also disable all-tags sections.
>=20
> So my proposal is to forget about different queries for filters and
> counts and having here only the following options: filter, hide-tags and
> hide-if-empty.
>=20
> Then, the documentation would be simple: "This section displays buttons
> for all tags of messages matching a FILTER. Optionally, some of these
> tags may be hidden."
>=20
> Is there a use case, which is not covered by this?

I'm not sure, I can imagine someone wanting to have an overview of all
his tags, whether there are, e.g., unread messages or not. If we decide
to keep this functionality, it should be inverted though, i.e. one has
to explicitly specify :show-empty-searches to get them.

About the counts: I introduced this because Austin Clements says he
finds it useful in his comment here:

id:"BANLkTi=3D729DWai4q57iBSfz1wDhBXsmndQ@mail.gmail.com"

Also, I think/hope that we can just improve the documentation
sufficiently without sacrificing flexibility.

>=20
> > +  :type
> > +  (let ((opts
> > +	 '((:title (string :tag "Title for this section"))
> > +	   (:make-query (string :tag "Filter for each tag"))
> > +	   (:make-count (string :tag "Different query to generate counts"))
> > +	   (:hide-tags (repeat :tag "Tags that will be hidden" string))
>=20
> I can imagine, that :hide-tags could also be a (list of) regexp(es).
>=20
> > +	   (:initially-hidden (boolean :tag "Hide this on startup?"))
>=20
> This is IMHO not needed here, as you always has to enable this section
> manually.

A user might still want to have the section collapsed when starting the
notmuch UI and only have it shown when he needs it. (I use that for a
section that displays unread counts for each tag).

>=20
> > +		 (:hide-empty-tags
>=20
> Rename to :hide-empty-queries.
>=20
> > +		  (boolean :tag "Hide tags with no matching messages"))
>=20
> Hide queries...

Right, thanks.

>=20
> > +		 (:hide-if-empty (boolean :tag "Hide if empty"))))))
> > +
> > +(defcustom notmuch-hello-sections
> > +  (list #'notmuch-hello-insert-header
> > +	#'notmuch-hello-insert-saved-searches
> > +	#'notmuch-hello-insert-search
> > +	#'notmuch-hello-insert-recent-searches
> > +	#'notmuch-hello-insert-alltags
> > +	#'notmuch-hello-insert-footer)
> > +  "Sections for notmuch-hello.
> > +
> > +Each entry of this list should be a function of no arguments that
> > +should return if notmuch-hello-target is produced as part of its
>=20
> What is notmuch-hello-target? I guess I know what it is from reading the
> code, but if I didn't read the code, this mentioning it here would be of
> little value for me. Perhaps make the notmuch-hello-target clickable and
> document it below.

Yes, you're right, this needs more documentation.

>=20
> > +output and nil otherwise. For convenience an element can also be
> > +a list of the form (FUNC ARG1 ARG2 .. ARGN) in which case FUNC
> > +will be applied to the rest of the list.
> > +
> > +The functions will be run to construct the content of the
> > +notmuch-hello buffer in the order they appear in this list."
> > +  :group 'notmuch
> > +  :type=20
> > +  '(repeat
> > +    (choice (function-item notmuch-hello-insert-header)
> > +	    (function-item notmuch-hello-insert-saved-searches)
> > +	    (function-item notmuch-hello-insert-search)
> > +	    (function-item notmuch-hello-insert-recent-searches)
> > +	    (function-item notmuch-hello-insert-alltags)
> > +	    (function-item notmuch-hello-insert-footer)
> > +	    (function-item notmuch-hello-insert-inbox)
> > +	    notmuch-hello-tags-section
> > +	    notmuch-hello-query-section
> > +	    (function :tag "Custom function"))))
> > +
> > +;; only defined to avoid compilation warnings about free variables
> > +(defvar notmuch-hello-target nil)
>=20
> Add the documentation as discussed above. Additionally, it seems that
> having only the label in this variable is not enough, since the same
> label can appear in multiple sections. Perhaps, we need both the title
> of the section and the label here.

What do you mean by label? "Custom function"? If yes, that element will
have the actual function name in the input element next to it anyway.

> > +  ;;(setq buffer-read-only t)
>=20
> Don't we want to get rid of this line?

I guess so, it was there before my patch though.

>=20
> > +  )
> > +
> > +(defun notmuch-hello-generate-tag-alist (&optional hide-tags filter-qu=
ery filter-count)
> >    "Return an alist from tags to queries to display in the all-tags sec=
tion."
> >    (notmuch-remove-if-not
> > -   #'cdr
> > +   #'identity
> >     (mapcar (lambda (tag)
> > -	     (cons tag
> > -		   (cond
> > -		    ((functionp notmuch-hello-tag-list-make-query)
> > -		     (concat "tag:" tag " and ("
> > -			     (funcall notmuch-hello-tag-list-make-query tag) ")"))
> > -		    ((stringp notmuch-hello-tag-list-make-query)
> > -		     (concat "tag:" tag " and ("
> > -			     notmuch-hello-tag-list-make-query ")"))
> > -		    (t (concat "tag:" tag)))))
> > +	     (let ((query (notmuch-hello-filtered-query (concat "tag:" tag)
> > +							filter-query)))
> > +	       (when query
> > +		 (if filter-count
> > +		     (list tag (notmuch-hello-filtered-query tag filter-query)
> > +			   (notmuch-hello-filtered-query (concat "tag:" tag)
> > +							 filter-count))
> > +		   (cons tag (notmuch-hello-filtered-query
> > +			      (concat "tag:" tag) filter-query))))))
> >  	   (notmuch-remove-if-not
> >  	    (lambda (tag)
> > -	      (not (member tag notmuch-hello-hide-tags)))
> > +	      (not (member tag hide-tags)))
> >  	    (process-lines notmuch-command "search-tags")))))
> >=20=20
> > +(defun notmuch-hello-insert-header ()
> > +  "Insert the default notmuch-hello header."
> > +  (when notmuch-show-logo
> > +    (let ((image notmuch-hello-logo))
> > +      ;; The notmuch logo uses transparency. That can display poorly
> > +      ;; when inserting the image into an emacs buffer (black logo on
> > +      ;; a black background), so force the background colour of the
> > +      ;; image. We use a face to represent the colour so that
> > +      ;; `defface' can be used to declare the different possible
> > +      ;; colours, which depend on whether the frame has a light or
> > +      ;; dark background.
> > +      (setq image (cons 'image
> > +			(append (cdr image)
> > +				(list :background (face-background 'notmuch-hello-logo-background)=
))))
> > +      (insert-image image))
> > +    (widget-insert "  "))
> > +
> > +  (widget-insert "Welcome to ")
> > +  ;; Hack the display of the links used.
> > +  (let ((widget-link-prefix "")
> > +	(widget-link-suffix ""))
> > +    (widget-create 'link
> > +		   :notify (lambda (&rest ignore)
> > +			     (browse-url notmuch-hello-url))
> > +		   :help-echo "Visit the notmuch website."
> > +		   "notmuch")
> > +    (widget-insert ". ")
> > +    (widget-insert "You have ")
> > +    (widget-create 'link
> > +		   :notify (lambda (&rest ignore)
> > +			     (notmuch-hello-update))
> > +		   :help-echo "Refresh"
> > +		   (notmuch-hello-nice-number
> > +		    (string-to-number (car (process-lines notmuch-command "count")))=
))
> > +    (widget-insert " messages.")))
>=20
> Perhaps you want to end this (and also all other) section with an empty
> line so that the order of sections doesn't matter. I use sections in
> this order: header, inbox, saved-searches and there is no space between
> header and inbox and two spaces between inbox and saved-searches.

My thinking was to have no section end with a newline and insert a
newline between each section when building the notmuch-hello buffer, to
prevent forgotten trailing newlines when defining a new section.

> I might be useful to include here a link to the customization of this
> page. Maybe, it would be useful to have notmuch-hello subgroup in
> customization interface and link to this group. But creation of the
> subgroup should be definitely in a separate patch.

Yes definitely. Pieter Praet recently sent a patch reorganizing the
customization options into subgroups, so I'll change it once that patch
has been applied.

Cheers,
Daniel

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJOFFflAAoJEIaTAtce+Z+J4zgQAIVBNWufY2dbYvTsWLUexJBf
ATbk3NfNvme2VnpNnTSRsrWLUuXzEbgqch0j08NDqdK1eh89NGOgbHhRSuRSqKAu
sIFEO2fIDfS9iPl+MxpBZJS3F6tSUnjIiD8JIerAM590rCufRsglfRPOwyX2cdhY
xPhVLYIN+9hStdEGtNl4NHiEMzDvdsYPqU+tSgSpA6t9kZXFRw8qg6s87+W+1m9s
o0nL16mM6UyYRABrTEpIeZA4NmKzZDoJkc8N3+nMvJ8w7I3wYjaf+8cf20+uJf6S
X707vktct6vgJ6U3QEapzXYqUBKKyRcgfZZg5Ui5689SZpf3/FQ8Kjx064IYxWQJ
M/ixMExRLtIeezE2nRJc/XNtOxnEe9x0K4emTuX+xCN49g4XARw4OJbG3IJfFZcN
r4qDl3SLs++q/y3liEVx2CMyacDR67cHN8S6FcmgXFM+uxZST+YJruR3/+QUEc9T
YFbcggGmhX6iSDN5UF31hVK9MS5ehRJiOVbKiCzgCgn7ARv88hlxMNSPQA8OeAMW
bF5of3tAm2Ec5d1RmJe0vsNXOi+he+GJZJ3a9rhTxNKDWHtaHNAyluukaOAzSH1D
DUYyocVz2mkcX6Gu/rsfVzoTk8tTY96XZz+aiRkpTD1bSpLzfrdkJRLNpP1F2Jmm
duzLXx5JyZv+cL212f3x
=bYeE
-----END PGP SIGNATURE-----
--=-=-=--