--- /dev/null
+Return-Path: <dmitry.kurochkin@gmail.com>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by olra.theworths.org (Postfix) with ESMTP id AACCD429E2F\r
+ for <notmuch@notmuchmail.org>; Wed, 14 Dec 2011 04:56:20 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.799\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
+ tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
+ FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
+Received: from olra.theworths.org ([127.0.0.1])\r
+ by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id 67hslP3k5qI9 for <notmuch@notmuchmail.org>;\r
+ Wed, 14 Dec 2011 04:56:19 -0800 (PST)\r
+Received: from mail-fx0-f53.google.com (mail-fx0-f53.google.com\r
+ [209.85.161.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id 2E08C429E27\r
+ for <notmuch@notmuchmail.org>; Wed, 14 Dec 2011 04:56:19 -0800 (PST)\r
+Received: by faaa5 with SMTP id a5so1226475faa.26\r
+ for <notmuch@notmuchmail.org>; Wed, 14 Dec 2011 04:56:16 -0800 (PST)\r
+DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
+ h=from:to:cc:subject:in-reply-to:references:user-agent:date\r
+ :message-id:mime-version:content-type;\r
+ bh=FCpf1oJfGQsnOpqAwkrEXhnTrunbAoherA5cm+dOQGE=;\r
+ b=lAu36UbYuLTGVeelLfMn6Sk9NoUnwG2GUoT9UM1sS6bITMm0PapFnJtmXYRCN5svuL\r
+ GtSLZzs2QUlAxU1JEqITts40a917VkE+ZGtXArdqR67gnC4DptncgNWZbVZekFwwZrSC\r
+ gYcYzIGRiGFeQ/ygK0864ONBfWHz/HMOtYuPM=\r
+Received: by 10.180.18.233 with SMTP id z9mr4531871wid.0.1323867376461;\r
+ Wed, 14 Dec 2011 04:56:16 -0800 (PST)\r
+Received: from localhost ([91.144.186.21])\r
+ by mx.google.com with ESMTPS id m5sm3329463wie.2.2011.12.14.04.56.13\r
+ (version=TLSv1/SSLv3 cipher=OTHER);\r
+ Wed, 14 Dec 2011 04:56:14 -0800 (PST)\r
+From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
+To: Daniel Schoepe <daniel@schoepe.org>, notmuch@notmuchmail.org\r
+Subject: Re: [PATCH v6 1/2] emacs: User-defined sections in notmuch-hello\r
+In-Reply-To: <87aa6vvodi.fsf@gmail.com>\r
+References: <87ippzysmv.fsf@steelpick.2x.cz>\r
+ <1318253982-23588-1-git-send-email-daniel@schoepe.org>\r
+ <1318253982-23588-2-git-send-email-daniel@schoepe.org>\r
+ <87aa6vvodi.fsf@gmail.com>\r
+User-Agent: Notmuch/0.10.2+96~g74e5ae5 (http://notmuchmail.org) Emacs/23.3.1\r
+ (x86_64-pc-linux-gnu)\r
+Date: Wed, 14 Dec 2011 16:55:36 +0400\r
+Message-ID: <877h1zuxbr.fsf@gmail.com>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain; charset=us-ascii\r
+Cc: Daniel Schoepe <daniel.schoepe@googlemail.com>\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.13\r
+Precedence: list\r
+List-Id: "Use and development of the notmuch mail system."\r
+ <notmuch.notmuchmail.org>\r
+List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
+List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
+List-Post: <mailto:notmuch@notmuchmail.org>\r
+List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
+List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
+ <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
+X-List-Received-Date: Wed, 14 Dec 2011 12:56:20 -0000\r
+\r
+On Wed, 14 Dec 2011 07:11:21 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:\r
+> Hi Daniel.\r
+> \r
+> I have finished reviewing this patch at last. Sorry, it is a bit messy.\r
+> Overall, I like the patch. It is a very nice improvement.\r
+> \r
+> I am sure I have missed some important points, but I guess this is the\r
+> best I can do right now. Perhaps I will find more comments for the next\r
+> version of the patch :)\r
+> \r
+> As we already discussed on IRC, there are some trailing whitespaces to\r
+> cleanup.\r
+> \r
+> Here is the review:\r
+> \r
+> +(defvar notmuch-custom-section-options\r
+> \r
+> s/notmuch-custom-section-options/notmuch-hello-custom-section-options/ for consistency?\r
+> \r
+> + (:filter-count (string :tag "Different filter message counts"))\r
+> \r
+> It was not clear to me what this option is for from the docstring.\r
+> Perhaps something like: "Count query filter, if different from :filter"?\r
+> \r
+> + (:initially-hidden (const :tag "Hide this on startup?" t))\r
+> \r
+> "This" refers to section, right? If yes, let's state it explicitly:\r
+> "Hide this section on startup". Also, we should probably remove the\r
+> question mark, or add it to other options for consistency.\r
+> \r
+> Should the default be to show all sections?\r
+> \r
+> + (:hide-if-empty (const :tag "Hide if empty" t)))\r
+> \r
+> As I understand, this controls whether the whole sections is visible.\r
+> It is not clear what "if empty" means. Does it mean that all queries\r
+> are empty? Or all queries are empty and :show-empty-sections is\r
+> false? Consider changing to something like: "Hide this section if all\r
+> queries are empty [and hidden]".\r
+> \r
+> + `(list :tag ""\r
+> + (const :tag "" notmuch-hello-insert-query-list)\r
+> \r
+> Do we need to explicitly specify empty tags? Aren't they empty by\r
+> default?\r
+> \r
+> + :tag "Customized tag-list (see docstring for details)"\r
+> + :tag "Customized queries section (see docstring for details)"\r
+> \r
+> Perhaps it would be more useful to add reference to\r
+> `notmuch-hello-sections'? I.e. "see `notmuch-hello-sections' for\r
+> details.\r
+> \r
+> Please s/Customized tag-list/Customized tag-list section/ everywhere for\r
+> consistency (or remove section from "Customized queries section").\r
+> \r
+> +Each entry of this list should be a function of no arguments that\r
+> +should return if `notmuch-hello-target' is produced as part of its\r
+> +output and nil otherwise.\r
+> \r
+> Something is missing between "return if". IMO it is really hard to\r
+> understand what the function should actually do and what it should\r
+> return. Are this functions expected to add section content to current\r
+> position? As I understand, the return value indicates whether cursor\r
+> should be positioned somewhere inside this section. It is a minor\r
+> detail, but it is described in the first (and complex sentence) as if\r
+> it was the most important part. Consider moving the return and "no\r
+> arguments" to the 3rd paragraph which describes details about the\r
+> functions. I would also swap 2nd and 3rd paragraph. Smth like:\r
+> \r
+> The list contains functions which are used to construct sections in\r
+> notmuch-hello buffer. When notmuch-hello buffer is constructed,\r
+> these functions are run in the order they appear in this list. Each\r
+> function produces a section simply by adding content to the current\r
+> buffer. A section should not end with an empty line, because a\r
+> newline will be inserted after each section by `notmuch-hello'.\r
+> \r
+> Each function should take no arguments. If the produced section\r
+> includes `notmuch-hello-target' (i.e. cursor should be positioned\r
+> inside this section), the function should return [something].\r
+> Otherwise, it should return nil.\r
+> \r
+> For convenience an element can also be a list of the form (FUNC ARG1\r
+> ARG2 .. ARGN) in which case FUNC will be applied to the rest of the\r
+> list.\r
+> \r
+> [ details about customized tag-list and queries sections ]\r
+> \r
+> This is just a draft. Feel free to use it or ignore it.\r
+> \r
+> + For convenience an element can also be\r
+> \r
+> Remove space the leading space and do `fill-paragraph'.\r
+> \r
+> + (function :tag "Custom function"))))\r
+> \r
+> Perhaps "Custom section" would be more accurate?\r
+> \r
+> + "Button at position of point before rebuilding the notmuch-buffer\r
+> \r
+> Missing dot at the end.\r
+> \r
+> s/Button/Button text/?\r
+> \r
+> +This variable contains the string of the button, if any, the\r
+> \r
+> s/the string/text/ or label?\r
+> \r
+> +rebuilt. This is never actually set globally and defined as a\r
+> \r
+> s/is never actually set/should never be set/?\r
+> \r
+> +(defvar notmuch-hello-hidden-sections nil\r
+> + "List of query section titles whose contents are hidden")\r
+> \r
+> Is this really for query sections only?\r
+> \r
+> Does this duplicate :initially-hidden option from\r
+> notmuch-custom-section-options?\r
+> \r
+> How about adding a global alist variable notmuch-hello-state to store\r
+> the state needed for section functions? Currently, it would contain\r
+> two values: :first-run and :target. This would allow us to add more\r
+> state variables in the future without polluting the global namespace.\r
+> Also, it would make clear what variables are section function are\r
+> supposed to use and perhaps even change (docstring should explain that\r
+> of course).\r
+> \r
+> `notmuch-hello-filtered-query':\r
+> \r
+> + (and result (concat query " and (" result ")"))))\r
+> \r
+> How about using the result as query instead of filter? I.e. returning\r
+> the result without adding the query to it. IMO it is simpler and more\r
+> flexible. In particular, that would allow the function to return nil\r
+> in case the query should not be shown.\r
+> \r
+> Query should be put in ().\r
+> \r
+> + (concat query " and (" filter ")"))\r
+> \r
+> Same here.\r
+> \r
+> + (t (concat query))))\r
+> \r
+> Why do we need concat here?\r
+> \r
+> `notmuch-hello-query-counts':\r
+> \r
+> + (notmuch-hello-filtered-query (cdr query-and-count)\r
+> + (or (plist-get options :filter-count)\r
+> + (plist-get options :filter)))))))\r
+> \r
+> and\r
+> \r
+> + (list name (notmuch-hello-filtered-query\r
+> + (car query-and-count) (plist-get options :filter))\r
+> + message-count))))\r
+> \r
+> We already handled :filter and :filter-count options in\r
+> `notmuch-hello-generate-tag-alist'. We should just use the generated\r
+> queries passed in query-alist.\r
+> \r
+> It seems that `notmuch-hello-query-counts' should handle only\r
+> :show-empty-searches option. If that is true, docstring should be\r
+> updated accordingly. Also, I think it is better to pass a single\r
+> :show-empty-searches option as a parameter instead of the whole\r
+> options plist.\r
+> \r
+\r
+After thinking more about it, handling :filter and :filter-count in\r
+`notmuch-hello-query-counts' is useful. I may want to set "not\r
+tag:spam" filter for all saved searches (if we add this customize option\r
+later). So `notmuch-hello-query-counts' should handle :filter and\r
+:filter-count options, while `notmuch-hello-generate-tag-alist' should\r
+just handle :hide-tags and return ("name" . "tag:name") list.\r
+\r
+Actually, we should rename :hide-tags to more general :hide-queries or\r
+:hide-search and handle it in `notmuch-hello-query-counts' as well.\r
+`notmuch-hello-generate-tag-alist' would become very simple: it would\r
+just get all tags from notmuch and make list of ("name" . "tag:name")\r
+for each. All query-related options would be handled on a single place.\r
+Currently, :hide-tags is used only for tags, it makes little sense to\r
+hide saved searches. But one may want to add a section which gets list\r
+of queries from some external source (similar to notmuch search-tags)\r
+and hiding some queries would make sense.\r
+\r
+Regards,\r
+ Dmitry\r
+\r
+> - reordered-list)\r
+> + searches)\r
+> \r
+> I am not sure if this is a mistake. I hope it is not, because it\r
+> allows us to remove some code :) If this change is correct, please\r
+> make it a separate patch and remove unused reordered-list variable,\r
+> notmuch-hello-reflect and notmuch-hello-reflect-generate-row\r
+> functions. Otherwise, revert the change.\r
+> \r
+> - "Major mode for convenient notmuch navigation. This is your entry portal into notmuch.\r
+> + "Major mode for convenient notmuch navigation. This is your entry portal into notmuch.\r
+> \r
+> Please revert.\r
+> \r
+> - (interactive)\r
+> - (kill-all-local-variables)\r
+> - (use-local-map notmuch-hello-mode-map)\r
+> - (setq major-mode 'notmuch-hello-mode\r
+> - mode-name "notmuch-hello")\r
+> - ;;(setq buffer-read-only t)\r
+> -)\r
+> -\r
+> + (interactive)\r
+> + (kill-all-local-variables)\r
+> + (use-local-map notmuch-hello-mode-map)\r
+> + (setq major-mode 'notmuch-hello-mode\r
+> + mode-name "notmuch-hello"))\r
+> +\r
+> \r
+> Please revert. The commented out line may be removed in a separate patch.\r
+> \r
+> `notmuch-hello-generate-tag-alist':\r
+> \r
+> + (list tag (notmuch-hello-filtered-query tag filter-query)\r
+> \r
+> It should be (concat "tag:" tag) instead of tag. Besides we already\r
+> have it in the query variable, so just use it.\r
+> \r
+> + (cons tag (notmuch-hello-filtered-query\r
+> + (concat "tag:" tag) filter-query))))))\r
+> \r
+> Same as above: use the query variable.\r
+> \r
+> `notmuch-hello-insert-saved-searches':\r
+> \r
+> Looks like we do not need both `final-target-pos'. Can we just return\r
+> `found-target-pos'?\r
+> \r
+> `notmuch-hello-insert-search':\r
+> \r
+> + (insert "\n"))\r
+> \r
+> Should this be `widget-insert'?\r
+> \r
+> Note that there are changes in master that need to be merged into\r
+> `notmuch-hello-insert-search' during rebase.\r
+> \r
+> `notmuch-hello-insert-searches':\r
+> \r
+> if my above comments on `notmuch-hello-query-counts' are correct, the\r
+> docstring should be fixed because `notmuch-hello-insert-searches' does\r
+> not handle :filter and :filter-count options. Would be nice to move\r
+> this documentation somewhere instead of deleting it.\r
+> \r
+> + (searches (apply 'notmuch-hello-query-counts query-alist options)))\r
+> \r
+> Why do we need `apply' here?\r
+> \r
+> `notmuch-hello-insert-tags-section':\r
+> \r
+> + "Insert a section displaying all tags and message counts for each.\r
+> \r
+> Perhaps s/and message counts for each/with message counts/?\r
+> \r
+> `notmuch-hello-insert-inbox':\r
+> \r
+> Perhaps change docstring to something more consistent with other\r
+> notmuch-hello-insert-* functions? E.g.:\r
+> \r
+> Insert a section displaying saved search and inbox messages for each tag.\r
+> \r
+> + (notmuch-hello-generate-tag-alist))\r
+> + :filter "tag:inbox"))\r
+> \r
+> If my above comments are correct, then :filter option should be given\r
+> to `notmuch-hello-generate-tag-alist' instead of\r
+> `notmuch-hello-insert-searches'.\r
+> \r
+> `notmuch-hello-insert-alltags':\r
+> \r
+> Missing dot at the end of docstring.\r
+> \r
+> Perhaps s/and associated message counts/with message counts/?\r
+> \r
+> `notmuch-hello':\r
+> \r
+> - ; Jump through a hoop to get this value from the deprecated variable\r
+> - ; name (`notmuch-folders') or from the default value.\r
+> + ; Jump through a hoop to get this value from the deprecated variable\r
+> + ; name (`notmuch-folders') or from the default value.\r
+> \r
+> Please revert.\r
+> \r
+> (if (not notmuch-saved-searches)\r
+> - (setq notmuch-saved-searches (notmuch-saved-searches)))\r
+> + (setq notmuch-saved-searches (notmuch-saved-searches)))\r
+> \r
+> Please revert.\r
+> \r
+> + (setq notmuch-hello-first-run nil)))\r
+> \r
+> Please move this statement to the top level. There is no need for it\r
+> to be inside let.\r
+> \r
+> \r
+> Regards,\r
+> Dmitry\r
projects / notmuch-archives.git / commitdiff