Re: [PATCH v6 1/2] emacs: User-defined sections in notmuch-hello

[Dmitry Kurochkin <dmitry.kurochkin@gmail.com>]

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.

-	  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