From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Daniel Schoepe <daniel@schoepe.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v6 1/2] emacs: User-defined sections in notmuch-hello
In-Reply-To: <87aa6vvodi.fsf@gmail.com>
References: <87ippzysmv.fsf@steelpick.2x.cz>
	<1318253982-23588-1-git-send-email-daniel@schoepe.org>
	<1318253982-23588-2-git-send-email-daniel@schoepe.org>
	<87aa6vvodi.fsf@gmail.com>
User-Agent: Notmuch/0.10.2+96~g74e5ae5 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Wed, 14 Dec 2011 16:55:36 +0400
Message-ID: <877h1zuxbr.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Daniel Schoepe <daniel.schoepe@googlemail.com>
Precedence: list

On Wed, 14 Dec 2011 07:11:21 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Hi Daniel.
> 
> I have finished reviewing this patch at last.  Sorry, it is a bit messy.
> Overall, I like the patch.  It is a very nice improvement.
> 
> I am sure I have missed some important points, but I guess this is the
> best I can do right now.  Perhaps I will find more comments for the next
> version of the patch :)
> 
> As we already discussed on IRC, there are some trailing whitespaces to
> cleanup.
> 
> Here is the review:
> 
> +(defvar notmuch-custom-section-options
> 
> s/notmuch-custom-section-options/notmuch-hello-custom-section-options/ for consistency?
> 
> +    (:filter-count (string :tag "Different filter message counts"))
> 
> It was not clear to me what this option is for from the docstring.
> Perhaps something like: "Count query filter, if different from :filter"?
> 
> +    (:initially-hidden (const :tag "Hide this on startup?" t))
> 
> "This" refers to section, right?  If yes, let's state it explicitly:
> "Hide this section on startup".  Also, we should probably remove the
> question mark, or add it to other options for consistency.
> 
> Should the default be to show all sections?
> 
> +    (:hide-if-empty (const :tag "Hide if empty" t)))
> 
> As I understand, this controls whether the whole sections is visible.
> It is not clear what "if empty" means.  Does it mean that all queries
> are empty?  Or all queries are empty and :show-empty-sections is
> false?  Consider changing to something like: "Hide this section if all
> queries are empty [and hidden]".
> 
> +  `(list :tag ""
> +	 (const :tag "" notmuch-hello-insert-query-list)
> 
> Do we need to explicitly specify empty tags?  Aren't they empty by
> default?
> 
> +  :tag "Customized tag-list (see docstring for details)"
> +  :tag "Customized queries section (see docstring for details)"
> 
> Perhaps it would be more useful to add reference to
> `notmuch-hello-sections'?  I.e. "see `notmuch-hello-sections' for
> details.
> 
> Please s/Customized tag-list/Customized tag-list section/ everywhere for
> consistency (or remove section from "Customized queries section").
> 
> +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
> +output and nil otherwise.
> 
> Something is missing between "return if".  IMO it is really hard to
> understand what the function should actually do and what it should
> return.  Are this functions expected to add section content to current
> position?  As I understand, the return value indicates whether cursor
> should be positioned somewhere inside this section.  It is a minor
> detail, but it is described in the first (and complex sentence) as if
> it was the most important part.  Consider moving the return and "no
> arguments" to the 3rd paragraph which describes details about the
> functions.  I would also swap 2nd and 3rd paragraph.  Smth like:
> 
>   The list contains functions which are used to construct sections in
>   notmuch-hello buffer.  When notmuch-hello buffer is constructed,
>   these functions are run in the order they appear in this list.  Each
>   function produces a section simply by adding content to the current
>   buffer.  A section should not end with an empty line, because a
>   newline will be inserted after each section by `notmuch-hello'.
> 
>   Each function should take no arguments.  If the produced section
>   includes `notmuch-hello-target' (i.e. cursor should be positioned
>   inside this section), the function should return [something].
>   Otherwise, it should return nil.
> 
>   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.
> 
>   [ details about customized tag-list and queries sections ]
> 
> This is just a draft.  Feel free to use it or ignore it.
> 
> + For convenience an element can also be
> 
> Remove space the leading space and do `fill-paragraph'.
> 
> +	    (function :tag "Custom function"))))
> 
> Perhaps "Custom section" would be more accurate?
> 
> +  "Button at position of point before rebuilding the notmuch-buffer
> 
> Missing dot at the end.
> 
> s/Button/Button text/?
> 
> +This variable contains the string of the button, if any, the
> 
> s/the string/text/ or label?
> 
> +rebuilt. This is never actually set globally and defined as a
> 
> s/is never actually set/should never be set/?
> 
> +(defvar notmuch-hello-hidden-sections nil
> +  "List of query section titles whose contents are hidden")
> 
> Is this really for query sections only?
> 
> Does this duplicate :initially-hidden option from
> notmuch-custom-section-options?
> 
> How about adding a global alist variable notmuch-hello-state to store
> the state needed for section functions?  Currently, it would contain
> two values: :first-run and :target.  This would allow us to add more
> state variables in the future without polluting the global namespace.
> Also, it would make clear what variables are section function are
> supposed to use and perhaps even change (docstring should explain that
> of course).
> 
> `notmuch-hello-filtered-query':
> 
> +      (and result (concat query " and (" result ")"))))
> 
> How about using the result as query instead of filter?  I.e. returning
> the result without adding the query to it.  IMO it is simpler and more
> flexible.  In particular, that would allow the function to return nil
> in case the query should not be shown.
> 
> Query should be put in ().
> 
> +    (concat query " and (" filter ")"))
> 
> Same here.
> 
> +   (t (concat query))))
> 
> Why do we need concat here?
> 
> `notmuch-hello-query-counts':
> 
> +		(notmuch-hello-filtered-query (cdr query-and-count)
> +					      (or (plist-get options :filter-count)
> +						 (plist-get options :filter)))))))
> 
> and
> 
> +	   (list name (notmuch-hello-filtered-query
> +		       (car query-and-count) (plist-get options :filter))
> +		 message-count))))
> 
> We already handled :filter and :filter-count options in
> `notmuch-hello-generate-tag-alist'.  We should just use the generated
> queries passed in query-alist.
> 
> It seems that `notmuch-hello-query-counts' should handle only
> :show-empty-searches option.  If that is true, docstring should be
> updated accordingly.  Also, I think it is better to pass a single
> :show-empty-searches option as a parameter instead of the whole
> options plist.
> 

After thinking more about it, handling :filter and :filter-count in
`notmuch-hello-query-counts' is useful.  I may want to set "not
tag:spam" filter for all saved searches (if we add this customize option
later).  So `notmuch-hello-query-counts' should handle :filter and
:filter-count options, while `notmuch-hello-generate-tag-alist' should
just handle :hide-tags and return ("name" . "tag:name") list.

