44/2a251d872b2622f4fbfd50fa280f64a71921e7

   1 Return-Path: <pieter@praet.org>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5         by olra.theworths.org (Postfix) with ESMTP id DF237431FB6\r
   6         for <notmuch@notmuchmail.org>; Wed, 22 Feb 2012 10:44:20 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.7\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5\r
  12         tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
  13 Received: from olra.theworths.org ([127.0.0.1])\r
  14         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15         with ESMTP id 8s+-APUknbXF for <notmuch@notmuchmail.org>;\r
  16         Wed, 22 Feb 2012 10:44:20 -0800 (PST)\r
  17 Received: from mail-we0-f181.google.com (mail-we0-f181.google.com\r
  18         [74.125.82.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  19         (No client certificate requested)\r
  20         by olra.theworths.org (Postfix) with ESMTPS id A6A8A431FAE\r
  21         for <notmuch@notmuchmail.org>; Wed, 22 Feb 2012 10:44:19 -0800 (PST)\r
  22 Received: by werp13 with SMTP id p13so265640wer.26\r
  23         for <notmuch@notmuchmail.org>; Wed, 22 Feb 2012 10:44:18 -0800 (PST)\r
  24 Received-SPF: pass (google.com: domain of pieter@praet.org designates\r
  25         10.180.92.227 as permitted sender) client-ip=10.180.92.227; \r
  26 Authentication-Results: mr.google.com;\r
  27         spf=pass (google.com: domain of pieter@praet.org\r
  28         designates 10.180.92.227 as permitted sender)\r
  29         smtp.mail=pieter@praet.org\r
  30 Received: from mr.google.com ([10.180.92.227])\r
  31         by 10.180.92.227 with SMTP id cp3mr38259690wib.13.1329936258372\r
  32         (num_hops = 1); Wed, 22 Feb 2012 10:44:18 -0800 (PST)\r
  33 Received: by 10.180.92.227 with SMTP id cp3mr31648878wib.13.1329936258296;\r
  34         Wed, 22 Feb 2012 10:44:18 -0800 (PST)\r
  35 Received: from localhost ([109.131.181.26])\r
  36         by mx.google.com with ESMTPS id fw5sm30967066wib.0.2012.02.22.10.44.16\r
  37         (version=TLSv1/SSLv3 cipher=OTHER);\r
  38         Wed, 22 Feb 2012 10:44:17 -0800 (PST)\r
  39 From: Pieter Praet <pieter@praet.org>\r
  40 To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>,\r
  41         Notmuch Mail <notmuch@notmuchmail.org>\r
  42 Subject: Re: [PATCH] emacs: make `notmuch-show-open-or-close-all' toggle\r
  43         visibility\r
  44 In-Reply-To: <87wr7r80re.fsf@gmail.com>\r
  45 References: <1327469139-1968-1-git-send-email-pieter@praet.org>\r
  46         <87wr7r80re.fsf@gmail.com>\r
  47 User-Agent: Notmuch/0.11.1+210~g6afc43e (http://notmuchmail.org) Emacs/23.3.1\r
  48         (x86_64-unknown-linux-gnu)\r
  49 Date: Wed, 22 Feb 2012 19:41:58 +0100\r
  50 Message-ID: <87vcmy90cp.fsf@praet.org>\r
  51 MIME-Version: 1.0\r
  52 Content-Type: text/plain; charset=us-ascii\r
  53 X-Gm-Message-State:\r
  54  ALoCoQkFw+6mWgJZIVQc4Qgmlmm/OB2b46syy0+nbr+nqwkWRLQKnsSUfaBm/mXjynUiA+s7hY+6\r
  55 X-BeenThere: notmuch@notmuchmail.org\r
  56 X-Mailman-Version: 2.1.13\r
  57 Precedence: list\r
  58 List-Id: "Use and development of the notmuch mail system."\r
  59         <notmuch.notmuchmail.org>\r
  60 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  61         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  62 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  63 List-Post: <mailto:notmuch@notmuchmail.org>\r
  64 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  65 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  66         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  67 X-List-Received-Date: Wed, 22 Feb 2012 18:44:21 -0000\r
  68 \r
  69 On Mon, 13 Feb 2012 14:51:17 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:\r
  70 > Hi Pieter.\r
  71 > \r
  72 > On Wed, 25 Jan 2012 06:25:39 +0100, Pieter Praet <pieter@praet.org> wrote:\r
  73 > > * emacs/notmuch-show.el (notmuch-show-open-or-close-all):\r
  74 > >   Rename to `notmuch-show-toggle-all-messages', and make it toggle\r
  75 > >   visibility of all messages based on the visibility of the current\r
  76 > >   message, instead of setting visibility based on whether or not a\r
  77 > >   prefix arg was supplied.\r
  78 > > \r
  79 > > Same functionality, less effort (reaching for 'C-u' is a pain)...\r
  80 > > \r
  81 > > ---\r
  82 > >  emacs/notmuch-show.el |   22 ++++++++++++----------\r
  83 > >  1 files changed, 12 insertions(+), 10 deletions(-)\r
  84 > > \r
  85 > > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el\r
  86 > > index e6a5b31..2d17f74 100644\r
  87 > > --- a/emacs/notmuch-show.el\r
  88 > > +++ b/emacs/notmuch-show.el\r
  89 > > @@ -1050,8 +1050,8 @@ thread id.  If a prefix is given, crypto processing is toggled."\r
  90 > >     (define-key map "p" 'notmuch-show-previous-open-message)\r
  91 > >     (define-key map (kbd "DEL") 'notmuch-show-rewind)\r
  92 > >     (define-key map " " 'notmuch-show-advance-and-archive)\r
  93 > > -   (define-key map (kbd "M-RET") 'notmuch-show-open-or-close-all)\r
  94 > >     (define-key map (kbd "RET") 'notmuch-show-toggle-message)\r
  95 > > +   (define-key map (kbd "M-RET") 'notmuch-show-toggle-all-messages)\r
  96 > \r
  97 > Should the function name include "visible" or "visibility" to make it\r
  98 > clear what is toggled?  E.g. `notmuch-show-toggle-visibility-all'.\r
  99 > \r
 100 > Also, consider changing "all-messages" to just "all" or "thread".  That\r
 101 > seems to be more consistent with other functions.\r
 102 >\r
 103 \r
 104 Good point, but we also have `notmuch-show-toggle-message' and\r
 105 `notmuch-show-toggle-headers', so `notmuch-show-toggle-visibility-all'\r
 106 would imply that both messages as well as headers are toggled.\r
 107 \r
 108 Also, `notmuch-show-toggle-visibility-thread' sounds to me like\r
 109 it would toggle the thread itself instead of the messages of\r
 110 which it is composed, so my personal expectation would be that\r
 111 it just blanks the entire buffer. (what's in a name....)\r
 112 \r
 113 How about renaming the relevant functions like so:\r
 114 - `notmuch-show-toggle-headers'    -> `notmuch-show-toggle-visibility-headers'\r
 115 - `notmuch-show-toggle-message'    -> `notmuch-show-toggle-visibility-message'\r
 116 - `notmuch-show-open-or-close-all' -> `notmuch-show-toggle-visibility-messages'\r
 117 \r
 118 > >     (define-key map "#" 'notmuch-show-print-message)\r
 119 > >     map)\r
 120 > >        "Keymap for \"notmuch show\" buffers.")\r
 121 > > @@ -1502,16 +1502,18 @@ the result."\r
 122 > >       (not (plist-get props :message-visible))))\r
 123 > >    (force-window-update))\r
 124 > >  \r
 125 > > -(defun notmuch-show-open-or-close-all ()\r
 126 > > -  "Set the visibility all of the messages in the current thread.\r
 127 > > -By default make all of the messages visible. With a prefix\r
 128 > > -argument, hide all of the messages."\r
 129 > > +(defun notmuch-show-toggle-all-messages ()\r
 130 > > +  "Toggle the visibility of all messages in the current thread.\r
 131 > > +If the current message is visible, hide all messages -- and vice versa."\r
 132 > >    (interactive)\r
 133 > > -  (save-excursion\r
 134 > > -    (goto-char (point-min))\r
 135 > > -    (loop do (notmuch-show-message-visible (notmuch-show-get-message-properties)\r
 136 > > -                                      (not current-prefix-arg))\r
 137 > > -     until (not (notmuch-show-goto-message-next))))\r
 138 > > +  (let ((toggle (notmuch-show-message-visible-p)))\r
 139 > \r
 140 > Please rename "toggle" to "visible-p".  That would make it more clear\r
 141 > what the variable means, and is consistent with\r
 142 > `notmuch-show-message-visible-p'.\r
 143 >\r
 144 \r
 145 AFAIK the '-p' suffix is "reserved" for predicate functions, and\r
 146 using it for a variable could be confusing.  But I'm not aware of\r
 147 any guidelines on indicating the variable type except when it\r
 148 stores one or more functions [1,2]...\r
 149 \r
 150 Perhaps we could call it `visible-bool' ?\r
 151 \r
 152 Anyways, I've gone with your suggestion: `visible-p' it is...\r
 153 \r
 154 > > +    (save-excursion\r
 155 > > +      (goto-char (point-min))\r
 156 > > +      (loop do (notmuch-show-message-visible\r
 157 > > +           (notmuch-show-get-message-properties)\r
 158 > > +           (not toggle))\r
 159 > > +       until (not (notmuch-show-goto-message-next)))))\r
 160 > \r
 161 > A new `notmuch-show-mapc' function was introduced in a recent commit.\r
 162 > Please use it here instead of a custom loop.\r
 163 >\r
 164 \r
 165 Nice!\r
 166 \r
 167 > > +  (recenter-top-bottom 1)\r
 168 > \r
 169 > There was no `recenter-top-bottom' call before.  Why is it needed now?\r
 170 > It seems like an independent change and, if it is needed, would be\r
 171 > better as a separate patch.  At the very least, it's worth mentioning in\r
 172 > the preamble and perhaps in a comment.\r
 173 >\r
 174 \r
 175 It ensures that the message being uncollapsed is put properly in view\r
 176 (instead of starting somewhere in the middle of the buffer) whilst also\r
 177 making it obvious that/if/when there's previous messages in the thread\r
 178 (due to its argument being 1 instead of 0).\r
 179 \r
 180 I thought about using `notmuch-show-message-adjust' instead, but that\r
 181 obscures the fact that there's previous messages.\r
 182 \r
 183 As it's quite essential in making the function DTRT, I've opted to\r
 184 clarify it in a comment as well as the commit message instead of\r
 185 splitting it out into a separate patch.\r
 186 \r
 187 > Regards,\r
 188 >   Dmitry\r
 189 > \r
 190 > >    (force-window-update))\r
 191 > >  \r
 192 > >  (defun notmuch-show-next-button ()\r
 193 > > -- \r
 194 > > 1.7.8.1\r
 195 > > \r
 196 > > _______________________________________________\r
 197 > > notmuch mailing list\r
 198 > > notmuch@notmuchmail.org\r
 199 > > http://notmuchmail.org/mailman/listinfo/notmuch\r
 200 \r
 201 So... I've expanded the test suite to cover everything I might be\r
 202 breaking, renamed the toggle functions to be consistent, and addressed\r
 203 all your comments in some way or another.  I've also thrown in a bonus\r
 204 patch which is *not* meant to be applied (WIP, should eventually provide\r
 205 functionality similar to `notmuch-search-filter{,-by-tag}').\r
 206 \r
 207 Patches follow.\r
 208 \r
 209 \r
 210 Peace\r
 211 \r
 212 -- \r
 213 Pieter\r
 214 \r
 215 [1] http://www.gnu.org/software/emacs/manual/html_node/elisp/Coding-Conventions.html\r
 216 [2] http://www.gnu.org/software/emacs/manual/html_node/elisp/Hooks.html\r