7d/e0d3291ccaae8622e332fd051d9f6721eeea53

   1 Return-Path: <cworth@cworth.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 6860D431FBC\r
   6         for <notmuch@notmuchmail.org>; Wed, 24 Feb 2010 10:49: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: -3.265\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-3.265 tagged_above=-999 required=5\r
  12         tests=[ALL_TRUSTED=-1.8, AWL=1.134, BAYES_00=-2.599] autolearn=ham\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 flBOLfamYsjz; Wed, 24 Feb 2010 10:49:19 -0800 (PST)\r
  16 Received: from yoom.home.cworth.org (localhost [127.0.0.1])\r
  17         by olra.theworths.org (Postfix) with ESMTP id 772E2431FAE;\r
  18         Wed, 24 Feb 2010 10:49:19 -0800 (PST)\r
  19 Received: by yoom.home.cworth.org (Postfix, from userid 1000)\r
  20         id CB3FE25427B; Wed, 24 Feb 2010 10:49:16 -0800 (PST)\r
  21 From: Carl Worth <cworth@cworth.org>\r
  22 To: Matthieu Lemerre <racin@free.fr>, notmuch@notmuchmail.org\r
  23 In-Reply-To: <87y6l7144e.fsf@free.fr>\r
  24 References: <87y6l7144e.fsf@free.fr>\r
  25 Date: Wed, 24 Feb 2010 10:49:16 -0800\r
  26 Message-ID: <87tyt6wjtf.fsf@yoom.home.cworth.org>\r
  27 MIME-Version: 1.0\r
  28 Content-Type: multipart/signed; boundary="=-=-=";\r
  29         micalg=pgp-sha1; protocol="application/pgp-signature"\r
  30 Subject: Re: [notmuch] [PATCH] Support for deletion (patch included)\r
  31 X-BeenThere: notmuch@notmuchmail.org\r
  32 X-Mailman-Version: 2.1.13\r
  33 Precedence: list\r
  34 List-Id: "Use and development of the notmuch mail system."\r
  35         <notmuch.notmuchmail.org>\r
  36 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  37         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  38 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  39 List-Post: <mailto:notmuch@notmuchmail.org>\r
  40 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  41 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  42         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  43 X-List-Received-Date: Wed, 24 Feb 2010 18:49:20 -0000\r
  44 \r
  45 --=-=-=\r
  46 Content-Transfer-Encoding: quoted-printable\r
  47 \r
  48 On Sun, 13 Dec 2009 12:54:09 +0100, Matthieu Lemerre <racin@free.fr> wrote:\r
  49 > I forgot the attachment..\r
  50 \r
  51 Hi Matthieu,\r
  52 \r
  53 This is a very interesting patch. Thanks for contributing it.\r
  54 \r
  55 Could you also write a commit message describing what the patch does?\r
  56 The easiest way for me to apply that would be if you would create a git\r
  57 commit, then run "git format-patch origin/master" and mail the resulting\r
  58 files, (the "git send-email" command can be used here, or you can insert\r
  59 the files into a mail-composition buffer and modify them as needed).\r
  60 \r
  61 A couple of minor comments on the patch:\r
  62 \r
  63 >      (define-key map "a" 'notmuch-search-archive-thread)\r
  64 > +    (define-key map "d" 'notmuch-search-mark-as-deleted)\r
  65 \r
  66 For consistency, let's name this notmuch-search-delete-thread.\r
  67 \r
  68 And we'll probably want a notmuch-show-delete-message function as well,\r
  69 no?\r
  70 \r
  71 > +(defvar notmuch-search-history nil)\r
  72 \r
  73 Excellent! I've wanted custom search history for a while, and just\r
  74 didn't know how to do it with (interactive "s"). It looks easy enough\r
  75 with read-string as you're doing here. But this is independent\r
  76 functionality, so would be preferred as an independent patch/commit.\r
  77 \r
  78 >    (forward-line))\r
  79 >=20=20\r
  80 > +\r
  81 > +(defun notmuch-search-mark-as-deleted ()\r
  82 > +  "Mark the currently selected thread as deleted (set its \"deleted\" ta=\r
  83 g).\r
  84 > +This function advances the next thread when finished."\r
  85 > +  (interactive)\r
  86 > +  (notmuch-search-add-tag "deleted")\r
  87 > +  (forward-line))\r
  88 > +\r
  89 > +\r
  90 >  (defun notmuch-search-process-sentinel (proc msg)\r
  91 \r
  92 Watch that extra whitespace. The convention is a single line of\r
  93 whitespace between each function.\r
  94 \r
  95 And should we also archive the thread before removing the deleted tag?\r
  96 \r
  97 > +With prefix argument, include deleted items.\r
  98 \r
  99 That's a pretty good interface I think.\r
 100 \r
 101 Another approach would be to do something like what sup does---that\r
 102 would be to scan the search terms and if it contains "tag:deleted" and\r
 103 all then don't prepend the "not tag:deleted and" to the search\r
 104 string. The assumption there is that if the user is explicitly\r
 105 mentioning the deleted tag, then we should just rely on the user to\r
 106 explicitly describe how the tag should be treated.\r
 107 \r
 108 That's perhaps not an unreasonable heuristic, and might be done even in\r
 109 addition to the prefix-argument approach. But that could be an\r
 110 additional commit, and I won't require it.\r
 111 \r
 112 > +  (interactive (let* ((prefix current-prefix-arg)\r
 113 > +                   (query (if prefix\r
 114 > +                              (read-string "Notmuch search (including deleted): "\r
 115 > +                                           notmuch-search-query-string\r
 116 > +                                           'notmuch-search-history)\r
 117 > +                            (read-string "Notmuch search: " nil\r
 118 > +                                         'notmuch-search-history))))\r
 119 \r
 120 Why is the second (initial-input) argument non-nil in one case, but nil\r
 121 in the other? The documentation for `read-string' says the argument is\r
 122 deprecated and should be nil in all new code.\r
 123 \r
 124 > +              (list query nil prefix)))\r
 125 > +  (let ((real-query (if include-deleted query=20\r
 126 > +                   (concat "not tag:deleted and (" query ")")))\r
 127 > +     (buffer (get-buffer-create (concat "*notmuch-search-" query\r
 128 > "*"))))\r
 129 \r
 130 Does the include-deleted case actually work? I don't see anything in the\r
 131 code that sets this variable. (I'm just reviewing here--I haven't tested\r
 132 it manually).\r
 133 \r
 134 > @@ -1351,7 +1374,6 @@ search."\r
 135 >=20=20\r
 136 >  Runs a new search matching only messages that match both the\r
 137 >  current search results AND the additional query string provided."\r
 138 > -  (interactive "sFilter search: ")\r
 139 >    (let ((grouped-query (if (string-match-p notmuch-search-disjunctive-re=\r
 140 gexp query) (concat "( " query " )") query)))\r
 141 >      (notmuch-search (concat notmuch-search-query-string " and " grouped-=\r
 142 query) notmuch-search-oldest-first)))\r
 143 \r
 144 Is this just an accidental chunk in the patch? I don't see why this\r
 145 function should become non-interactive now.\r
 146 \r
 147 Thanks again,\r
 148 \r
 149 =2DCarl\r
 150 \r
 151 --=-=-=\r
 152 Content-Type: application/pgp-signature\r
 153 \r
 154 -----BEGIN PGP SIGNATURE-----\r
 155 Version: GnuPG v1.4.10 (GNU/Linux)\r
 156 \r
 157 iD8DBQFLhXSs6JDdNq8qSWgRAi66AJ9NySB432DcQX+Xoj8xDOyJeEDW5ACfTdAO\r
 158 heqIkO3z9aeGcA/eJRLOtK0=\r
 159 =8MQE\r
 160 -----END PGP SIGNATURE-----\r
 161 --=-=-=--\r