fa/045484a3dff12c828c679dc4a6847aa2874ce6

   1 Return-Path: <ethan.glasser.camp@gmail.com>\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 AA36F431FB6\r
   6         for <notmuch@notmuchmail.org>; Fri, 19 Oct 2012 09:36:33 -0700 (PDT)\r
   7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.776\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.776 tagged_above=-999 required=5\r
  12         tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
  13         FREEMAIL_FROM=0.001, HS_INDEX_PARAM=0.023, RCVD_IN_DNSWL_LOW=-0.7]\r
  14         autolearn=disabled\r
  15 Received: from olra.theworths.org ([127.0.0.1])\r
  16         by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
  17         with ESMTP id uuwMOSeZbhHR for <notmuch@notmuchmail.org>;\r
  18         Fri, 19 Oct 2012 09:36:33 -0700 (PDT)\r
  19 Received: from mail-vb0-f53.google.com (mail-vb0-f53.google.com\r
  20         [209.85.212.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
  21         (No client certificate requested)\r
  22         by olra.theworths.org (Postfix) with ESMTPS id F11F8431FAE\r
  23         for <notmuch@notmuchmail.org>; Fri, 19 Oct 2012 09:36:32 -0700 (PDT)\r
  24 Received: by mail-vb0-f53.google.com with SMTP id fc21so715065vbb.26\r
  25         for <notmuch@notmuchmail.org>; Fri, 19 Oct 2012 09:36:32 -0700 (PDT)\r
  26 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113;\r
  27         h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
  28         :mime-version:content-type;\r
  29         bh=FL+NLvrYWzganYQhz/+4PHAe7lOrthGsOWAPFykAvzA=;\r
  30         b=TTdH7xc+7xr2l+nW1mcoACoR9uaX0T1+NDkCm4fUig5WZW7XQ08YYL9Vg/l325CQjI\r
  31         fx6K3wJ0gGmbm59svbdzEm+zlH9VawSG/P3dcND8sAQ8E98e6D55lnHHmG51m5Ud0XGq\r
  32         PpRrHimlk3VAP8QCV0E1b+i7et/Mh/SnGYA/i4AY70QCqoXgyj+bqMPUnS5JjUfmPqjl\r
  33         yrw18vSxSFnFCVSQoZ8a0gqZiwKFJ8rwPVC6BVJyhO3N7UcD3CkOR3weja3j1GWxzWWb\r
  34         fO02VrTXSYOMmYFL1RqawTKB1XiRkw+1lijN118KLQljeEvgaEDtqgMNYofFGqoiMbDA\r
  35         tlLQ==\r
  36 Received: by 10.52.27.82 with SMTP id r18mr1818894vdg.120.1350664592262;\r
  37         Fri, 19 Oct 2012 09:36:32 -0700 (PDT)\r
  38 Received: from smtp.gmail.com (p70-80.acedsl.com. [66.114.70.80])\r
  39         by mx.google.com with ESMTPS id w17sm1921449vdf.16.2012.10.19.09.36.30\r
  40         (version=TLSv1/SSLv3 cipher=OTHER);\r
  41         Fri, 19 Oct 2012 09:36:30 -0700 (PDT)\r
  42 From: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>\r
  43 To: Mark Walters <markwalters1009@gmail.com>,\r
  44         Jameson Graef Rollins <jrollins@finestructure.net>,\r
  45         Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org\r
  46 Subject: Re: [PATCH v2] emacs: add function to toggle display of all\r
  47         multipart/alternative parts\r
  48 In-Reply-To: <87r4rf9q8x.fsf@qmul.ac.uk>\r
  49 References: <1339775751-18002-1-git-send-email-jani@nikula.org>\r
  50         <87zk80gvmb.fsf@servo.finestructure.net>\r
  51         <87pq8vokmp.fsf@qmul.ac.uk>\r
  52         <87ipcrvlcw.fsf@servo.finestructure.net>\r
  53         <87r4rf9q8x.fsf@qmul.ac.uk>\r
  54 User-Agent: Notmuch/0.14+45~g6ea9330 (http://notmuchmail.org) Emacs/23.3.1\r
  55         (x86_64-pc-linux-gnu)\r
  56 Date: Fri, 19 Oct 2012 12:36:26 -0400\r
  57 Message-ID: <87pq4e771x.fsf@betacantrips.com>\r
  58 MIME-Version: 1.0\r
  59 Content-Type: text/plain; charset=us-ascii\r
  60 X-BeenThere: notmuch@notmuchmail.org\r
  61 X-Mailman-Version: 2.1.13\r
  62 Precedence: list\r
  63 List-Id: "Use and development of the notmuch mail system."\r
  64         <notmuch.notmuchmail.org>\r
  65 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
  66         <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  67 List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
  68 List-Post: <mailto:notmuch@notmuchmail.org>\r
  69 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  70 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
  71         <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  72 X-List-Received-Date: Fri, 19 Oct 2012 16:36:33 -0000\r
  73 \r
  74 Mark Walters <markwalters1009@gmail.com> writes:\r
  75 \r
  76 > Some messages are sent as multipart/alternative but the alternatives\r
  77 > contain different information. This allows the user to cycle which\r
  78 > part to view. By default this is bound to 'W'.\r
  79 > ---\r
  80 >\r
  81 > This version at least uses the notmuch escaping for message-id which\r
  82 > makes me a bit happier: it probably doesn't have any nasty security\r
  83 > flaws. I do still feel that the lisp is a bit ugly though.\r
  84 \r
  85 For what it's worth, I don't feel that this code is horrible. It seems\r
  86 like there remain design decisions to be made about how notmuch show\r
  87 "ought" to handle multipart/alternatives, but I can at least comment on\r
  88 this code.\r
  89 \r
  90 First, the use of a plist looks fine to me because most of the time it's\r
  91 going to have length 0. At most it will have one entry per message -- a\r
  92 few hundred. So I'm not worried about efficiency concerns.\r
  93 \r
  94 >  (defcustom notmuch-show-stash-mlarchive-link-alist\r
  95 >    '(("Gmane" . "http://mid.gmane.org/")\r
  96 >      ("MARC" . "http://marc.info/?i=")\r
  97 > @@ -536,9 +540,19 @@ message at DEPTH in the current thread."\r
  98 >  \r
  99 >  (defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type)\r
 100 >    (notmuch-show-insert-part-header nth declared-type content-type nil)\r
 101 > -  (let ((chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part))))\r
 102 > -     (inner-parts (plist-get part :content))\r
 103 > -     (start (point)))\r
 104 > +  (let* ((chosen-nth (or (lax-plist-get notmuch-show-message-multipart/alternative-display-part\r
 105 > +                                     (notmuch-id-to-query (plist-get msg :id))) 0))\r
 106 > +      (chosen-type (nth chosen-nth\r
 107 > +                       (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part))))\r
 108 > +      (inner-parts (plist-get part :content))\r
 109 > +      (start (point)))\r
 110 \r
 111 Changing let to let* makes me the slightest bit uneasy, although I'm not\r
 112 sure I could explain why.\r
 113 \r
 114 It would be nice if you could wrap the manipulation of\r
 115 notmuch-show-message-multipart/alternative-display-part in functions,\r
 116 ideally with names that are shorter than the variable they\r
 117 manipulate. Specifically, I think the definition of chosen-nth (which is\r
 118 almost repeated below) could be its own function, something like\r
 119 (notmuch-show-message-current-multipart msg), which could take a msg-id\r
 120 or a plist and do the plist-get and id-to-query that you do here.\r
 121 \r
 122 > +    ;; If we have run out of possible content-types restart from the beginning\r
 123 > +    (unless chosen-type\r
 124 > +      (setq chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part))))\r
 125 > +      (setq notmuch-show-message-multipart/alternative-display-part\r
 126 > +         (lax-plist-put notmuch-show-message-multipart/alternative-display-part\r
 127 > +                        (notmuch-id-to-query (plist-get msg :id)) 0)))\r
 128 > +\r
 129 >      ;; This inserts all parts of the chosen type rather than just one,\r
 130 >      ;; but it's not clear that this is the wrong thing to do - which\r
 131 >      ;; should be chosen if there are more than one that match?\r
 132 > @@ -942,6 +956,16 @@ message at DEPTH in the current thread."\r
 133 >            "Not processing cryptographic MIME parts."))\r
 134 >    (notmuch-show-refresh-view))\r
 135 >  \r
 136 > +(defun notmuch-show-cycle-message-multipart ()\r
 137 > +  "Cycle which part to display of a multipart messageToggle the display of non-matching messages."\r
 138 \r
 139 This docstring is broken.\r
 140 \r
 141 > +  (interactive)\r
 142 > +  (let* ((msg-id (notmuch-show-get-message-id))\r
 143 > +      (next-part (1+ (or (lax-plist-get notmuch-show-message-multipart/alternative-display-part msg-id) 0))))\r
 144 > +    (setq notmuch-show-message-multipart/alternative-display-part\r
 145 > +     (lax-plist-put notmuch-show-message-multipart/alternative-display-part msg-id next-part))\r
 146 > +    (message "Cycling multipart/alternative for current message")\r
 147 > +    (notmuch-show-refresh-view)))\r
 148 \r
 149 Maybe move the reset-and-go-back-to-zero behavior to this function\r
 150 instead of in the display function. This opens you up to weird\r
 151 situations if one of the multipart/alternatives should disappear from a\r
 152 message or if some other function should change the alternative on\r
 153 display for a given message, but both of these seem unlikely to me..\r
 154 \r
 155 Ethan\r