[PATCH v6 1/2] emacs: User-defined sections in notmuch-hello
Dmitry Kurochkin
dmitry.kurochkin at gmail.com
Wed Dec 14 04:55:36 PST 2011
On Wed, 14 Dec 2011 07:11:21 +0400, Dmitry Kurochkin <dmitry.kurochkin at 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
More information about the notmuch
mailing list