Re: notmuch-emacs replying produces excessive indenting on multipart/signed or multip...

[notmuch-archives.git] / 1c / c71cd9fc3cce1f33496da1fa8cf818d81a43cc

Return-Path: <m.walters@qmul.ac.uk>\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 54A39431FBC\r
        for <notmuch@notmuchmail.org>; Thu, 13 Dec 2012 00:54:28 -0800 (PST)\r
X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
X-Spam-Flag: NO\r
X-Spam-Score: -1.098\r
X-Spam-Level: \r
X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5\r
        tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,\r
        NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] 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 GIl7sN6Oaec2 for <notmuch@notmuchmail.org>;\r
        Thu, 13 Dec 2012 00:54:26 -0800 (PST)\r
Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])\r
        (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\r
        (No client certificate requested)\r
        by olra.theworths.org (Postfix) with ESMTPS id DE126431FB6\r
        for <notmuch@notmuchmail.org>; Thu, 13 Dec 2012 00:54:25 -0800 (PST)\r
Received: from smtp.qmul.ac.uk ([138.37.6.40])\r
        by mail2.qmul.ac.uk with esmtp (Exim 4.71)\r
        (envelope-from <m.walters@qmul.ac.uk>)\r
        id 1Tj4Yf-00083d-Bi; Thu, 13 Dec 2012 08:54:21 +0000\r
Received: from 93-97-24-31.zone5.bethere.co.uk ([93.97.24.31] helo=localhost)\r
        by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69)\r
        (envelope-from <m.walters@qmul.ac.uk>)\r
        id 1Tj4Ye-0000j4-PC; Thu, 13 Dec 2012 08:54:21 +0000\r
From: Mark Walters <markwalters1009@gmail.com>\r
To: Austin Clements <aclements@csail.mit.edu>, notmuch@notmuchmail.org\r
Subject: Re: [PATCH 2/3] emacs: show: add overlays for each part\r
In-Reply-To: <87txrtnssb.fsf@awakening.csail.mit.edu>\r
References: <1354663662-20524-1-git-send-email-markwalters1009@gmail.com>\r
        <1354663662-20524-3-git-send-email-markwalters1009@gmail.com>\r
        <87txrtnssb.fsf@awakening.csail.mit.edu>\r
User-Agent: Notmuch/0.14+155~g7edfdc3 (http://notmuchmail.org) Emacs/23.4.1\r
        (x86_64-pc-linux-gnu)\r
Date: Thu, 13 Dec 2012 08:54:22 +0000\r
Message-ID: <87obhy72o1.fsf@qmul.ac.uk>\r
MIME-Version: 1.0\r
Content-Type: text/plain; charset=us-ascii\r
X-Sender-Host-Address: 93.97.24.31\r
X-QM-SPAM-Info: Sender has good ham record.  :)\r
X-QM-Body-MD5: 187b337f0f41c1dc60bb4f5aa986515d (of first 20000 bytes)\r
X-SpamAssassin-Score: -1.8\r
X-SpamAssassin-SpamBar: -\r
X-SpamAssassin-Report: The QM spam filters have analysed this message to\r
        determine if it is\r
        spam. We require at least 5.0 points to mark a message as spam.\r
        This message scored -1.8 points.\r
        Summary of the scoring: \r
        * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,\r
        *      medium trust\r
        *      [138.37.6.40 listed in list.dnswl.org]\r
        * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail\r
        provider *      (markwalters1009[at]gmail.com)\r
        *  0.5 AWL AWL: From: address is in the auto white-list\r
