--- /dev/null
+Return-Path: <daniel@schoepe.org>\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 87190429E54\r
+ for <notmuch@notmuchmail.org>; Sat, 21 Jan 2012 16:39:39 -0800 (PST)\r
+X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: -0.8\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=-0.8 tagged_above=-999 required=5\r
+ tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
+ 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 lnn9Ki4uFYCD for <notmuch@notmuchmail.org>;\r
+ Sat, 21 Jan 2012 16:39:38 -0800 (PST)\r
+Received: from mail-ey0-f181.google.com (mail-ey0-f181.google.com\r
+ [209.85.215.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
+ (No client certificate requested)\r
+ by olra.theworths.org (Postfix) with ESMTPS id 986DE429E40\r
+ for <notmuch@notmuchmail.org>; Sat, 21 Jan 2012 16:39:37 -0800 (PST)\r
+Received: by eaal1 with SMTP id l1so933034eaa.26\r
+ for <notmuch@notmuchmail.org>; Sat, 21 Jan 2012 16:39:35 -0800 (PST)\r
+DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=schoepe.org; s=google;\r
+ h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
+ :mime-version:content-type:x-gm-message-state;\r
+ bh=WFqcOlzp7R+t7ZE+qSVRyIDyHIgOW/9AcLFHLSv+7o4=;\r
+ b=kRgu2e648by1ahJu4OOcrrJjZRxLTIKVxiSgQuzqyTxwPyho2jSNH9+wKnkL2cQ1x9\r
+ kJhLewDAZVvuK4QDIL0BeBb+g0rd4ESxwevUjWi6AU4RgmBCnofdi31JDStTzPZmFTDR\r
+ g2vqouOO55yuaKhDSy36uc7RG3SfToYFQcnzU=\r
+Received: by 10.213.34.17 with SMTP id j17mr600473ebd.84.1327192774811;\r
+ Sat, 21 Jan 2012 16:39:34 -0800 (PST)\r
+Received: from localhost (dslb-188-107-198-094.pools.arcor-ip.net.\r
+ [188.107.198.94])\r
+ by mx.google.com with ESMTPS id t59sm32567809eeh.10.2012.01.21.16.39.31\r
+ (version=TLSv1/SSLv3 cipher=OTHER);\r
+ Sat, 21 Jan 2012 16:39:32 -0800 (PST)\r
+From: Daniel Schoepe <daniel@schoepe.org>\r
+To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>, 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.11+62~g888dddb (http://notmuchmail.org) Emacs/24.0.92.1\r
+ (x86_64-pc-linux-gnu)\r
+Date: Sun, 22 Jan 2012 01:39:26 +0100\r
+Message-ID: <87d3ac8ta9.fsf@schoepe.localhost>\r
+MIME-Version: 1.0\r
+Content-Type: multipart/signed; boundary="=-=-=";\r
+ micalg=pgp-sha1; protocol="application/pgp-signature"\r
+X-Gm-Message-State:\r
+ ALoCoQl9tmpXbO1MF/J5u1hIZsQY42u4KX9g1LZNhhD8l+Q/5kCrL5zaXLkEBPBUqC9PifAcu0Ol\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: Sun, 22 Jan 2012 00:39:39 -0000\r
+\r
+--=-=-=\r
+Content-Type: text/plain\r
+Content-Transfer-Encoding: quoted-printable\r
+\r
+On Wed, 14 Dec 2011 07:11:21 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmai=\r
+l.com> wrote:\r
+> Hi Daniel.\r
+>=20\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
+>=20\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
+>=20\r
+> As we already discussed on IRC, there are some trailing whitespaces to\r
+> cleanup.\r
+>=20\r
+> Here is the review:\r
+>=20\r
+> +(defvar notmuch-custom-section-options\r
+>=20\r
+> s/notmuch-custom-section-options/notmuch-hello-custom-section-options/\r
+> for consistency?\r
+\r
+Agreed.\r
+\r
+>=20\r
+> + (:filter-count (string :tag "Different filter message counts"))\r
+>=20\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
+This option is for displaying a different number of messages next to the\r
+button, than actually match the query the button links to. I think this is\r
+something that was requested a while ago in the context of my patch that\r
+added notmuch-hello-tag-list-make-query.\r
+\r
+> + (:initially-hidden (const :tag "Hide this on startup?" t))\r
+>=20\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
+Agreed.\r
+\r
+> Should the default be to show all sections?\r
+\r
+That's the default I'd prefer, since I want to see most of the section\r
+I define by default. If you have some less bike-shedy arguments for\r
+changing this, I'm all ears.\r
+\r
+>=20\r
+> + (:hide-if-empty (const :tag "Hide if empty" t)))\r
+>=20\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
+Okay, I'll clarify this in the next version.\r
+\r
+> + `(list :tag ""\r
+> + (const :tag "" notmuch-hello-insert-query-list)\r
+>=20\r
+> Do we need to explicitly specify empty tags? Aren't they empty by\r
+> default?\r
+\r
+It displays the symbol after the const if this is missing.\r
+\r
+>=20\r
+> + :tag "Customized tag-list (see docstring for details)"\r
+> + :tag "Customized queries section (see docstring for details)"\r
+>=20\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
+Since this is mainly displayed in the drop-down menu for the section\r
+type in the customize page for notmuch-hello-sections (or\r
+customize-group 'notmuch), references a) don't work there and b)\r
+usually would point to the same thing as the user is currently editing.\r
+\r
+> Please s/Customized tag-list/Customized tag-list section/ everywhere for\r
+> consistency (or remove section from "Customized queries section").\r
+\r
+Done.\r
+\r
+>=20\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
+>=20\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
+>=20\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
+>=20\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
+>=20\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
+>=20\r
+> [ details about customized tag-list and queries sections ]\r
+>=20\r
+> This is just a draft. Feel free to use it or ignore it.\r
+\r
+Thanks, that is quite a bit clearer than what I wrote.\r
+\r
+> + For convenience an element can also be\r
+>=20\r
+> Remove space the leading space and do `fill-paragraph'.\r
+>=20\r
+> + (function :tag "Custom function"))))\r
+>=20\r
+> Perhaps "Custom section" would be more accurate?\r
+\r
+Yes, it would.\r
+\r
+>=20\r
+> + "Button at position of point before rebuilding the notmuch-buffer\r
+>=20\r
+> Missing dot at the end.\r
+>=20\r
+> s/Button/Button text/?\r
+>=20\r
+> +This variable contains the string of the button, if any, the\r
+>=20\r
+> s/the string/text/ or label?\r
+>=20\r
+> +rebuilt. This is never actually set globally and defined as a\r
+>=20\r
+> s/is never actually set/should never be set/?\r
+\r
+Sounds good.\r
+\r
+>=20\r
+> +(defvar notmuch-hello-hidden-sections nil\r
+> + "List of query section titles whose contents are hidden")\r
+>=20\r
+> Is this really for query sections only?\r
+\r
+No, good catch.\r
+\r
+>=20\r
+> Does this duplicate :initially-hidden option from\r
+> notmuch-custom-section-options?\r
+\r
+This is actually for keeping track of which sections the user chose to\r
+hide via the "[hide]"-button. Changing the corresponding field\r
+:initially-hidden field in notmuch-sections is not possible for\r
+arbitrary sections, i.e. functions and would alter what the users for\r
+customize. Also, exiting notmuch and then starting it again would no\r
+longer do what the user set in his configuration, if he didn't want to\r
+hide it in the beginning, but then clicked on [hide] later.\r
+\r
+>=20\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
+Given that all the variables are prefixed with notmuch-hello already, I\r
+don't think that the polluting the namespace is really a problem. And\r
+given that section functions would have to access target, I'd rather\r
+keep it in a separate variable so people who write a new type of section\r
+don't have to inspect some detail in a variable whose other components\r
+are internal implementation details (this is the case for first-run and\r
+hidden-sections). If this actually becomes a problem we can still\r
+move them there later.\r
+\r
+>=20\r
+> `notmuch-hello-filtered-query':\r
+>=20\r
+> + (and result (concat query " and (" result ")"))))\r
+>=20\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
+The entry is already hidden if the function returns nil, because of the\r
+`and'. I agree that it's simpler though, so I changed it.\r
+\r
+>=20\r
+> Query should be put in ().\r
+>=20\r
+> + (concat query " and (" filter ")"))\r
+>=20\r
+> Same here.\r
+>=20\r
+> + (t (concat query))))\r
+>=20\r
+> Why do we need concat here?\r
+\r
+Fixed.\r
+\r
+>=20\r
+> `notmuch-hello-query-counts':\r
+>=20\r
+> + (notmuch-hello-filtered-query (cdr query-and-count)\r
+> + (or (plist-get options :filter-count)\r
+> + (plist-get options :filter)))))))\r
+>=20\r
+> and\r
+>=20\r
+> + (list name (notmuch-hello-filtered-query\r
+> + (car query-and-count) (plist-get options :filter))\r
+> + message-count))))\r
+>=20\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
+Yes, I handled that by moving all the code for that to -query-counts,\r
+like you suggested in your other mail.\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
+>=20\r
+> - reordered-list)\r
+> + searches)\r
+>=20\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
+It was definitely unintended and changes the default behavior, which is\r
+to have the strings sorted within the columns instead of the rows.\r
+\r
+>=20\r
+> - "Major mode for convenient notmuch navigation. This is your entry porta=\r
+l into notmuch.\r
+> + "Major mode for convenient notmuch navigation. This is your entry port=\r
+al into notmuch.\r
+>=20\r
+> Please revert.\r
+>=20\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
+>=20\r
+> Please revert. The commented out line may be removed in a separate patch.\r
+>=20\r
+> `notmuch-hello-generate-tag-alist':\r
+>=20\r
+> + (list tag (notmuch-hello-filtered-query tag filter-query)\r
+>=20\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
+>=20\r
+> + (cons tag (notmuch-hello-filtered-query\r
+> + (concat "tag:" tag) filter-query))))))\r
+>=20\r
+> Same as above: use the query variable.\r
+>=20\r
+> `notmuch-hello-insert-saved-searches':\r
+>=20\r
+> Looks like we do not need both `final-target-pos'. Can we just return\r
+> `found-target-pos'?\r
+>=20\r
+> `notmuch-hello-insert-search':\r
+>=20\r
+> + (insert "\n"))\r
+>=20\r
+> Should this be `widget-insert'?\r
+>=20\r
+> Note that there are changes in master that need to be merged into\r
+> `notmuch-hello-insert-search' during rebase.\r
+>=20\r
+> `notmuch-hello-insert-searches':\r
+>=20\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
+I moved it to notmuch-hello-insert-tags-section, which actually\r
+does handle those option and is a high-level function that will\r
+probably be used a lot more by users.\r
+\r
+>=20\r
+> + (searches (apply 'notmuch-hello-query-counts query-alist options)))\r
+>=20\r
+> Why do we need `apply' here?\r
+\r
+Because we want each item in `options' to be passed as an individual\r
+argument. Note that apply is a bit peculiar about its last argument.\r
+\r
+> `notmuch-hello-insert-tags-section':\r
+>=20\r
+> + "Insert a section displaying all tags and message counts for each.\r
+>=20\r
+> Perhaps s/and message counts for each/with message counts/?\r
+>=20\r
+> `notmuch-hello-insert-inbox':\r
+>=20\r
+> Perhaps change docstring to something more consistent with other\r
+> notmuch-hello-insert-* functions? E.g.:\r
+>=20\r
+> Insert a section displaying saved search and inbox messages for each\r
+> tag.\r
+\r
+Changed, thanks.\r
+\r
+>=20\r
+> + (notmuch-hello-generate-tag-alist))\r
+> + :filter "tag:inbox"))\r
+>=20\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
+>=20\r
+> `notmuch-hello-insert-alltags':\r
+>=20\r
+> Missing dot at the end of docstring.\r
+>=20\r
+> Perhaps s/and associated message counts/with message counts/?\r
+>=20\r
+> `notmuch-hello':\r
+>=20\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
+>=20\r
+> Please revert.\r
+>=20\r
+> (if (not notmuch-saved-searches)\r
+> - (setq notmuch-saved-searches (notmuch-saved-searches)))\r
+> + (setq notmuch-saved-searches (notmuch-saved-searches)))\r
+>=20\r
+> Please revert.\r
+>=20\r
+> + (setq notmuch-hello-first-run nil)))\r
+>=20\r
+> Please move this statement to the top level. There is no need for it\r
+> to be inside let.\r
+\r
+Fixed.\r
+\r
+I'll send my current version shortly, which does not yet include\r
+Michal's performance improvement, because there probably still be a few\r
+rough edges. The performance improvement could also be put in separate\r
+patch so I don't have to keep rebasing this uncomfortably big patch for\r
+much longer.\r
+\r
+Thank you for your very detailed review.\r
+\r
+Cheers,\r
+Daniel\r
+\r
+--=-=-=\r
+Content-Type: application/pgp-signature\r
+\r
+-----BEGIN PGP SIGNATURE-----\r
+Version: GnuPG v1.4.11 (GNU/Linux)\r
+\r
+iQIcBAEBAgAGBQJPG1q+AAoJEIaTAtce+Z+J6IEP/3Ri3FqBe5x7HAB5gmlPvWdQ\r
+Kw11Bt9IFbQCmmtvBy8IOgFddWyeu/zaFr/wyHkoqvRpg77t2k5kj9FP2jqTPHQf\r
+nETumv4KezLOxqcZfnfUgOmcibCGyDgTxLHBlmMXGX0/V/cvCBiBvPwgVzsIQyYC\r
+X5TJpedvjDzqQc8y8Xit/DJ5JsKQxRIStnRkNRahxp4R/dMcNVai3aGNHw3xnKAv\r
+IMro9CiTmshi9O2PkXnDpYxNKD5IrjhmTlfffsPNWlPM4d70miUyUTHxhmpre7xn\r
+sj9OKqmvLkhrdgVsO9CdSyF7YUWWYu2Q3MbI+TcEbgL3lkbY8GLa6y4b5Zm/0QIt\r
+RQYfkUf7EqR6IMdJ3n/ApEP9/PX6LkutNWh0DtYq3HYjnlBnzEb4V/7bWrnSoj5b\r
+no7UkenZrYUrTDUBrNkG4kOIyQYTSe5CgR5m4F4N0vHi9r2fNvnBKE8X8D4x5W2Z\r
+VhpGVzEp36CtAIH7txyiGkYKbgP4tiFybNlrnVU+qCEVHmkvHjW44dLza382E7K7\r
+GzED/L8YdQTxAZCCkFrvxeS/zbn8JoFAQcuGS1sJ2eQbL1v6L10rkp3w7ciGPSSV\r
+yU821CrXcb6jA/M6hR9VozkTrh5MhPnWZOclPzDbTTcLcVH+Dn7H3BN6h5PSlUJR\r
+bqSJmtCLNUDUsBLtFd/d\r
+=OPgO\r
+-----END PGP SIGNATURE-----\r
+--=-=-=--\r
projects / notmuch-archives.git / commitdiff