ec/97b6db36af9aa7a40217da9a78e959ac0e55a0

   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 695CB431FB6\r
   6         for <notmuch@notmuchmail.org>; Sat, 27 Oct 2012 17:08:58 -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 9EcVQPzK08vB for <notmuch@notmuchmail.org>;\r
  18         Sat, 27 Oct 2012 17:08:54 -0700 (PDT)\r
  19 Received: from mail-qc0-f181.google.com (mail-qc0-f181.google.com\r
  20         [209.85.216.181]) (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 E9DA0431FAF\r
  23         for <notmuch@notmuchmail.org>; Sat, 27 Oct 2012 17:08:53 -0700 (PDT)\r
  24 Received: by mail-qc0-f181.google.com with SMTP id x40so2310975qcp.26\r
  25         for <notmuch@notmuchmail.org>; Sat, 27 Oct 2012 17:08:52 -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=B8rkjqe5YyFWY0Ty/OOTXjyJoY5CcBSoeMjgnw3CMjc=;\r
  30         b=HXor86Nje+zDMilqk+GJwQ+D5U6445DCh164scrG3NpDQn3MsW1hOzOtDPWui8n3mY\r
  31         ZXB54Ha1WxcHlgJJI4HDLMCfwAWkux/XgCqMuCAe6Yc9/XZe+yk1MKh0A0etyUTR07Kb\r
  32         JPlZN+LPpFe/Ak1kFMwu3hiKsm9LI8TRRzddU1pHwMzRGJclZNOYQtr1joT5qJUHY9fq\r
  33         Ujg8BJHHkvJRoKHNqIbqU8Nm3a5/jcyW81WVK6Qs++AnR0YWn8vaHRw3zXBKyqtP3MTb\r
  34         AEorxyVybRioCSFZ8mHoSenl4ESriEX2Sl3t+QmRScxCBM+bPQv6JImM0RUr6yOfz58C\r
  35         e7yg==\r
  36 Received: by 10.224.194.193 with SMTP id dz1mr14727965qab.0.1351382932078;\r
  37         Sat, 27 Oct 2012 17:08:52 -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 bc9sm3485926qab.2.2012.10.27.17.08.50\r
  40         (version=TLSv1/SSLv3 cipher=OTHER);\r
  41         Sat, 27 Oct 2012 17:08:51 -0700 (PDT)\r
  42 From: Ethan Glasser-Camp <ethan.glasser.camp@gmail.com>\r
  43 To: Mark Walters <markwalters1009@gmail.com>, notmuch@notmuchmail.org\r
  44 Subject: Re: [PATCH (draft) 1/2] emacs: allow the user to toggle the\r
  45         visibility of multipart/alternative parts\r
  46 In-Reply-To: <1351152563-27277-2-git-send-email-markwalters1009@gmail.com>\r
  47 References: <1351152563-27277-1-git-send-email-markwalters1009@gmail.com>\r
  48         <1351152563-27277-2-git-send-email-markwalters1009@gmail.com>\r
  49 User-Agent: Notmuch/0.14+45~g6ea9330 (http://notmuchmail.org) Emacs/23.4.1\r
  50         (x86_64-pc-linux-gnu)\r
  51 Date: Sat, 27 Oct 2012 20:08:49 -0400\r
  52 Message-ID: <87sj8zh2zy.fsf@betacantrips.com>\r
  53 MIME-Version: 1.0\r
  54 Content-Type: text/plain; charset=us-ascii\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: Sun, 28 Oct 2012 00:08:58 -0000\r
  68 \r
  69 Mark Walters <markwalters1009@gmail.com> writes:\r
  70 \r
  71 > This patch adds a keybinding to the buttons in the notmuch-show emacs\r
  72 > buffer to allow the user to toggle the visibility of each part of a\r
  73 > message in the show buffer.  This is particularly useful for\r
  74 > multipart/alternative parts where the parts are not really\r
  75 > alternatives but contain different information.\r
  76 > ---\r
  77 >  emacs/notmuch-show.el |   47 +++++++++++++++++++++++++++++++++++++++--------\r
  78 >  1 files changed, 39 insertions(+), 8 deletions(-)\r
  79 >\r
  80 > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el\r
  81 > index 0f54259..9157669 100644\r
  82 > --- a/emacs/notmuch-show.el\r
  83 > +++ b/emacs/notmuch-show.el\r
  84 > @@ -155,6 +155,10 @@ indentation."\r
  85 >  (make-variable-buffer-local 'notmuch-show-indent-content)\r
  86 >  (put 'notmuch-show-indent-content 'permanent-local t)\r
  87 >\r
  88 > +(defvar notmuch-show-message-multipart/alternative-display-parts nil)\r
  89 > +(make-variable-buffer-local 'notmuch-show-message-multipart/alternative-display-parts)\r
  90 > +(put 'notmuch-show-message-multipart/alternative-display-parts 'permanent-local t)\r
  91 > +\r
  92 >  (defcustom notmuch-show-stash-mlarchive-link-alist\r
  93 >    '(("Gmane" . "http://mid.gmane.org/")\r
  94 >      ("MARC" . "http://marc.info/?i=")\r
  95 > @@ -455,6 +459,7 @@ message at DEPTH in the current thread."\r
  96 >      (define-key map "v" 'notmuch-show-part-button-view)\r
  97 >      (define-key map "o" 'notmuch-show-part-button-interactively-view)\r
  98 >      (define-key map "|" 'notmuch-show-part-button-pipe)\r
  99 > +    (define-key map "t" 'notmuch-show-part-button-internally-show)\r
 100 >      map)\r
 101 >    "Submap for button commands")\r
 102 >  (fset 'notmuch-show-part-button-map notmuch-show-part-button-map)\r
 103 > @@ -531,6 +536,16 @@ message at DEPTH in the current thread."\r
 104 >      (let ((handle (mm-make-handle (current-buffer) (list content-type))))\r
 105 >        (mm-pipe-part handle))))\r
 106 >\r
 107 > +(defun notmuch-show-internally-show-part (message-id nth &optional filename content-type)\r
 108 > +  "Set a part to be displayed internally"\r
 109 > +  (let ((current-parts (lax-plist-get notmuch-show-message-multipart/alternative-display-parts message-id)))\r
 110 > +    (setq notmuch-show-message-multipart/alternative-display-parts\r
 111 > +       (lax-plist-put notmuch-show-message-multipart/alternative-display-parts message-id\r
 112 > +                      (if (memq nth current-parts)\r
 113 > +                          (delq nth current-parts)\r
 114 > +                        (cons nth current-parts)))))\r
 115 > +  (notmuch-show-refresh-view))\r
 116 > +\r
 117 >  (defun notmuch-show-multipart/*-to-list (part)\r
 118 >    (mapcar (lambda (inner-part) (plist-get inner-part :content-type))\r
 119 >         (plist-get part :content)))\r
 120 > @@ -543,12 +558,15 @@ message at DEPTH in the current thread."\r
 121 >      ;; This inserts all parts of the chosen type rather than just one,\r
 122 >      ;; but it's not clear that this is the wrong thing to do - which\r
 123 >      ;; should be chosen if there are more than one that match?\r
 124 > +\r
 125 > +    ;; The variable user-parts says which parts should override the\r
 126 > +    ;; default so we use xor (handcoded since lisp does not have it).\r
 127 \r
 128 I don't follow the comment. user-parts isn't used in this\r
 129 function. Neither is xor.\r
 130 \r
 131 >      (mapc (lambda (inner-part)\r
 132 >           (let ((inner-type (plist-get inner-part :content-type)))\r
 133 > -           (if (or notmuch-show-all-multipart/alternative-parts\r
 134 > -                   (string= chosen-type inner-type))\r
 135 > -               (notmuch-show-insert-bodypart msg inner-part depth)\r
 136 > -             (notmuch-show-insert-part-header (plist-get inner-part :id) inner-type inner-type nil " (not shown)"))))\r
 137 > +           (notmuch-show-insert-bodypart msg inner-part depth\r
 138 > +                                         (not (or notmuch-show-all-multipart/alternative-parts\r
 139 > +                                                  (string=\r
 140 >           chosen-type inner-type))))))\r
 141 \r
 142 For what it's worth, I found this not-shown logic very confusing, and\r
 143 have had to think about it seven or eight different times to make sure I\r
 144 understood what's going on. I'm not sure why exactly this is, though I\r
 145 could offer hypotheses -- the fact that it's split across two functions,\r
 146 or the fiddling with mime-types. I'm satisfied that it's correct, but I\r
 147 wish it could be made clearer.\r
 148 \r
 149 This is just armchair hypothesizing, but here are some ideas that might\r
 150 make it more obvious what's going on: bringing the user-parts logic into\r
 151 this function; making user-parts, instead of a "t" meaning "user has\r
 152 toggled this", something like 'opened or 'closed and if user-parts for\r
 153 this message is absent, falling back to this calculation; alternately,\r
 154 prefilling user-parts with t when show is invoked, according to this\r
 155 calculation, and then not using it any more; moving this\r
 156 not-shown calculation into a separate function, something like notmuch-show-get-message-visibility.\r
 157 \r
 158 I guess I jumped into this series halfway, but why are we doing this\r
 159 with the wipe/redraw technique instead of just using invisible overlays,\r
 160 like we do more generally with notmuch-show? I think I agree that\r
 161 toggling individual parts is a good UI approach, and this isn't a bad\r
 162 way to implement it, but I wonder if we could do it better/easier if we\r
 163 used emacs's builtin functionality.\r
 164 \r
 165 Thanks!\r
 166 \r
 167 Ethan\r