--- /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 290C7429E26\r
+ for <notmuch@notmuchmail.org>; Tue, 13 Dec 2011 19:12:04 -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 fCiVnK0tgsrl for <notmuch@notmuchmail.org>;\r
+ Tue, 13 Dec 2011 19:12:03 -0800 (PST)\r
+Received: from mail-ww0-f45.google.com (mail-ww0-f45.google.com\r
+ [74.125.82.45]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client\r
+ certificate requested) by olra.theworths.org (Postfix) with ESMTPS id\r
+ E4841429E25 for <notmuch@notmuchmail.org>; Tue, 13 Dec 2011 19:12:02 -0800\r
+ (PST)\r
+Received: by wgbds13 with SMTP id ds13so575066wgb.2\r
+ for <notmuch@notmuchmail.org>; Tue, 13 Dec 2011 19:12:00 -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=OYTCZESx+piR+sNuqs72az1kebqHwOt3wGD4w9YaEVU=;\r
+ b=J/ZyUhJLZb23aL3O0ie2Wm/jVpcBVLLjIRMlgDXAIXGrBwsWkmkj43bjBarDYFmhY+\r
+ Ugi6F4F53AJh0SoZADLgSmQ3IlD+SPlT62/4a7AHHkfC6rWGqoczkEcbU4vW4ZeqD6gQ\r
+ IX8VASxwKStObv/ndPjkKuOFxHfk5+28B49bI=\r
+Received: by 10.180.14.69 with SMTP id n5mr1654011wic.13.1323832320258;\r
+ Tue, 13 Dec 2011 19:12:00 -0800 (PST)\r
+Received: from localhost ([91.144.186.21])\r
+ by mx.google.com with ESMTPS id ej17sm1602459wbb.14.2011.12.13.19.11.57\r
+ (version=TLSv1/SSLv3 cipher=OTHER);\r
+ Tue, 13 Dec 2011 19:11:58 -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: <1318253982-23588-2-git-send-email-daniel@schoepe.org>\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
+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 07:11:21 +0400\r
+Message-ID: <87aa6vvodi.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 03:12:04 -0000\r
+\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
+- 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