From: Mark Walters <markwalters1009@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>, notmuch@notmuchmail.org
Subject: Re: [PATCH 1/5] emacs: compile and load notmuch-pick.el if present.
In-Reply-To: <m2ipdcqjwj.fsf@guru.guru-group.fi>
References: <1343164911-31589-1-git-send-email-markwalters1009@gmail.com>
	<1343164911-31589-2-git-send-email-markwalters1009@gmail.com>
	<m2ipdcqjwj.fsf@guru.guru-group.fi>
User-Agent: Notmuch/0.13.2+93~gf33b188 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Wed, 25 Jul 2012 22:12:27 +0100
Message-ID: <87fw8fsfyc.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Wed, 25 Jul 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Wed, Jul 25 2012, Mark Walters <markwalters1009@gmail.com> wrote:
>
>> Compile and load notmuch-pick.el if present.
>>
>> All the actual setup of pick is done in the function notmuch-pick-init
>> so we call that in the notmuch init function if it is bound. This
>> function will setup all extra keybinding etc.
>
> Great stuff! See a few thoughts below...

Thanks for these: I think they are all clear improvements.

>
>> ---
>>  emacs/Makefile.local |    3 ++-
>>  emacs/notmuch.el     |    5 +++++
>>  2 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/emacs/Makefile.local b/emacs/Makefile.local
>> index fb82247..9f4dba6 100644
>> --- a/emacs/Makefile.local
>> +++ b/emacs/Makefile.local
>> @@ -15,7 +15,8 @@ emacs_sources := \
>>  	$(dir)/notmuch-crypto.el \
>>  	$(dir)/notmuch-tag.el \
>>  	$(dir)/coolj.el \
>> -	$(dir)/notmuch-print.el
>> +	$(dir)/notmuch-print.el \
>> +	$(wildcard $(dir)/notmuch-pick.el)
>
> I wonder whether having this conditional is good idea. What if
> someone copies (or (sym)links) notmuch-pick.el there and then
> compiles and takes to use. Next time he takes clean tree and
> forgets to do this copying and installs to the same destination.
> Now there is old notmuch-pick.elc which might be out of sync.
>
> I think it would be better to provide a shell script in 
> notmuch-pick directory which byte-compiles and installs notmuch-pick
> in case user wants to install/update notmuch-pick. Whenever notmuch-pick
> is good enough to be shipped inside $(dir) above then the aboce conditional
> is not needed (at all).
>

I have now done this: I actually use a Makefile so that I can pick up
the config from notmuch: in particular the install directory
for the notmuch-pick.elc file. Does that seem reasonable?


>>  emacs_images := \
>>  	$(srcdir)/$(dir)/notmuch-logo.png
>> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
>> index fd1836f..4f3da4f 100644
>> --- a/emacs/notmuch.el
>> +++ b/emacs/notmuch.el
>> @@ -59,6 +59,9 @@
>>  (require 'notmuch-maildir-fcc)
>>  (require 'notmuch-message)
>>  
>> +;; Load notmuch-pick if available (but do not error if not present).
>> +(load "notmuch-pick" t)
>> +
>>  (defcustom notmuch-search-result-format
>>    `(("date" . "%12s ")
>>      ("count" . "%-7s ")
>> @@ -1088,6 +1091,8 @@ current search results AND that are tagged with the given tag."
>>  (defun notmuch ()
>>    "Run notmuch and display saved searches, known tags, etc."
>>    (interactive)
>> +  (when (fboundp 'notmuch-pick-init)
>> +    (notmuch-pick-init))
>>    (notmuch-hello))
>
> Instead of this could notmuch-pick.el contain:
>
> In the beginning:
>
> (require 'notmuch-hello)
> (require 'notmuch-show)
> (require 'notmuch) ;; XXX ATM, as notmuch-search-mode-map is defined here
>
> And, at the end, before (provide 'notmuch-pick), execute the lines
> what currenty are contained in (notmuch-pick-init).
>
> Then, those who want to start using notmuch-pick at this time
> can (just) write the following in their .emacs:
>
> (require 'notmuch)
> (require 'notmuch-pick)

This works very nicely. And if we want a transitional period the
notmuch-pick could be in (dir) but not loaded automatically.

Many thanks for the excellent suggestions.

Mark