MIME-Version: 1.0
Sender: amdragon@gmail.com
In-Reply-To: <1319528464-22654-1-git-send-email-schnouki@schnouki.net>
References: <87pqhlh64c.fsf@thor.loria.fr>
	<1319528464-22654-1-git-send-email-schnouki@schnouki.net>
Date: Fri, 4 Nov 2011 23:51:55 -0400
Message-ID:
 <CAH-f9WvuSFJJC+e0JRFwKD8xYtyncNYZreCXxxMDeSahpWAuRw@mail.gmail.com>
Subject: Re: [PATCH] emacs: Let the user choose where to compose new mails
From: Austin Clements <amdragon@mit.edu>
To: Thomas Jost <schnouki@schnouki.net>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Cc: notmuch@notmuchmail.org
Precedence: list

This seems like a good option to have, but your approach seems
unnecessarily complicated. I'm always wary of defcustom's :set because
it means you can't just setq the variable, which defeats the
underlying beauty of the customize system.=A0 You could eliminate the
other two variables and compute them on the fly, or, if you really
feel they may need to be controlled independently, make the custom
variable a pair or alist (which you can hide behind a few const
choices).  Alternatively, it seems like the variable could instead
take a single function (basically what notmuch-mua-switch-function is
now) and you could provide two new functions that simply combine
switch-to-buffer-other-x and set-window-dedicated-p.

On Oct 25, 2011 3:41 AM, "Thomas Jost" <schnouki@schnouki.net> wrote:
>
> ---
> =A0emacs/notmuch-mua.el | =A0 37 +++++++++++++++++++++++++++++++++++--
> =A01 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/emacs/notmuch-mua.el b/emacs/notmuch-mua.el
> index 8824b08..ebc922f 100644
> --- a/emacs/notmuch-mua.el
> +++ b/emacs/notmuch-mua.el
> @@ -31,6 +31,34 @@
> =A0 :group 'notmuch
> =A0 :type 'hook)
>
> +(defvar notmuch-mua-switch-function nil
> + =A0"Function used to switch and display the mail buffer. This is
> + =A0normally set by `notmuch-mua-compose-in'.")
> +(defvar notmuch-mua-dedicated-flag nil
> + =A0"Flag to pass to `set-window-dedicated-p' in the mail buffer.
> + =A0This is normally set by `notmuch-mua-compose-in'.")
> +(defcustom notmuch-mua-compose-in 'current-window
> + =A0"Where to create the mail buffer used to compose a new message.
> + =A0Possible values are `current-window' (default), `new-window'
> + =A0and `new-frame'. If set to `new-window' or `new-frame', the
> + =A0mail buffer will be displayer in a new window/frame that will

s/displayer/displayed/

> + =A0be destroyed when the buffer is killed. You may want to
> + =A0customize `message-kill-buffer-on-exit' accordingly."

You should also describe 'current-window' in the defcustom docstring.
(Currently you describe the effect of the other two, but not
current-window.)

> + =A0:group 'notmuch
> + =A0:type 'symbol
> + =A0:set (lambda (sym val)
> + =A0 =A0 =A0 =A0(cond ((eq val 'current-window)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 (setq notmuch-mua-switch-function nil
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 notmuch-mua-dedicated-flag nil)=
)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0((eq val 'new-window)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 (setq notmuch-mua-switch-function 'switch-t=
o-buffer-other-window
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 notmuch-mua-dedicated-flag 1))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0((eq val 'new-frame)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 (setq notmuch-mua-switch-function 'switch-t=
o-buffer-other-frame
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 notmuch-mua-dedicated-flag 1))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0(t (error (concat "Bad value for notmuch-mua=
-compose-in: "
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(symbol-=
value val)))))))
> +

The defcustom would be more user-friendly if it gave a choice between
const values, rather than requiring the user to enter a symbol value
(and then possibly rejecting it on validation).  Something like
  :type '(choice (const :tag "Compose in the current window" current-window=
)
                 (const :tag "Compose mail in a new window"  new-window)
                 (const :tag "Compose mail in a new frame"   new-frame))

> =A0(defcustom notmuch-mua-user-agent-function 'notmuch-mua-user-agent-ful=
l
> =A0 "Function used to generate a `User-Agent:' string. If this is
> =A0`nil' then no `User-Agent:' will be generated."
> @@ -99,7 +127,8 @@ list."
> =A0 =A0 =A0 =A0((same-window-regexps '("\\*mail .*")))
> =A0 =A0 =A0 (notmuch-mua-mail (mail-header 'to headers)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(mail-header 'subject head=
ers)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (message-headers-to-generat=
e headers t '(to subject))))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (message-headers-to-generat=
e headers t '(to subject))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 nil notmuch-mua-switch-func=
tion))
> =A0 =A0 ;; insert the message body - but put it in front of the signature
> =A0 =A0 ;; if one is present
> =A0 =A0 (goto-char (point-max))
> @@ -112,6 +141,8 @@ list."
> =A0 (message-goto-body))
>
> =A0(defun notmuch-mua-forward-message ()
> + =A0(when notmuch-mua-switch-function
> + =A0 =A0(funcall notmuch-mua-switch-function (current-buffer)))
> =A0 (message-forward)
>
> =A0 (when notmuch-mua-user-agent-function
> @@ -121,6 +152,7 @@ list."
> =A0 (message-sort-headers)
> =A0 (message-hide-headers)
> =A0 (set-buffer-modified-p nil)
> + =A0(set-window-dedicated-p (selected-window) notmuch-mua-dedicated-flag=
)
>
> =A0 (message-goto-to))
>
> @@ -143,6 +175,7 @@ list."
> =A0 (message-sort-headers)
> =A0 (message-hide-headers)
> =A0 (set-buffer-modified-p nil)
> + =A0(set-window-dedicated-p (selected-window) notmuch-mua-dedicated-flag=
)
>
> =A0 (message-goto-to))
>
> @@ -199,7 +232,7 @@ the From: address first."
> =A0 (let ((other-headers
> =A0 =A0 =A0 =A0 (when (or prompt-for-sender notmuch-always-prompt-for-sen=
der)
> =A0 =A0 =A0 =A0 =A0 (list (cons 'from (notmuch-mua-prompt-for-sender)))))=
)
> - =A0 =A0(notmuch-mua-mail nil nil other-headers)))
> + =A0 =A0(notmuch-mua-mail nil nil other-headers nil notmuch-mua-switch-f=
unction)))
>
> =A0(defun notmuch-mua-new-forward-message (&optional prompt-for-sender)
> =A0 "Invoke the notmuch message forwarding window.
> --
> 1.7.7
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch