Date: Mon, 7 Jan 2013 16:20:08 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH v2] emacs: show: make buttons select window
Message-ID: <20130107212008.GM17581@mit.edu>
References: <20130107202221.GL17581@mit.edu>
	<1357592840-31155-1-git-send-email-markwalters1009@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1357592840-31155-1-git-send-email-markwalters1009@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

LGTM.

Quoth Mark Walters on Jan 07 at  9:07 pm:
> Emacs has two button type objects: widgets (as used for saved searches
> in notmuch-hello) and buttons as used by parts/citations and id links
> in notmuch-show. These two behave subtly differently when clicked with
> the mouse: widgets select the window clicked before running the
> action, buttons do not.
> 
> This patch makes all of these behave the same: clicking always selects
> the clicked window. It does this by defining a notmuch-button-type
> supertype that the other notmuch buttons can inherit from. This
> supertype binds the mouse-action to select the window and then
> activate the button.
> ---
> 
> This versions fixes most of the comments raised by Austin's review.
> The one change I didn't make was changing :supertype to 'supertype. In
> principle I agree with Austin but the : form is used for inheritance
> for other notmuch buttons in wash and crypto.
> 
> Best wishes
> 
> Mark
> 
> 
> 
> 
>  emacs/notmuch-crypto.el |    5 ++++-
>  emacs/notmuch-lib.el    |   15 +++++++++++++++
>  emacs/notmuch-show.el   |    4 +++-
>  emacs/notmuch-wash.el   |    3 ++-
>  4 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/emacs/notmuch-crypto.el b/emacs/notmuch-crypto.el
> index 83e5d37..5233824 100644
> --- a/emacs/notmuch-crypto.el
> +++ b/emacs/notmuch-crypto.el
> @@ -19,6 +19,8 @@
>  ;;
>  ;; Authors: Jameson Rollins <jrollins@finestructure.net>
>  
> +(require 'notmuch-lib)
> +
>  (defcustom notmuch-crypto-process-mime nil
>    "Should cryptographic MIME parts be processed?
>  
> @@ -76,7 +78,8 @@ mode."
>  (define-button-type 'notmuch-crypto-status-button-type
>    'action (lambda (button) (message (button-get button 'help-echo)))
>    'follow-link t
> -  'help-echo "Set notmuch-crypto-process-mime to process cryptographic mime parts.")
> +  'help-echo "Set notmuch-crypto-process-mime to process cryptographic mime parts."
> +  :supertype 'notmuch-button-type)
>  
>  (defun notmuch-crypto-insert-sigstatus-button (sigstatus from)
>    (let* ((status (plist-get sigstatus :status))
> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
> index 0407f8a..6836192 100644
> --- a/emacs/notmuch-lib.el
> +++ b/emacs/notmuch-lib.el
> @@ -97,6 +97,21 @@ For example, if you wanted to remove an \"inbox\" tag and add an
>    :group 'notmuch-search
>    :group 'notmuch-show)
>  
> +;; By default clicking on a button does not select the window
> +;; containing the button (as opposed to clicking on a widget which
> +;; does). This means that the button action is then executed in the
> +;; current selected window which can cause problems if the button
> +;; changes the buffer (e.g., id: links) or moves point.
> +;;
> +;; This provides a button type which overrides mouse-action so that
> +;; the button's window is selected before the action is run. Other
> +;; notmuch buttons can get the same behaviour by inheriting from this
> +;; button type.
> +(define-button-type 'notmuch-button-type
> +  'mouse-action (lambda (button)
> +		  (select-window (posn-window (event-start last-input-event)))
> +		  (button-activate button)))
> +
>  (defun notmuch-version ()
>    "Return a string with the notmuch version number."
>    (let ((long-string
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5751d98..059194d 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -469,7 +469,8 @@ message at DEPTH in the current thread."
>    'action 'notmuch-show-part-button-default
>    'keymap 'notmuch-show-part-button-map
>    'follow-link t
> -  'face 'message-mml)
> +  'face 'message-mml
> +  :supertype 'notmuch-button-type)
>  
>  (defvar notmuch-show-part-button-map
>    (let ((map (make-sparse-keymap)))
> @@ -1075,6 +1076,7 @@ buttons for a corresponding notmuch search."
>  	;; Remove the overlay created by goto-address-mode
>  	(remove-overlays (first link) (second link) 'goto-address t)
>  	(make-text-button (first link) (second link)
> +			  :type 'notmuch-button-type
>  			  'action `(lambda (arg)
>  				     (notmuch-show ,(third link)))
>  			  'follow-link t
> diff --git a/emacs/notmuch-wash.el b/emacs/notmuch-wash.el
> index d6db4fa..826b6f4 100644
> --- a/emacs/notmuch-wash.el
> +++ b/emacs/notmuch-wash.el
> @@ -115,7 +115,8 @@ lower).")
>  (define-button-type 'notmuch-wash-button-invisibility-toggle-type
>    'action 'notmuch-wash-toggle-invisible-action
>    'follow-link t
> -  'face 'font-lock-comment-face)
> +  'face 'font-lock-comment-face
> +  :supertype 'notmuch-button-type)
>  
>  (define-button-type 'notmuch-wash-button-citation-toggle-type
>    'help-echo "mouse-1, RET: Show citation"