From: Tomi Ollila <tomi.ollila@iki.fi>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v4 5/5] notmuch-emacs-mua: do not create a frame by
 default with --client
In-Reply-To: <87si73uvbu.fsf@nikula.org>
References: <cover.1439629384.git.jani@nikula.org>
 <455026447d3f75b1038ac11cebe734431604be9d.1439629384.git.jani@nikula.org>
 <m21tepvpm5.fsf@guru.guru-group.fi> <87si73uvbu.fsf@nikula.org>
User-Agent: Notmuch/0.20.2+68~g0c35549 (http://notmuchmail.org) Emacs/24.3.1
 (x86_64-unknown-linux-gnu)
Date: Fri, 28 Aug 2015 22:18:17 +0300
Message-ID: <m2mvxbjk7a.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list

On Fri, Aug 28 2015, Jani Nikula <jani@nikula.org> wrote:

> On Wed, 26 Aug 2015, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>> On Sat, Aug 15 2015, Jani Nikula <jani@nikula.org> wrote:
>>
>>> Make the default behaviour for --client the same as emacsclient
>>> default: do not create a new frame. Add a new option --create-frame,
>>> passing the same option to emacsclient to create a frame.
>>>
>>> ---
>>>
>>> v2: fix killing frame with --create-frame
>>>
>>> v3: fix --create-frame w/o --client parameter check
>>>
>>> v4: fix kill terminal logic, document behaviour with daemon mode
>>> ---
>>>  doc/man1/notmuch-emacs-mua.rst |  6 ++++++
>>>  notmuch-emacs-mua              | 14 ++++++++++----
>>>  2 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/doc/man1/notmuch-emacs-mua.rst b/doc/man1/notmuch-emacs-mu=
a.rst
>>> index 89a2fead17b1..c3689eb82c8a 100644
>>> --- a/doc/man1/notmuch-emacs-mua.rst
>>> +++ b/doc/man1/notmuch-emacs-mua.rst
>>> @@ -45,6 +45,12 @@ Supported options for **notmuch-emacs-mua** include
>>>          Automatically start Emacs in daemon mode, if the Emacs server
>>>          is not running. Applicable with ``--client``.
>>>=20=20
>>> +    ``--create-frame``
>>> +        Create a new frame instead of trying to use the current Emacs
>>> +        frame. Applicable with ``--client``. This will be required
>>> +        when Emacs is running (or automatically started with
>>> +        ``--auto-daemon``) in daemon mode.
>>> +
>>>      ``--print``
>>>          Output the resulting elisp to stdout instead of evaluating it.
>>>=20=20
>>> diff --git a/notmuch-emacs-mua b/notmuch-emacs-mua
>>> index ac03a4a60d56..b51d8d0e2c5b 100755
>>> --- a/notmuch-emacs-mua
>>> +++ b/notmuch-emacs-mua
>>> @@ -37,11 +37,10 @@ PRINT_ONLY=3D
>>>  NO_WINDOW=3D
>>>  USE_EMACSCLIENT=3D
>>>  AUTO_DAEMON=3D
>>> -CREATE_FRAME=3D"-c"
>>> +CREATE_FRAME=3D
>>>=20=20
>>>  # The crux of it all: construct an elisp progn and eval it.
>>>  ELISP=3D"(prog1 'done (require 'notmuch) (notmuch-mua-new-mail)"
>>> -ELISP=3D"${ELISP} (setq message-exit-actions (list #'save-buffers-kill=
-terminal))"
>>>=20=20
>>>  # Short options compatible with mutt(1).
>>>  while getopts :s:c:b:i:h opt; do
>>> @@ -65,7 +64,7 @@ while getopts :s:c:b:i:h opt; do
>>>  		    opt=3D${opt%%=3D*}
>>>  		    ;;
>>>  		# Long options without arguments.
>>> -		--help|--print|--no-window-system|--client|--auto-daemon)
>>> +		--help|--print|--no-window-system|--client|--auto-daemon|--create-fr=
ame)
>>>  		    ;;
>>>  		*)
>>>  		    echo "$0: unknown long option ${opt}, or argument mismatch." >&2
>>> @@ -103,7 +102,6 @@ while getopts :s:c:b:i:h opt; do
>>>  	    ;;
>>>  	--no-window-system)
>>>  	    NO_WINDOW=3D"-nw"
>>> -	    CREATE_FRAME=3D
>>>  	    ;;
>>>  	--client)
>>>  	    USE_EMACSCLIENT=3D"yes"
>>> @@ -111,6 +109,9 @@ while getopts :s:c:b:i:h opt; do
>>>  	--auto-daemon)
>>>  	    AUTO_DAEMON=3D"--alternate-editor=3D"
>>>  	    ;;
>>> +	--create-frame)
>>> +	    CREATE_FRAME=3D"-c"
>>> +	    ;;
>>>  	*)
>>>  	    # We should never end up here.
>>>  	    echo "$0: internal error (option ${opt})." >&2
>>
>> This looks pretty good now; I player quite a bit with --client,
>> --create-frame and --auto-daemon=20
>> (with and without setting EMACSCLIENT=3D'echo emacsclient')
>>
>> Based on that I think this check should be added here (after option
>> parsing):
>>
>> if [ -n "$AUTO_DAEMON" -a -z "$CREATE_FRAME" ]; then
>>     echo "$0: --auto-daemon is only applicable with --create-frame." >&2
>>     exit 1=20=20=20=20=20=20=20
>> fi
>>
>> without this one may execute ./notmuch-emacs-mua --client --auto-daemon
>> which yields starting emacs in daemon mode (in this example it is expect=
ed
>> emacs is not running; otherwise --auto-daemon has no use in this example)
>> -- but no ui to that newly-running emacs is provided. Similar behaviour
>> can be observed by the following
>
> This problem is present with the current code already, right now. I
> don't see the need to fix everything in my series.