Actually, we should rename :hide-tags to more general :hide-queries or
:hide-search and handle it in `notmuch-hello-query-counts' as well.
`notmuch-hello-generate-tag-alist' would become very simple: it would
just get all tags from notmuch and make list of ("name" . "tag:name")
for each.  All query-related options would be handled on a single place.
Currently, :hide-tags is used only for tags, it makes little sense to
hide saved searches.  But one may want to add a section which gets list
of queries from some external source (similar to notmuch search-tags)
and hiding some queries would make sense.

Regards,
  Dmitry

> -	  reordered-list)
> +	  searches)
> 
> I am not sure if this is a mistake.  I hope it is not, because it
> allows us to remove some code :) If this change is correct, please
> make it a separate patch and remove unused reordered-list variable,
> notmuch-hello-reflect and notmuch-hello-reflect-generate-row
> functions.  Otherwise, revert the change.
> 
> - "Major mode for convenient notmuch navigation. This is your entry portal into notmuch.
> +  "Major mode for convenient notmuch navigation. This is your entry portal into notmuch.
> 
> Please revert.
> 
> - (interactive)
> - (kill-all-local-variables)
> - (use-local-map notmuch-hello-mode-map)
> - (setq major-mode 'notmuch-hello-mode
> -       mode-name "notmuch-hello")
> - ;;(setq buffer-read-only t)
> -)
> -
> +  (interactive)
> +  (kill-all-local-variables)
> +  (use-local-map notmuch-hello-mode-map)
> +  (setq major-mode 'notmuch-hello-mode
> +	mode-name "notmuch-hello"))
> +
> 
> Please revert.  The commented out line may be removed in a separate patch.
> 
> `notmuch-hello-generate-tag-alist':
> 
> +		     (list tag (notmuch-hello-filtered-query tag filter-query)
> 
> It should be (concat "tag:" tag) instead of tag.  Besides we already
> have it in the query variable, so just use it.
> 
> +		   (cons tag (notmuch-hello-filtered-query
> +			      (concat "tag:" tag) filter-query))))))
> 
> Same as above: use the query variable.
> 
> `notmuch-hello-insert-saved-searches':
> 
> Looks like we do not need both `final-target-pos'.  Can we just return
> `found-target-pos'?
> 
> `notmuch-hello-insert-search':
> 
> +  (insert "\n"))
> 
> Should this be `widget-insert'?
> 
> Note that there are changes in master that need to be merged into
> `notmuch-hello-insert-search' during rebase.
> 
> `notmuch-hello-insert-searches':
> 
> if my above comments on `notmuch-hello-query-counts' are correct, the
> docstring should be fixed because `notmuch-hello-insert-searches' does
> not handle :filter and :filter-count options.  Would be nice to move
> this documentation somewhere instead of deleting it.
> 
> +	  (searches (apply 'notmuch-hello-query-counts query-alist options)))
> 
> Why do we need `apply' here?
> 
> `notmuch-hello-insert-tags-section':
> 
> +  "Insert a section displaying all tags and message counts for each.
> 
> Perhaps s/and message counts for each/with message counts/?
> 
> `notmuch-hello-insert-inbox':
> 
> Perhaps change docstring to something more consistent with other
> notmuch-hello-insert-* functions?  E.g.:
> 
>   Insert a section displaying saved search and inbox messages for each tag.
> 
> +				  (notmuch-hello-generate-tag-alist))
> +				 :filter "tag:inbox"))
> 
> If my above comments are correct, then :filter option should be given
> to `notmuch-hello-generate-tag-alist' instead of
> `notmuch-hello-insert-searches'.
> 
> `notmuch-hello-insert-alltags':
> 
> Missing dot at the end of docstring.
> 
> Perhaps s/and associated message counts/with message counts/?
> 
> `notmuch-hello':
> 
> -  ; Jump through a hoop to get this value from the deprecated variable
> -  ; name (`notmuch-folders') or from the default value.
> +					; Jump through a hoop to get this value from the deprecated variable
> +					; name (`notmuch-folders') or from the default value.
> 
> Please revert.
> 
>    (if (not notmuch-saved-searches)
> -    (setq notmuch-saved-searches (notmuch-saved-searches)))
> +      (setq notmuch-saved-searches (notmuch-saved-searches)))
> 
> Please revert.
> 
> +    (setq notmuch-hello-first-run nil)))
> 
> Please move this statement to the top level.  There is no need for it
> to be inside let.
> 
> 
> Regards,
>   Dmitry