X-QM-Scan-Virus: ClamAV says the message is clean\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: Thu, 13 Dec 2012 08:54:28 -0000\r
\r
\r
Hi \r
\r
Many thanks for the review: I will post a new version shortly. Some\r
comments inline below:\r
\r
On Tue, 11 Dec 2012, Austin Clements <aclements@csail.mit.edu> wrote:\r
> On Tue, 04 Dec 2012, Mark Walters <markwalters1009@gmail.com> wrote:\r
>> This make notmuch-show-insert-bodypart add an overlay for any\r
>\r
> s/make/makes/\r
>\r
>> non-trivial part with a button header (currently the first text/plain\r
>> part does not have a button). At this point the overlay is available\r
>> to the button but there is no action using it yet.\r
>>\r
>> In addition a not-shown variable which is used to request the part be\r
>\r
> not-shown is really an argument (I found this confusing).\r
>\r
>> hidden by default down to the overlay but this is not acted on yet.\r
>> ---\r
>>  emacs/notmuch-show.el |   62 +++++++++++++++++++++++++++++++++++++-----------\r
>>  1 files changed, 48 insertions(+), 14 deletions(-)\r
>>\r
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el\r
>> index f8ce037..3215ebc 100644\r
>> --- a/emacs/notmuch-show.el\r
>> +++ b/emacs/notmuch-show.el\r
>> @@ -569,10 +569,9 @@ message at DEPTH in the current thread."\r
>>      ;; should be chosen if there are more than one that match?\r
>>      (mapc (lambda (inner-part)\r
>>          (let ((inner-type (plist-get inner-part :content-type)))\r
>> -          (if (or notmuch-show-all-multipart/alternative-parts\r
>> -                  (string= chosen-type inner-type))\r
>> -              (notmuch-show-insert-bodypart msg inner-part depth)\r
>> -            (notmuch-show-insert-part-header (plist-get inner-part :id) inner-type inner-type nil " (not shown)"))))\r
>> +          (notmuch-show-insert-bodypart msg inner-part depth\r
>> +                                        (not (or notmuch-show-all-multipart/alternative-parts\r
>\r
> Since notmuch-show-all-multipart/alternative-parts was basically a hack\r
> around our poor multipart/alternative support, I think this series (or a\r
> follow up patch) should change its default to nil or even eliminate it\r
> entirely.\r
\r
I have added a patch at the end setting this to nil by default.\r
\r
>> +                                                 (string= chosen-type inner-type))))))\r
>\r
> You could let-bind the (not (or ..)) to some variable ("hide" perhaps)\r
> in the let above to avoid this crazy line length.\r
>\r
>>        inner-parts)\r
>>  \r
>>      (when notmuch-show-indent-multipart\r
>> @@ -840,17 +839,52 @@ message at DEPTH in the current thread."\r
>>        (setq handlers (cdr handlers))))\r
>>    t)\r
>>  \r
>> -(defun notmuch-show-insert-bodypart (msg part depth)\r
>> -  "Insert the body part PART at depth DEPTH in the current thread."\r
>> +(defun notmuch-show-insert-part-overlays (msg beg end not-shown)\r
>\r
> s/not-shown/hide/?  Or hidden?\r
>\r
>> +  "Add an overlay to the part between BEG and END"\r
>> +  (let* ((button (button-at beg))\r
>> +     (part-beg (and button (1+ (button-end button)))))\r
>> +\r
>> +    ;; If the part contains no text we do not make it toggleable.\r
>> +    (unless (or (not button) (eq part-beg end))\r
>\r
> (when (and button (/= part-beg end)) ...) ?\r
>\r
>> +      (let ((base-label (button-get button :base-label))\r
>> +        (overlay (make-overlay part-beg end))\r
>> +        (message-invis-spec (plist-get msg :message-invis-spec))\r
>> +        (invis-spec (make-symbol "notmuch-part-region")))\r
>> +\r
>> +    (overlay-put overlay 'invisible (list invis-spec message-invis-spec))\r
>\r
> Non-trivial buffer invisibility specs are really bad for performance\r
> (Emacs' renderer does the obvious O(n^2) thing when rendering a buffer\r
> with an invisibility spec).  Unfortunately, because of notmuch-wash and\r
> the way overlays with trivial 'invisible properties combine with\r
> overlays with list-type 'invisible properties combine, I don't think it\r
> can be avoided.  But if we get rid of buffer invisibility specs from\r
> notmuch-wash, this code can also get much simpler.\r
\r
How do you plan to get rid of the invisibility properties from notmuch\r
wash? \r
\r
>> +    (overlay-put overlay 'isearch-open-invisible #'notmuch-wash-region-isearch-show)\r
>\r
> This will leave the "(not shown)" in the part header if isearch unfolds\r
> the part.\r
>\r
> Do we even want isearch to unfold parts?  It's not clear we do for\r
> multipart/alternative.  If we do, probably the right thing is something\r
> like\r
\r
I don't think we want to search hidden bodyparts so I just deleted this\r
line.\r
\r
>\r
> (overlay-put overlay 'notmuch-show-part-button button)\r
> (overlay-put overlay 'isearch-open-invisible #'notmuch-show-part-isearch-open)\r
>\r
> (defun notmuch-show-part-isearch-open (overlay)\r
>   (notmuch-show-toggle-invisible-part-action\r
>    (overlay-get overlay 'notmuch-show-part-button)))\r
>\r
>> +    (overlay-put overlay 'priority 10)\r
>> +    (overlay-put overlay 'type "part")\r
>> +    ;; Now we have to add invis-spec to every overlay this\r
>> +    ;; overlay contains, otherwise these inner overlays will\r
>> +    ;; override this one.\r
>\r
> Interesting.  In the simple case of using nil or t for 'invisible, the\r
> specs do combine as one would expect, but you're right that, with a\r
> non-trivial invisibility-spec, the highest priority overlay wins.  It's\r
> too bad we don't know the depth of the part or we could just set the\r
> overlay priority.  This is another thing that would go away if we didn't\r
> use buffer invisibility-specs.\r
\r
I don't think priorities get it right. As far as I could tell if you\r
have two overlays at different priorities both with invisibility\r
properties then the higher priority overlay decides visibility by\r
itself: that is if it says invisible then the text is invisible, and if\r
it says visible then the text is visible.\r
\r
>\r
>> +    (mapc (lambda (inner)\r
>\r
> I would use a (dolist (inner (overlays-in part-beg end)) ...) here.\r
> Seems a little more readable.\r
>\r
>> +            (when (and (>= (overlay-start inner) part-beg)\r
>> +                       (<= (overlay-end inner) end))\r
>> +              (overlay-put inner 'invisible\r
>> +                           (cons invis-spec (overlay-get inner 'invisible)))))\r
>> +          (overlays-in part-beg end))\r
>> +\r
>> +    (button-put button 'invisibility-spec invis-spec)\r
>> +    (button-put button 'overlay overlay))\r
>> +      (goto-char (point-max)))))\r
>\r
> This goto-char seems oddly out of place, since it has nothing to do with\r
> overlay creation.  Was it supposed to be here instead of in\r
> notmuch-show-insert-bodypart?  Is it even necessary?\r
\r
It was suppose to be in notmuch-show-insert-body-part. It was necessary\r
but I have wrapped the toggle-button call in the third patch and now it\r
does not seem necessary.\r
\r
It is possible that the toggle-button function should not move point: if\r
that got changed the save-excursion here could be removed. (I will\r
discuss this in my reply to your comments on the next patch).\r
\r
Best wishes \r
\r
Mark\r
\r
>\r
>> +\r
>> +(defun notmuch-show-insert-bodypart (msg part depth &optional not-shown)\r
>\r
> Same comment about not-shown.  (Also in the commit message.)\r
>\r
>> +  "Insert the body part PART at depth DEPTH in the current thread.\r
>> +\r
>> +If not-shown is TRUE then initially hide this part."\r
>\r
> s/not-shown/NOT-SHOWN/ (or whatever) and s/TRUE/non-nil/\r
>\r
>>    (let ((content-type (downcase (plist-get part :content-type)))\r
>> -    (nth (plist-get part :id)))\r
>> -    (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type))\r
>> -  ;; Some of the body part handlers leave point somewhere up in the\r
>> -  ;; part, so we make sure that we're down at the end.\r
>> -  (goto-char (point-max))\r
>> -  ;; Ensure that the part ends with a carriage return.\r
>> -  (unless (bolp)\r
>> -    (insert "\n")))\r
>> +    (nth (plist-get part :id))\r
>> +    (beg (point)))\r
>> +\r
>> +    (notmuch-show-insert-bodypart-internal msg part content-type nth depth content-type)\r
>> +    ;; Some of the body part handlers leave point somewhere up in the\r
>> +    ;; part, so we make sure that we're down at the end.\r
>> +    (goto-char (point-max))\r
>> +    ;; Ensure that the part ends with a carriage return.\r
>> +    (unless (bolp)\r
>> +      (insert "\n"))\r
>> +    (notmuch-show-insert-part-overlays msg beg (point) not-shown)))\r
>>  \r
>>  (defun notmuch-show-insert-body (msg body depth)\r
>>    "Insert the body BODY at depth DEPTH in the current thread."\r
>> -- \r
>> 1.7.9.1\r