Yes, the problem was already present in patch 4/5 (and probably earlier as
you mentioned that there was problem already) -- I just did not want
to pick that also in this discussion... ;/

>> $ emacsclient --eval '(kill emacs) ;; but beware! '=20
>> $ emacsclient --alternate-editor=3D --eval '(find-file "/etc/passwd")'
>>
>> (side note: using this --alternate-editor=3D is pretty nifty way to get =
it done)
>>
>> emacs --daemon is executed which loaded /etc/passwd but w/o ui
>>
>> Note the difference in:
>>
>> emacsclient --alternate-editor=3D /etc/passwd
>>
>> now emacs --daemon is executed *and* ui where /etc/passwd is available is
>> provided!
>>
>> otoh,
>> emacsclient --alternate-editor=3D -c [-nw] --eval '(find-file "/etc/pass=
wd")'
>>
>> works fine, albeit a bit different than the one where /etc/passwd was gi=
ven
>> as a simple filename argument.
>>
>> comparative to ./notmuch-emacs-mua --client --create-frame --auto-daemon=
=20
>>
>>> @@ -128,6 +129,11 @@ for arg; do
>>>      ELISP=3D"${ELISP} (message-goto-to) (insert \"${arg}, \")"
>>>  done
>>>=20=20
>>> +# Kill the terminal/frame if we're creating one.
>>> +if [ -z "$USE_EMACSCLIENT" -o -n "$CREATE_FRAME" -o -n "$NO_WINDOW" ];=
 then
>>> +    ELISP=3D"${ELISP} (setq message-exit-actions (list #'save-buffers-=
kill-terminal))"
>>> +fi
>>
>> I am not very happy that message-exit-actions was added to $ELISP when
>> not using emacsclient; when emacs is started its sole (initial) purpose =
is
>> to serve mail sending (and not lending a frame in some other emacs) -- in
>> this case it would be nice to be able to retrieve the sent mail buffer.
>
> And I am not very happy with being expected to modify these patches ad
> infinitum while trying to get back the *one* use case I wrote this
> script for in the first place, and that got broken somewhere down the
> line.

I can agree that these patches could be applied as is and I can do the
followup patch that changes the things later ... but I'd like to get these
changes I presented discussed and agreed (whatever the agreement is) before
doing these so that the result will be good and safe for all potential
users...

>
> Sincerely,
> Jani.
>

yst=C3=A4v=C3=A4llisin terveisin :D,
Tomi

>
>>
>> perhaps the above could be changed to just=20
>>
>> if [ -n "$CREATE_FRAME" ]; then
>>     ELISP=3D"${ELISP} (setq message-exit-actions (list #'save-buffers-ki=
ll-terminal))"
>> fi
>>
>> another option is --yet-another-option ;/
>>
>> No other issues (relevant to this series).
>>
>> Tomi
>>
>>
>>> +
>>>  # End progn.
>>>  ELISP=3D"${ELISP})"
>>>=20=20
>>> --=20
>>> 2.1.4