From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Daniel Schoepe <daniel@schoepe.org>, notmuch@notmuchmail.org
Cc: Daniel Schoepe <daniel.schoepe@googlemail.com>
Subject: Re: [PATCH v6 1/2] emacs: User-defined sections in notmuch-hello
Date: Wed, 14 Dec 2011 16:55:36 +0400 [thread overview]
Message-ID: <877h1zuxbr.fsf@gmail.com> (raw)
In-Reply-To: <87aa6vvodi.fsf@gmail.com>
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
next prev parent reply other threads:[~2011-12-14 12:56 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <id:"1309379221-5617-1-git-send-email-daniel.schoepe@googlemail.com">
2011-07-07 22:53 ` [PATCH v4 0/2] emacs: User-defined sections in notmuch-hello Daniel Schoepe
2011-07-07 22:53 ` [PATCH v4 1/2] " Daniel Schoepe
2011-07-08 23:00 ` Michal Sojka
2011-07-08 23:13 ` Daniel Schoepe
2011-07-09 5:35 ` Michal Sojka
2011-07-09 17:07 ` Daniel Schoepe
2011-07-07 22:53 ` [PATCH v4 2/2] emacs: Tests for user-defined sections Daniel Schoepe
2011-07-09 18:03 ` [PATCH v5 0/2] emacs: User-defined sections in notmuch-hello Daniel Schoepe
2011-07-09 18:03 ` [PATCH v5 1/2] " Daniel Schoepe
2011-07-09 18:03 ` [PATCH v5 2/2] emacs: Tests for user-defined sections Daniel Schoepe
2011-07-11 10:32 ` [PATCH] emacs: NEWS entry " Daniel Schoepe
2011-08-14 16:55 ` [PATCH v5 0/2] emacs: User-defined sections in notmuch-hello Daniel Schoepe
2011-08-15 8:40 ` Michal Sojka
2011-10-10 13:39 ` [PATCH v6 " Daniel Schoepe
2011-10-10 13:39 ` [PATCH v6 1/2] " Daniel Schoepe
2011-11-24 13:54 ` David Bremner
2011-11-24 14:01 ` Daniel Schoepe
2011-11-24 15:43 ` Michal Sojka
2011-11-28 4:06 ` Dmitry Kurochkin
2011-11-28 7:57 ` Michal Sojka
2011-12-14 3:11 ` Dmitry Kurochkin
2011-12-14 12:55 ` Dmitry Kurochkin [this message]
2012-01-22 0:39 ` Daniel Schoepe
2012-01-22 0:54 ` [PATCH v7 " Daniel Schoepe
2012-01-22 0:54 ` [PATCH v7 2/2] emacs: Tests for user-defined sections Daniel Schoepe
2012-01-23 23:07 ` Dmitry Kurochkin
2012-01-28 21:30 ` Daniel Schoepe
2012-01-28 21:44 ` [PATCH v8 0/2] emacs: User-defined sections in notmuch-hello Daniel Schoepe
2012-01-28 21:44 ` [PATCH v8 1/2] " Daniel Schoepe
2012-01-28 21:44 ` [PATCH v8 2/2] emacs: Tests for user-defined sections Daniel Schoepe
2012-01-28 22:48 ` [PATCH v7 " Dmitry Kurochkin
2012-01-28 22:54 ` Daniel Schoepe
2011-10-10 13:39 ` [PATCH v6 " Daniel Schoepe
2011-10-13 14:09 ` [PATCH] emacs-hello: Do not calculate the count of the messages in Michal Sojka
2012-01-16 11:33 ` David Edmondson
2012-01-16 12:39 ` Daniel Schoepe
2012-01-16 10:59 ` [PATCH v6 0/2] emacs: User-defined sections in notmuch-hello David Edmondson
2012-01-16 11:13 ` Daniel Schoepe
2012-02-17 7:48 ` [PATCH v9 " Dmitry Kurochkin
2012-02-17 7:48 ` [PATCH v9 1/2] " Dmitry Kurochkin
2012-02-17 7:48 ` [PATCH v9 2/2] emacs: Tests for user-defined sections Dmitry Kurochkin
2012-02-17 14:48 ` [PATCH v10 0/2] emacs: User-defined sections in notmuch-hello Dmitry Kurochkin
2012-02-17 14:48 ` [PATCH v10 1/2] " Dmitry Kurochkin
2012-03-01 12:36 ` David Bremner
2012-03-01 14:57 ` Michal Sojka
2012-03-01 15:00 ` Dmitry Kurochkin
2012-03-07 19:53 ` Pieter Praet
2012-03-07 20:04 ` Daniel Schoepe
2012-03-07 20:11 ` David Bremner
2012-02-17 14:48 ` [PATCH v10 2/2] emacs: Tests for user-defined sections Dmitry Kurochkin
2012-02-18 22:10 ` [PATCH v10 0/2] emacs: User-defined sections in notmuch-hello Michal Sojka
2012-02-18 22:12 ` [PATCH] emacs-hello: Do not calculate the count of the messages in hidden sections Michal Sojka
2012-03-01 22:18 ` Mark Walters
2012-03-02 0:34 ` Daniel Schoepe
2012-03-02 0:36 ` Daniel Schoepe
2012-03-05 2:00 ` Dmitry Kurochkin
2012-03-10 14:23 ` David Bremner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://notmuchmail.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=877h1zuxbr.fsf@gmail.com \
--to=dmitry.kurochkin@gmail.com \
--cc=daniel.schoepe@googlemail.com \
--cc=daniel@schoepe.org \
--cc=notmuch@notmuchmail.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://yhetil.org/notmuch.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).