From: Austin Clements Date: Thu, 30 May 2013 22:30:59 +0000 (+2000) Subject: Re: [PATCH v2 2/3] emacs: show: move the insertion of the header button to the top... X-Git-Url: http://git.tremily.us/?a=commitdiff_plain;h=95169133372f413e88c351137492cb5fff3de9cd;p=notmuch-archives.git Re: [PATCH v2 2/3] emacs: show: move the insertion of the header button to the top level --- diff --git a/70/74bd09d91624f7cc5c96b1a0d61de665c4c1a5 b/70/74bd09d91624f7cc5c96b1a0d61de665c4c1a5 new file mode 100644 index 000000000..1fabeb995 --- /dev/null +++ b/70/74bd09d91624f7cc5c96b1a0d61de665c4c1a5 @@ -0,0 +1,320 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 2CB00431FC3 + for ; Thu, 30 May 2013 15:31:15 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id 5hFv4qRtbRoU for ; + Thu, 30 May 2013 15:31:09 -0700 (PDT) +Received: from dmz-mailsec-scanner-4.mit.edu (dmz-mailsec-scanner-4.mit.edu + [18.9.25.15]) + by olra.theworths.org (Postfix) with ESMTP id 3CE8A431FD2 + for ; Thu, 30 May 2013 15:31:09 -0700 (PDT) +X-AuditID: 1209190f-b7f256d000005616-b2-51a7d327d69c +Received: from mailhub-auth-2.mit.edu ( [18.7.62.36]) + by dmz-mailsec-scanner-4.mit.edu (Symantec Messaging Gateway) with SMTP + id 04.FA.22038.723D7A15; Thu, 30 May 2013 18:31:03 -0400 (EDT) +Received: from outgoing.mit.edu (outgoing-auth-1.mit.edu [18.9.28.11]) + by mailhub-auth-2.mit.edu (8.13.8/8.9.2) with ESMTP id r4UMV2Ck019853; + Thu, 30 May 2013 18:31:03 -0400 +Received: from awakening.csail.mit.edu (awakening.csail.mit.edu [18.26.4.91]) + (authenticated bits=0) + (User authenticated as amdragon@ATHENA.MIT.EDU) + by outgoing.mit.edu (8.13.8/8.12.4) with ESMTP id r4UMUxNt012623 + (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); + Thu, 30 May 2013 18:31:01 -0400 +Received: from amthrax by awakening.csail.mit.edu with local (Exim 4.80) + (envelope-from ) + id 1UiBN5-0005tg-O0; Thu, 30 May 2013 18:30:59 -0400 +From: Austin Clements +To: Mark Walters , notmuch@notmuchmail.org +Subject: Re: [PATCH v2 2/3] emacs: show: move the insertion of the header + button to the top level +In-Reply-To: <1369555061-21361-3-git-send-email-markwalters1009@gmail.com> +References: <1369555061-21361-1-git-send-email-markwalters1009@gmail.com> + <1369555061-21361-3-git-send-email-markwalters1009@gmail.com> +User-Agent: Notmuch/0.15.2+83~g8bee3c4 (http://notmuchmail.org) Emacs/23.4.1 + (i486-pc-linux-gnu) +Date: Thu, 30 May 2013 18:30:59 -0400 +Message-ID: <87a9nc2j8c.fsf@awakening.csail.mit.edu> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Brightmail-Tracker: + H4sIAAAAAAAAA+NgFnrNIsWRmVeSWpSXmKPExsUixG6noqt+eXmgwZrDKhar5/JYXL85k9mB + yWPnrLvsHs9W3WIOYIrisklJzcksSy3St0vgyji0egN7warUiiM/L7I0MD4P6mLk5JAQMJE4 + MHcxM4QtJnHh3nq2LkYuDiGBfYwSL35+YYJwNjJKLGu6zwrhnGaSOHLsCDtIi5DAEkaJ1dfS + QGw2AX2JFWsnsYLYIgKuEk+/fQYbKyyQKPFxyV+wek4BL4lPx6+wQPS2M0q873UAsUUFEiRW + 3j3BCGKzCKhK7H8wDWgOBwcv0Hkb11mDhHkFBCVOznwC1sosoCVx499LpgmMArOQpGYhSS1g + ZFrFKJuSW6Wbm5iZU5yarFucnJiXl1qka6KXm1mil5pSuokRFI6ckvw7GL8dVDrEKMDBqMTD + m5G0PFCINbGsuDL3EKMkB5OSKO/PU0AhvqT8lMqMxOKM+KLSnNTiQ4wSHMxKIryd54ByvCmJ + lVWpRfkwKWkOFiVx3qspN/2FBNITS1KzU1MLUotgsjIcHEoSvPMuAjUKFqWmp1akZeaUIKSZ + ODhBhvMADT8DUsNbXJCYW5yZDpE/xajLsfn85HeMQix5+XmpUuK8h0CKBECKMkrz4ObA0sgr + RnGgt4R5l4BU8QBTENykV0BLmICWPLEGW1KSiJCSamBUv+hfonnivN/LrKjqlYZbMup/n7gS + +WzLRj1vlvUi+u75q244F8TlSYbPrEk1t1tiz2a1R81/9lz7k1ODV3/4uUTpywHvuin6tToW + ezNe28krz1Hg+anLO+PN9EiG5pYF9ziiDrFYZ6ybnqghozu1Q+rA9D2qB1a9+H2aI/E5/7vc + l+Zzr+9QYinOSDTUYi4qTgQAYrJMrf4CAAA= +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Thu, 30 May 2013 22:31:15 -0000 + +On Sun, 26 May 2013, Mark Walters wrote: +> Previously each of the part insertion handlers inserted the part +> button themselves. Move this up into +> notmuch-show-insert-bodypart. Since a small number of the handlers +> modify the button (the encryption/signature ones) we need to pass the +> header button as an argument into the individual part insertion +> handlers. +> +> The patch is large but mostly simple. The only things of note are that +> we let the text/plain handler insert part buttons itself (as it does +> not always insert one and it applies motmuch-wash to the whole part +> including the button. Also this is by far the most important case so +> this reduces the risk of annoying side effects. +> --- +> emacs/notmuch-show.el | 87 +++++++++++++++++++++++------------------------- +> 1 files changed, 42 insertions(+), 45 deletions(-) +> +> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el +> index 51bda31..591ad56 100644 +> --- a/emacs/notmuch-show.el +> +++ b/emacs/notmuch-show.el +> @@ -576,8 +576,7 @@ message at DEPTH in the current thread." +> (mapcar (lambda (inner-part) (plist-get inner-part :content-type)) +> (plist-get part :content))) +> +> -(defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type) +> - (notmuch-show-insert-part-header nth declared-type content-type nil) +> +(defun notmuch-show-insert-part-multipart/alternative (msg part content-type nth depth declared-type button) +> (let ((chosen-type (car (notmuch-multipart/alternative-choose (notmuch-show-multipart/*-to-list part)))) +> (inner-parts (plist-get part :content)) +> (start (point))) +> @@ -654,8 +653,7 @@ message at DEPTH in the current thread." +> content-type) +> nil))) +> +> -(defun notmuch-show-insert-part-multipart/related (msg part content-type nth depth declared-type) +> - (notmuch-show-insert-part-header nth declared-type content-type nil) +> +(defun notmuch-show-insert-part-multipart/related (msg part content-type nth depth declared-type button) +> (let ((inner-parts (plist-get part :content)) +> (start (point))) +> +> @@ -674,16 +672,15 @@ message at DEPTH in the current thread." +> (indent-rigidly start (point) 1))) +> t) +> +> -(defun notmuch-show-insert-part-multipart/signed (msg part content-type nth depth declared-type) +> - (let ((button (notmuch-show-insert-part-header nth declared-type content-type nil))) +> - (button-put button 'face 'notmuch-crypto-part-header) +> - ;; add signature status button if sigstatus provided +> - (if (plist-member part :sigstatus) +> - (let* ((from (notmuch-show-get-header :From msg)) +> - (sigstatus (car (plist-get part :sigstatus)))) +> - (notmuch-crypto-insert-sigstatus-button sigstatus from)) +> - ;; if we're not adding sigstatus, tell the user how they can get it +> - (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts."))) +> +(defun notmuch-show-insert-part-multipart/signed (msg part content-type nth depth declared-type button) +> + (button-put button 'face 'notmuch-crypto-part-header) +> + ;; add signature status button if sigstatus provided +> + (if (plist-member part :sigstatus) +> + (let* ((from (notmuch-show-get-header :From msg)) +> + (sigstatus (car (plist-get part :sigstatus)))) +> + (notmuch-crypto-insert-sigstatus-button sigstatus from)) +> + ;; if we're not adding sigstatus, tell the user how they can get it +> + (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts.")) +> +> (let ((inner-parts (plist-get part :content)) +> (start (point))) +> @@ -696,20 +693,19 @@ message at DEPTH in the current thread." +> (indent-rigidly start (point) 1))) +> t) +> +> -(defun notmuch-show-insert-part-multipart/encrypted (msg part content-type nth depth declared-type) +> - (let ((button (notmuch-show-insert-part-header nth declared-type content-type nil))) +> - (button-put button 'face 'notmuch-crypto-part-header) +> - ;; add encryption status button if encstatus specified +> - (if (plist-member part :encstatus) +> - (let ((encstatus (car (plist-get part :encstatus)))) +> - (notmuch-crypto-insert-encstatus-button encstatus) +> - ;; add signature status button if sigstatus specified +> - (if (plist-member part :sigstatus) +> - (let* ((from (notmuch-show-get-header :From msg)) +> - (sigstatus (car (plist-get part :sigstatus)))) +> - (notmuch-crypto-insert-sigstatus-button sigstatus from)))) +> - ;; if we're not adding encstatus, tell the user how they can get it +> - (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts."))) +> +(defun notmuch-show-insert-part-multipart/encrypted (msg part content-type nth depth declared-type button) +> + (button-put button 'face 'notmuch-crypto-part-header) +> + ;; add encryption status button if encstatus specified +> + (if (plist-member part :encstatus) +> + (let ((encstatus (car (plist-get part :encstatus)))) +> + (notmuch-crypto-insert-encstatus-button encstatus) +> + ;; add signature status button if sigstatus specified +> + (if (plist-member part :sigstatus) +> + (let* ((from (notmuch-show-get-header :From msg)) +> + (sigstatus (car (plist-get part :sigstatus)))) +> + (notmuch-crypto-insert-sigstatus-button sigstatus from)))) +> + ;; if we're not adding encstatus, tell the user how they can get it +> + (button-put button 'help-echo "Set notmuch-crypto-process-mime to process cryptographic MIME parts.")) +> +> (let ((inner-parts (plist-get part :content)) +> (start (point))) +> @@ -722,8 +718,7 @@ message at DEPTH in the current thread." +> (indent-rigidly start (point) 1))) +> t) +> +> -(defun notmuch-show-insert-part-multipart/* (msg part content-type nth depth declared-type) +> - (notmuch-show-insert-part-header nth declared-type content-type nil) +> +(defun notmuch-show-insert-part-multipart/* (msg part content-type nth depth declared-type button) +> (let ((inner-parts (plist-get part :content)) +> (start (point))) +> ;; Show all of the parts. +> @@ -735,8 +730,7 @@ message at DEPTH in the current thread." +> (indent-rigidly start (point) 1))) +> t) +> +> -(defun notmuch-show-insert-part-message/rfc822 (msg part content-type nth depth declared-type) +> - (notmuch-show-insert-part-header nth declared-type content-type nil) +> +(defun notmuch-show-insert-part-message/rfc822 (msg part content-type nth depth declared-type button) +> (let* ((message (car (plist-get part :content))) +> (body (car (plist-get message :body))) +> (start (point))) +> @@ -757,7 +751,7 @@ message at DEPTH in the current thread." +> (indent-rigidly start (point) 1))) +> t) +> +> -(defun notmuch-show-insert-part-text/plain (msg part content-type nth depth declared-type) +> +(defun notmuch-show-insert-part-text/plain (msg part content-type nth depth declared-type button) +> (let ((start (point))) +> ;; If this text/plain part is not the first part in the message, +> ;; insert a header to make this clear. + +I know you're trying to minimize side-effects, but I think this will +also complicate maintenance. I'd rather see this call to +notmuch-show-insert-part-header removed and have the only call and all +special-case logic be in notmuch-show-insert-bodypart. As it is, +reasoning about exactly when a part button is inserted requires +understanding the conjunction of two widely separated parts of the code. + +> @@ -770,8 +764,7 @@ message at DEPTH in the current thread." +> (run-hook-with-args 'notmuch-show-insert-text/plain-hook msg depth)))) +> t) +> +> -(defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth declared-type) +> - (notmuch-show-insert-part-header nth declared-type content-type (plist-get part :filename)) +> +(defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth declared-type button) +> (insert (with-temp-buffer +> (insert (notmuch-get-bodypart-content msg part nth notmuch-show-process-crypto)) +> ;; notmuch-get-bodypart-content provides "raw", non-converted +> @@ -794,8 +787,8 @@ message at DEPTH in the current thread." +> t) +> +> ;; For backwards compatibility. +> -(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth declared-type) +> - (notmuch-show-insert-part-text/calendar msg part content-type nth depth declared-type)) +> +(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth declared-type button) +> + (notmuch-show-insert-part-text/calendar msg part content-type nth depth declared-type button)) +> +> (defun notmuch-show-get-mime-type-of-application/octet-stream (part) +> ;; If we can deduce a MIME type from the filename of the attachment, +> @@ -813,7 +806,7 @@ message at DEPTH in the current thread." +> nil)) +> nil)))) +> +> -(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type) +> +(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type button) +> ;; text/html handler to work around bugs in renderers and our +> ;; invisibile parts code. In particular w3m sets up a keymap which +> ;; "leaks" outside the invisible region and causes strange effects +> @@ -821,11 +814,10 @@ message at DEPTH in the current thread." +> ;; tell w3m not to set a keymap (so the normal notmuch-show-mode-map +> ;; remains). +> (let ((mm-inline-text-html-with-w3m-keymap nil)) +> - (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type))) +> + (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type button))) +> +> -(defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type) +> +(defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type button) +> ;; This handler _must_ succeed - it is the handler of last resort. +> - (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename)) +> (notmuch-mm-display-part-inline msg part nth content-type notmuch-show-process-crypto) +> t) +> +> @@ -848,13 +840,13 @@ message at DEPTH in the current thread." +> +> ;; + +> +> -(defun notmuch-show-insert-bodypart-internal (msg part content-type nth depth declared-type) +> +(defun notmuch-show-insert-bodypart-internal (msg part content-type nth depth declared-type button) +> (let ((handlers (notmuch-show-handlers-for content-type))) +> ;; Run the content handlers until one of them returns a non-nil +> ;; value. +> (while (and handlers +> (not (condition-case err +> - (funcall (car handlers) msg part content-type nth depth declared-type) +> + (funcall (car handlers) msg part content-type nth depth declared-type button) +> (error (progn +> (insert "!!! Bodypart insert error: ") +> (insert (error-message-string err)) +> @@ -882,6 +874,9 @@ message at DEPTH in the current thread." +> "Insert the body part PART at depth DEPTH in the current thread. +> +> If HIDE is non-nil then initially hide this part." +> + +> + ;; We handle text/plain specially as its code does things before +> + ;; inserting a part button (and does not always insert a part button). + +I think this comment would be clearer right above the button binding (or +maybe the separate diff hunks just make it more confusing than it is). + +> (let* ((content-type (downcase (plist-get part :content-type))) +> (mime-type (or (and (string= content-type "application/octet-stream") +> (notmuch-show-get-mime-type-of-application/octet-stream part)) +> @@ -889,9 +884,11 @@ If HIDE is non-nil then initially hide this part." +> "text/x-diff") +> content-type)) +> (nth (plist-get part :id)) +> - (beg (point))) +> + (beg (point)) +> + (button (unless (string= mime-type "text/plain") +> + (notmuch-show-insert-part-header nth mime-type content-type (plist-get part :filename))))) + +If you take my suggestion above, this would become something like + +(unless (and (string= mime-type "text/plain) (<= nth 1)) ...) + +or maybe clearer + +(when (or (not (string= mime-type "text/plain")) (> nth 1)) ...) + +> +> - (notmuch-show-insert-bodypart-internal msg part mime-type nth depth content-type) +> + (notmuch-show-insert-bodypart-internal msg part mime-type nth depth content-type button) +> ;; Some of the body part handlers leave point somewhere up in the +> ;; part, so we make sure that we're down at the end. +> (goto-char (point-max)) +> -- +> 1.7.9.1 +> +> _______________________________________________ +> notmuch mailing list +> notmuch@notmuchmail.org +> http://notmuchmail.org/mailman/listinfo/notmuch