On Sun, 26 May 2013, Mark Walters <markwalters1009 at gmail.com> 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 at notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch