Re: [PATCH] emacs: wash: make word-wrap bound message width

[notmuch-archives.git] / 5c / 2a2977287feb9db143fb234520a25962e6576d

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