To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH v7 1/8] emacs: Rework crypto switch toggle.
In-Reply-To: <20120208051016.GB27039@mit.edu>
References: <1327486729-18052-1-git-send-email-dme@dme.org>
	<1328520082-26659-1-git-send-email-dme@dme.org>
	<1328520082-26659-2-git-send-email-dme@dme.org>
	<20120208051016.GB27039@mit.edu>
User-Agent: Notmuch/0.11.1+153~gdd83b99 (http://notmuchmail.org)
	Emacs/24.0.92.1 (x86_64-pc-linux-gnu)
From: David Edmondson <dme@dme.org>
Date: Wed, 08 Feb 2012 06:21:34 +0000
Message-ID: <cunvcnh3kwh.fsf@hotblack-desiato.hh.sledj.net>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Cc: notmuch@notmuchmail.org
Precedence: list

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On Wed, 8 Feb 2012 00:10:16 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Seems reasonable.  I'm definitely in favor of erasing buffers instead
> of killing and recreating them.  Two questions below.

Thanks for the review.

> Quoth David Edmondson on Feb 06 at  9:21 am:
> > Re-work the existing crypto switch toggle to be based on a persistant
> > buffer-local variable.
> >=20
> > To allow this, modify `notmuch-show-refresh-view' to erase and re-draw
> > in the current buffer rather than killing the current buffer and
> > creating a new one. (This will also allow more per-buffer behaviour in
> > future patches.)
> >=20
> > Add a binding ('$') to toggle crypto processing of the current buffer
> > and remove the prefix argument approach that achieves a similar
> > result.
> > ---
> >  emacs/notmuch-show.el |  126 ++++++++++++++++++++++++-----------------=
-------
> >  emacs/notmuch.el      |    7 +--
> >  2 files changed, 66 insertions(+), 67 deletions(-)
> >=20
> > diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> > index 7469e2e..4b29dbd 100644
> > --- a/emacs/notmuch-show.el
> > +++ b/emacs/notmuch-show.el
> > @@ -125,6 +125,22 @@ indentation."
> >  		 (const :tag "View interactively"
> >  			notmuch-show-interactively-view-part)))
> >=20=20
> > +(defvar notmuch-show-thread-id nil)
> > +(make-variable-buffer-local 'notmuch-show-thread-id)
> > +(put 'notmuch-show-thread-id 'permanent-local t)
> > +
> > +(defvar notmuch-show-parent-buffer nil)
> > +(make-variable-buffer-local 'notmuch-show-parent-buffer)
> > +(put 'notmuch-show-parent-buffer 'permanent-local t)
> > +
> > +(defvar notmuch-show-query-context nil)
> > +(make-variable-buffer-local 'notmuch-show-query-context)
> > +(put 'notmuch-show-query-context 'permanent-local t)
> > +
> > +(defvar notmuch-show-process-crypto nil)
> > +(make-variable-buffer-local 'notmuch-show-process-crypto)
> > +(put 'notmuch-show-process-crypto 'permanent-local t)
> > +
>=20
> Do these need to be permanent-local given that refreshing is now done
> by erasing the buffer instead of killing it?

Yes. `notmuch-show-worker' (which does the refreshing) still calls
`notmuch-show-mode', which still calls `kill-all-local-variables'.

Tidying that is for another patch.

> >  (defmacro with-current-notmuch-show-message (&rest body)
> >    "Evaluate body with current buffer set to the text of current messag=
e"
> >    `(save-excursion
> > @@ -421,14 +437,11 @@ message at DEPTH in the current thread."
> >=20=20
> >  (defmacro notmuch-with-temp-part-buffer (message-id nth &rest body)
> >    (declare (indent 2))
> > -  (let ((process-crypto (make-symbol "process-crypto")))
> > -    `(let ((,process-crypto notmuch-show-process-crypto))
> > -       (with-temp-buffer
> > -	 (setq notmuch-show-process-crypto ,process-crypto)
> > -	 ;; Always acquires the part via `notmuch part', even if it is
> > -	 ;; available in the JSON output.
> > -	 (insert (notmuch-show-get-bodypart-internal ,message-id ,nth))
> > -	 ,@body))))
> > +  `(with-temp-buffer
> > +     ;; Always acquires the part via `notmuch part', even if it is
> > +     ;; available in the JSON output.
> > +     (insert (notmuch-show-get-bodypart-internal ,message-id ,nth))
> > +     ,@body))
>=20
> Why is the piping of notmuch-show-process-crypto no longer necessary?
> It's still buffer-local, and hence will be nil in the temp buffer.

It's a bug. v8 after breakfast.

> >=20=20
> >  (defun notmuch-show-save-part (message-id nth &optional filename conte=
nt-type)
> >    (notmuch-with-temp-part-buffer message-id nth
> > @@ -610,7 +623,7 @@ current buffer, if possible."
> >  	       (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 t=
o process cryptographic mime parts.")))
> > +      (button-put button 'help-echo "Set notmuch-crypto-process-mime t=
o process cryptographic MIME parts.")))
> >=20=20
> >    (let ((inner-parts (plist-get part :content))
> >  	(start (point)))
> > @@ -636,7 +649,7 @@ current buffer, if possible."
> >  		     (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 t=
o process cryptographic mime parts.")))
> > +      (button-put button 'help-echo "Set notmuch-crypto-process-mime t=
o process cryptographic MIME parts.")))
> >=20=20
> >    (let ((inner-parts (plist-get part :content))
> >  	(start (point)))
> > @@ -763,8 +776,6 @@ current buffer, if possible."
> >=20=20
> >  ;; Helper for parts which are generally not included in the default
> >  ;; JSON output.
> > -;; Uses the buffer-local variable notmuch-show-process-crypto to
> > -;; determine if parts should be decrypted first.
> >  (defun notmuch-show-get-bodypart-internal (message-id part-number)
> >    (let ((args '("show" "--format=3Draw"))
> >  	(part-arg (format "--part=3D%s" part-number)))
> > @@ -918,6 +929,15 @@ current buffer, if possible."
> >      ;; criteria.
> >      (notmuch-show-message-visible msg (plist-get msg :match))))
> >=20=20
> > +(defun notmuch-show-toggle-process-crypto ()
> > +  "Toggle the processing of cryptographic MIME parts."
> > +  (interactive)
> > +  (setq notmuch-show-process-crypto (not notmuch-show-process-crypto))
> > +  (message (if notmuch-show-process-crypto
> > +	       "Processing cryptographic MIME parts."
> > +	     "Not processing cryptographic MIME parts."))
> > +  (notmuch-show-refresh-view))
> > +
> >  (defun notmuch-show-insert-tree (tree depth)
> >    "Insert the message tree TREE at depth DEPTH in the current thread."
> >    (let ((msg (car tree))
> > @@ -933,15 +953,6 @@ current buffer, if possible."
> >    "Insert the forest of threads FOREST."
> >    (mapc (lambda (thread) (notmuch-show-insert-thread thread 0)) forest=
))
> >=20=20
> > -(defvar notmuch-show-thread-id nil)
> > -(make-variable-buffer-local 'notmuch-show-thread-id)
> > -(defvar notmuch-show-parent-buffer nil)
> > -(make-variable-buffer-local 'notmuch-show-parent-buffer)
> > -(defvar notmuch-show-query-context nil)
> > -(make-variable-buffer-local 'notmuch-show-query-context)
> > -(defvar notmuch-show-buffer-name nil)
> > -(make-variable-buffer-local 'notmuch-show-buffer-name)
> > -
> >  (defun notmuch-show-buttonise-links (start end)
> >    "Buttonise URLs and mail addresses between START and END.
> >=20=20
> > @@ -961,7 +972,7 @@ a corresponding notmuch search."
> >  			'face goto-address-mail-face))))
> >=20=20
> >  ;;;###autoload
> > -(defun notmuch-show (thread-id &optional parent-buffer query-context b=
uffer-name crypto-switch)
> > +(defun notmuch-show (thread-id &optional parent-buffer query-context b=
uffer-name)
> >    "Run \"notmuch show\" with the given thread ID and display results.
> >=20=20
> >  The optional PARENT-BUFFER is the notmuch-search buffer from
> > @@ -976,46 +987,41 @@ non-nil.
> >  The optional BUFFER-NAME provides the name of the buffer in
> >  which the message thread is shown. If it is nil (which occurs
> >  when the command is called interactively) the argument to the
> > -function is used.
> > -
> > -The optional CRYPTO-SWITCH toggles the value of the
> > -notmuch-crypto-process-mime customization variable for this show
> > -buffer."
> > +function is used."
> >    (interactive "sNotmuch show: ")
> > -  (let* ((process-crypto (if crypto-switch
> > -			     (not notmuch-crypto-process-mime)
> > -			   notmuch-crypto-process-mime)))
> > -    (notmuch-show-worker thread-id parent-buffer query-context buffer-=
name process-crypto)))
> > -
> > -(defun notmuch-show-worker (thread-id parent-buffer query-context buff=
er-name process-crypto)
> > -  (let* ((buffer-name (generate-new-buffer-name
> > -		       (or buffer-name
> > -			   (concat "*notmuch-" thread-id "*"))))
> > -	 (buffer (get-buffer-create buffer-name))
> > -	 (inhibit-read-only t))
> > -    (switch-to-buffer buffer)
> > +  (let ((buffer-name (generate-new-buffer-name
> > +		      (or buffer-name
> > +			  (concat "*notmuch-" thread-id "*")))))
> > +    (switch-to-buffer (get-buffer-create buffer-name))
> > +    ;; Set the default value for `notmuch-show-process-crypto' in this
> > +    ;; buffer.
> > +    (setq notmuch-show-process-crypto notmuch-crypto-process-mime)
> > +
> > +    (setq notmuch-show-thread-id thread-id
> > +	  notmuch-show-parent-buffer parent-buffer
> > +	  notmuch-show-query-context query-context)
> > +    (notmuch-show-worker)))
> > +
> > +(defun notmuch-show-worker ()
> > +  (let ((inhibit-read-only t))
> > +
> >      (notmuch-show-mode)
> >      ;; Don't track undo information for this buffer
> >      (set 'buffer-undo-list t)
> >=20=20
> > -    (setq notmuch-show-thread-id thread-id)
> > -    (setq notmuch-show-parent-buffer parent-buffer)
> > -    (setq notmuch-show-query-context query-context)
> > -    (setq notmuch-show-buffer-name buffer-name)
> > -    (setq notmuch-show-process-crypto process-crypto)
> > -
> >      (erase-buffer)
> >      (goto-char (point-min))
> >      (save-excursion
> > -      (let* ((basic-args (list thread-id))
> > -	     (args (if query-context
> > -		       (append (list "\'") basic-args (list "and (" query-context ")=
\'"))
> > +      (let* ((basic-args (list notmuch-show-thread-id))
> > +	     (args (if notmuch-show-query-context
> > +		       (append (list "\'") basic-args
> > +			       (list "and (" notmuch-show-query-context ")\'"))
> >  		     (append (list "\'") basic-args (list "\'")))))
> >  	(notmuch-show-insert-forest (notmuch-query-get-threads args))
> >  	;; If the query context reduced the results to nothing, run
> >  	;; the basic query.
> >  	(when (and (eq (buffer-size) 0)
> > -		   query-context)
> > +		   notmuch-show-query-context)
> >  	  (notmuch-show-insert-forest
> >  	   (notmuch-query-get-threads basic-args))))
> >=20=20
> > @@ -1032,21 +1038,14 @@ buffer."
> >=20=20
> >      (notmuch-show-mark-read)))
> >=20=20
> > -(defun notmuch-show-refresh-view (&optional crypto-switch)
> > -  "Refresh the current view (with crypto switch if prefix given).
> > +(defun notmuch-show-refresh-view ()
> > +  "Refresh the current view.
> >=20=20
> > -Kills the current buffer and reruns notmuch show with the same
> > -thread id.  If a prefix is given, crypto processing is toggled."
> > -  (interactive "P")
> > -  (let ((thread-id notmuch-show-thread-id)
> > -	(parent-buffer notmuch-show-parent-buffer)
> > -	(query-context notmuch-show-query-context)
> > -	(buffer-name notmuch-show-buffer-name)
> > -	(process-crypto (if crypto-switch
> > -			    (not notmuch-show-process-crypto)
> > -			  notmuch-show-process-crypto)))
> > -    (notmuch-kill-this-buffer)
> > -    (notmuch-show-worker thread-id parent-buffer query-context buffer-=
name process-crypto)))
> > +Refreshes the current view, observing changes in cryptographic prefere=
nces."
> > +  (interactive)
> > +  (let ((inhibit-read-only t))
> > +    (erase-buffer))
> > +  (notmuch-show-worker))
> >=20=20
> >  (defvar notmuch-show-stash-map
> >    (let ((map (make-sparse-keymap)))
> > @@ -1097,6 +1096,7 @@ thread id.  If a prefix is given, crypto processi=
ng is toggled."
> >  	(define-key map (kbd "M-RET") 'notmuch-show-open-or-close-all)
> >  	(define-key map (kbd "RET") 'notmuch-show-toggle-message)
> >  	(define-key map "#" 'notmuch-show-print-message)
> > +	(define-key map "$" 'notmuch-show-toggle-process-crypto)
> >  	map)
> >        "Keymap for \"notmuch show\" buffers.")
> >  (fset 'notmuch-show-mode-map notmuch-show-mode-map)
> > diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> > index cd04ffd..ff0db99 100644
> > --- a/emacs/notmuch.el
> > +++ b/emacs/notmuch.el
> > @@ -463,9 +463,9 @@ Complete list of currently available key bindings:
> >    "Return a list of authors for the current region"
> >    (notmuch-search-properties-in-region 'notmuch-search-subject beg end=
))
> >=20=20
> > -(defun notmuch-search-show-thread (&optional crypto-switch)
> > +(defun notmuch-search-show-thread ()
> >    "Display the currently selected thread."
> > -  (interactive "P")
> > +  (interactive)
> >    (let ((thread-id (notmuch-search-find-thread-id))
> >  	(subject (notmuch-prettify-subject (notmuch-search-find-subject))))
> >      (if (> (length thread-id) 0)
> > @@ -473,8 +473,7 @@ Complete list of currently available key bindings:
> >  		      (current-buffer)
> >  		      notmuch-search-query-string
> >  		      ;; Name the buffer based on the subject.
> > -		      (concat "*" (truncate-string-to-width subject 30 nil nil t) "*=
")
> > -		      crypto-switch)
> > +		      (concat "*" (truncate-string-to-width subject 30 nil nil t) "*=
"))
> >        (message "End of search results."))))
> >=20=20
> >  (defun notmuch-search-reply-to-thread (&optional prompt-for-sender)

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAk8yFG4ACgkQaezQq/BJZRbXOgCeM4wQB0VrfV5AXYmuYhL+RKet
bFIAn0VHsotEKKltcFE4k1+M5E3Xrj4c
=N2k6
-----END PGP SIGNATURE-----
--=-=-=--