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?

Agreed.

> 
> +    (: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"?

This option is for displaying a different number of messages next to the
button, than actually match the query the button links to. I think this is
something that was requested a while ago in the context of my patch that
added notmuch-hello-tag-list-make-query.

> +    (: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.

Agreed.

> Should the default be to show all sections?

That's the default I'd prefer, since I want to see most of the section
I define by default. If you have some less bike-shedy arguments for
changing this, I'm all ears.

> 
> +    (: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]".

Okay, I'll clarify this in the next version.

> +  `(list :tag ""
> +	 (const :tag "" notmuch-hello-insert-query-list)
> 
> Do we need to explicitly specify empty tags?  Aren't they empty by
> default?

It displays the symbol after the const if this is missing.

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

Since this is mainly displayed in the drop-down menu for the section
type in the customize page for notmuch-hello-sections (or
customize-group 'notmuch), references a) don't work there and b)
usually would point to the same thing as the user is currently editing.

> Please s/Customized tag-list/Customized tag-list section/ everywhere for
> consistency (or remove section from "Customized queries section").

Done.

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

Thanks, that is quite a bit clearer than what I wrote.

> + 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?

Yes, it would.

> 
> +  "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/?

Sounds good.

> 
> +(defvar notmuch-hello-hidden-sections nil
> +  "List of query section titles whose contents are hidden")
> 
> Is this really for query sections only?

No, good catch.

> 
> Does this duplicate :initially-hidden option from
> notmuch-custom-section-options?

This is actually for keeping track of which sections the user chose to
hide via the "[hide]"-button. Changing the corresponding field
:initially-hidden field in notmuch-sections is not possible for
arbitrary sections, i.e. functions and would alter what the users for
customize. Also, exiting notmuch and then starting it again would no
longer do what the user set in his configuration, if he didn't want to
hide it in the beginning, but then clicked on [hide] later.

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

Given that all the variables are prefixed with notmuch-hello already, I
don't think that the polluting the namespace is really a problem. And
given that section functions would have to access target, I'd rather
keep it in a separate variable so people who write a new type of section
don't have to inspect some detail in a variable whose other components
are internal implementation details (this is the case for first-run and
hidden-sections). If this actually becomes a problem we can still
move them there later.

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

The entry is already hidden if the function returns nil, because of the
`and'. I agree that it's simpler though, so I changed it.

> 
> Query should be put in ().
> 
> +    (concat query " and (" filter ")"))
> 
> Same here.
> 
> +   (t (concat query))))
> 
> Why do we need concat here?

Fixed.

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

Yes, I handled that by moving all the code for that to -query-counts,
like you suggested in your other mail.

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

It was definitely unintended and changes the default behavior, which is
to have the strings sorted within the columns instead of the rows.

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

I moved it to notmuch-hello-insert-tags-section, which actually
does handle those option and is a high-level function that will
probably be used a lot more by users.

> 
> +	  (searches (apply 'notmuch-hello-query-counts query-alist options)))
> 
> Why do we need `apply' here?

Because we want each item in `options' to be passed as an individual
argument. Note that apply is a bit peculiar about its last argument.

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

Changed, thanks.

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

Fixed.

I'll send my current version shortly, which does not yet include
Michal's performance improvement, because there probably still be a few
rough edges. The performance improvement could also be put in separate
patch so I don't have to keep rebasing this uncomfortably big patch for
much longer.

Thank you for your very detailed review.

Cheers,
Daniel