[PATCH v2 0/5] emacs: notmuch-hello status message refresh hook, etc

[PATCH v2 0/5] emacs: notmuch-hello status message refresh hook, etc

[Jani Nikula]

Hi all, v2 of [1] addressing review comments: added NEWS, improved
documentation, made the query customizable.

[1] id:"37899e28dbf67e4620a53279a869be3174c02d6f.1339775602.git.jani@nikula.org"

BR,
Jani.

Jani Nikula (5):
  emacs: document the notmuch-hello no-display argument
  emacs: add no-display arg to notmuch-hello-refresh-hook
  test: fix hook-counter to accept the new no-display param
  emacs: add notmuch hello refresh hook to display message count change
  NEWS: notmuch-hello refresh hook changes

 NEWS                   |   18 ++++++++++++++++++
 emacs/notmuch-hello.el |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 test/test-lib.el       |    2 +-
 3 files changed, 63 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/5] emacs: document the notmuch-hello no-display argument

[Jani Nikula]

---
 emacs/notmuch-hello.el |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 684bedc..4dfe5d2 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -751,7 +751,11 @@ following:
 
 ;;;###autoload
 (defun notmuch-hello (&optional no-display)
-  "Run notmuch and display saved searches, known tags, etc."
+  "Run notmuch and display saved searches, known tags, etc.
+
+If the optional NO-DISPLAY argument is non-nil, only update the
+notmuch-hello buffer if it already exists, without switching to
+it (i.e. use `set-buffer' rather than `switch-to-buffer')."
   (interactive)
 
   ;; Jump through a hoop to get this value from the deprecated variable
-- 
1.7.9.5

[PATCH v2 2/5] emacs: add no-display arg to notmuch-hello-refresh-hook

[Jani Nikula]

Add no-display arg to notmuch-hello-refresh-hook to allow each hook to
decide what is appropriate when no-display is t, i.e. when not
switching to the notmuch-hello buffer.

This breaks existing hooks people might have, which will now need to
accept the argument.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 emacs/notmuch-hello.el |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 4dfe5d2..fa14443 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -816,7 +816,7 @@ it (i.e. use `set-buffer' rather than `switch-to-buffer')."
       (unless (widget-at)
 	(when notmuch-hello-search-pos
 	  (goto-char notmuch-hello-search-pos)))))
-  (run-hooks 'notmuch-hello-refresh-hook)
+  (run-hook-with-args 'notmuch-hello-refresh-hook no-display)
   (setq notmuch-hello-first-run nil))
 
 (defun notmuch-folder ()
-- 
1.7.9.5

[PATCH v2 3/5] test: fix hook-counter to accept the new no-display param

[Jani Nikula]

notmuch-hello-refresh-hook is now called with no-display param. Accept
(and ignore) it in hook-counter.
---
 test/test-lib.el |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/test-lib.el b/test/test-lib.el
index fa3380c..864780c 100644
--- a/test/test-lib.el
+++ b/test/test-lib.el
@@ -84,7 +84,7 @@ running, quit if it terminated."
       (kill-emacs)
     (run-at-time "1 min" nil 'orphan-watchdog pid)))
 
-(defun hook-counter (hook)
+(defun hook-counter (hook &rest no-display)
   "Count how many times a hook is called.  Increments
 `hook'-counter variable value if it is bound, otherwise does
 nothing."
-- 
1.7.9.5

[PATCH v2 4/5] emacs: add notmuch hello refresh hook to display message count change

[Jani Nikula]

Add a notmuch hello refresh hook to display a message about change in
message count in the database since the notmuch-hello buffer was last
refreshed manually (no-display is nil).

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 emacs/notmuch-hello.el |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index fa14443..f7a3c95 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -148,6 +148,7 @@ International Bureau of Weights and Measures."
 (defcustom notmuch-hello-refresh-hook nil
   "Functions called after updating a `notmuch-hello' buffer."
   :type 'hook
+  :options '(notmuch-hello-refresh-status-message)
   :group 'notmuch-hello
   :group 'notmuch-hooks)
 
@@ -749,6 +750,43 @@ following:
     (let ((fill-column (- (window-width) notmuch-hello-indent)))
       (center-region start (point)))))
 
+(defcustom notmuch-hello-refresh-status-query "*"
+  "Query to use for `notmuch-hello-refresh-status-message' hook."
+  :type '(choice (const :tag "All messages" "*")
+		 (string :tag "Custom query"
+			 :value "tag:inbox"))
+  :group 'notmuch-hello)
+
+(defvar notmuch-hello-refresh-count 0
+  "Number of matching messages when `notmuch-hello' was last run.
+
+Used internally by `notmuch-hello-refresh-status-message'.")
+
+(defun notmuch-hello-refresh-status-message (no-display)
+  "Hook to display a status message when refreshing notmuch-hello buffer.
+
+Display a status message about the difference in message count
+matching `notmuch-hello-refresh-status-query' since the last time
+notmuch-hello was refreshed. Only takes into account explicit
+refreshes (NO-DISPLAY is nil)."
+  (unless no-display
+    (let* ((new-count
+	    (string-to-number
+	     (car (process-lines notmuch-command "count"
+				 notmuch-hello-refresh-status-query))))
+	   (diff-count (- new-count notmuch-hello-refresh-count)))
+      (cond
+       ((= notmuch-hello-refresh-count 0)
+	(message "You have %s messages."
+		 (notmuch-hello-nice-number new-count)))
+       ((> diff-count 0)
+	(message "You have %s more messages since last refresh."
+		 (notmuch-hello-nice-number diff-count)))
+       ((< diff-count 0)
+	(message "You have %s fewer messages since last refresh."
+		 (notmuch-hello-nice-number (- diff-count)))))
+      (setq notmuch-hello-refresh-count new-count))))
+
 ;;;###autoload
 (defun notmuch-hello (&optional no-display)
   "Run notmuch and display saved searches, known tags, etc.
-- 
1.7.9.5

[PATCH v2 5/5] NEWS: notmuch-hello refresh hook changes

[Jani Nikula]

---
 NEWS |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/NEWS b/NEWS
index 2b50ba3..435a1ab 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,21 @@
+Notmuch 0.15 (YYYY-MM-DD)
+=========================
+
+Emacs Interface
+---------------
+
+`notmuch-hello-refresh-hook` argument change
+
+  The `no-display` argument of `notmuch-hello` is now passed on to the
+  `notmuch-hello-refresh-hook`. Custom hooks need to be changed to accept the
+  argument.
+
+Status message on `notmuch-hello` refresh
+
+  A hook to display a status message about the change in the message count in
+  the database on `notmuch-hello` refresh is provided. The hook can be enabled
+  by customizing `notmuch-hello-refresh-hook`.
+
 Notmuch 0.14 (2012-08-20)
 =========================
 
-- 
1.7.9.5

Re: [PATCH v2 0/5] emacs: notmuch-hello status message refresh hook, etc

[Tomi Ollila]

On Sun, Sep 02 2012, Jani Nikula <jani@nikula.org> wrote:

> Hi all, v2 of [1] addressing review comments: added NEWS, improved
> documentation, made the query customizable.

Hi Jani

Just today I was looking these older patches below and was about to
comment those when these arrived.

I think the idea is great, but there is just something in the integration...

> [1] id:"37899e28dbf67e4620a53279a869be3174c02d6f.1339775602.git.jani@nikula.org"

I see hooks a great way to extend some functionality outside of that
package instead of touching it. Also, maybe I've been using emacs too
much as these hooks as defcustom seems little weird. Yet as I grepped
'defcustom.*hook' through emacs-24.2/lisp/*.el and I got many hits --
and some of those even added some ":options" to those (but most those
I looked did not specify anything to choose from).

That said, I am not _personally_ very thilled about the idea of defining
functions inside notmuch and then that work via a hook...

Therefore I'd like to propose an alternative way to do this:

Integrate this change status information to the notmuch-hello-insert-header ()
function, disabled by default -- and add customization variable which
can be used to enable it. like:

 Welcome to notmuch. You have 22 195 messages.

 Welcome to notmuch. You have 22 185 messages (-10).
 Welcome to notmuch. You have 22 205 messages (+20).

or second line for the status info.


I would enable this feature if implemented this way. Also I would enable
this implemented like this patch series if this is to be applied (I just 
give +-0). But this could also be implemented as a hook outside of notmuch
relying that variable no-display is available in dynamic scope -- but without
promising the variable will be there.

> BR,
> Jani.

Finally, I hope my comment does not silence the audience but that this
sparks more comment and good solution is used for this useful feature.

Tomi

>
> Jani Nikula (5):
>   emacs: document the notmuch-hello no-display argument
>   emacs: add no-display arg to notmuch-hello-refresh-hook
>   test: fix hook-counter to accept the new no-display param
>   emacs: add notmuch hello refresh hook to display message count change
>   NEWS: notmuch-hello refresh hook changes
>
>  NEWS                   |   18 ++++++++++++++++++
>  emacs/notmuch-hello.el |   46 ++++++++++++++++++++++++++++++++++++++++++++--
>  test/test-lib.el       |    2 +-
>  3 files changed, 63 insertions(+), 3 deletions(-)
>
> -- 
> 1.7.9.5
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 3/5] test: fix hook-counter to accept the new no-display param

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

Jani Nikula <jani@nikula.org> writes:
> notmuch-hello-refresh-hook is now called with no-display param. Accept
> (and ignore) it in hook-counter.

This should go together with the previous patch since without this
change, previous patch breaks code.

> ---
>  test/test-lib.el |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/test/test-lib.el b/test/test-lib.el
> index fa3380c..864780c 100644
> --- a/test/test-lib.el
> +++ b/test/test-lib.el
> @@ -84,7 +84,7 @@ running, quit if it terminated."
>        (kill-emacs)
>      (run-at-time "1 min" nil 'orphan-watchdog pid)))
>  
> -(defun hook-counter (hook)
> +(defun hook-counter (hook &rest no-display)
>    "Count how many times a hook is called.  Increments
>  `hook'-counter variable value if it is bound, otherwise does
>  nothing."
> -- 
> 1.7.9.5
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v2 0/5] emacs: notmuch-hello status message refresh hook, etc

[Michal Sojka]

On Sun, Sep 02 2012, Jani Nikula wrote:
> Hi all, v2 of [1] addressing review comments: added NEWS, improved
> documentation, made the query customizable.
>
> [1]
>     id:"37899e28dbf67e4620a53279a869be3174c02d6f.1339775602.git.jani@nikula.org"

Hello Jani,

if I understand correctly, the goal of this patchset is to display the
message with the difference in the number of messages before and after
refresh of notmuch-hello.

I think the current implementation is unnecessarily complicated. It
would be sufficient to implement this directly in `notmuch-hello'
without any hook. If `notmuch-hello-refresh-status-query' is nil
(default) no message would be shown. If it is configured to something
else, the message would be shown provided that no-display is nil. This
way you don't have to break existing user's hooks and achieve the same
behavior, don't you?

-Michal

Re: [PATCH v2 0/5] emacs: notmuch-hello status message refresh hook, etc

[Jani Nikula]

On Tue, 04 Sep 2012, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> On Sun, Sep 02 2012, Jani Nikula wrote:
>> Hi all, v2 of [1] addressing review comments: added NEWS, improved
>> documentation, made the query customizable.
>>
>> [1]
>>     id:"37899e28dbf67e4620a53279a869be3174c02d6f.1339775602.git.jani@nikula.org"
>
> Hello Jani,
>
> if I understand correctly, the goal of this patchset is to display the
> message with the difference in the number of messages before and after
> refresh of notmuch-hello.

Difference between two refreshes of notmuch-hello to be specific.

> I think the current implementation is unnecessarily complicated. It
> would be sufficient to implement this directly in `notmuch-hello'
> without any hook. If `notmuch-hello-refresh-status-query' is nil
> (default) no message would be shown. If it is configured to something
> else, the message would be shown provided that no-display is nil. This
> way you don't have to break existing user's hooks and achieve the same
> behavior, don't you?

I think it would be useful to pass no-display to hooks anyway,
regardless of the use here. I don't see that as a big reason to do this
one way or the other. Having this implemented directly in notmuch-hello
does not make this less complicated either. The only difference would be
having an if clause within notmuch-hello rather than an option in
notmuch-hello-refresh-hook.

This leaves us the matter of style. Tomi also expressed preference for
having this built-in to notmuch-hello rather than as a hook. I like
having it as a hook for a number of reasons:

* I think it provides a nice abstract interface without messing with the
  rest of notmuch-hello.

* I think toggling hooks on and off in the customization interface is
  nice (see e.g. notmuch-show-insert-text/plain-hook under
  notmuch-show). No need to add more and more customizations for things
  that can be options.

* If the user doesn't like the messages, it's easy to copy-paste the
  code to .emacs, modify, and add-hook.

* It serves as an example of what can be done in the notmuch-hello
  hooks.

If you and Tomi still insist on not having this as a hook, changing it
is not a big deal (faster than writing this email *sigh*). But even then
I'd propose merging patches 1-3 as those allow the user to write the
kind of hook he chooses.


BR,
Jani.

Re: [PATCH v2 0/5] emacs: notmuch-hello status message refresh hook, etc

[Michal Sojka]

On Tue, Sep 04 2012, Jani Nikula wrote:
> On Tue, 04 Sep 2012, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
>> On Sun, Sep 02 2012, Jani Nikula wrote:
>>> Hi all, v2 of [1] addressing review comments: added NEWS, improved
>>> documentation, made the query customizable.
>>>
>>> [1]
>>>     id:"37899e28dbf67e4620a53279a869be3174c02d6f.1339775602.git.jani@nikula.org"
>>
>> Hello Jani,
>>
>> if I understand correctly, the goal of this patchset is to display the
>> message with the difference in the number of messages before and after
>> refresh of notmuch-hello.
>
> Difference between two refreshes of notmuch-hello to be specific.
>
>> I think the current implementation is unnecessarily complicated. It
>> would be sufficient to implement this directly in `notmuch-hello'
>> without any hook. If `notmuch-hello-refresh-status-query' is nil
>> (default) no message would be shown. If it is configured to something
>> else, the message would be shown provided that no-display is nil. This
>> way you don't have to break existing user's hooks and achieve the same
>> behavior, don't you?
>
> I think it would be useful to pass no-display to hooks anyway,
> regardless of the use here.

I'm strongly against breaking existing user's setups without a good
reason. The meaning of no-display parameter, as I understand it, is only
to tells whether the refresh should happen in background or foreground.
Isn't there any other method for hooks to check for this conditions? For
example something like checking whether notmuch-hello buffer is shown in
a window of the active frame? Or a simple (perhaps buffer-local)
variable can contain this information.

I don't know what people use notmuch-hello-refresh-hook for. I suppose
that your use case (displaying a message outside of hello buffer) is
rather an exception than a typical use.

> I don't see that as a big reason to do this one way or the other.
> Having this implemented directly in notmuch-hello does not make this
> less complicated either.

That's true, but it would be simpler for users to set it up. With your
patches, users would have to configure the variable (query) as well as
the hook. I propose to have only one place to configure this feature.

-Michal

Re: [PATCH v2 0/5] emacs: notmuch-hello status message refresh hook, etc

[Tomi Ollila]

On Thu, Sep 06 2012, Michal Sojka wrote:

> On Tue, Sep 04 2012, Jani Nikula wrote:
>> On Tue, 04 Sep 2012, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
>>> On Sun, Sep 02 2012, Jani Nikula wrote:
>>>> Hi all, v2 of [1] addressing review comments: added NEWS, improved
>>>> documentation, made the query customizable.
>>>>
>>>> [1]
>>>>     id:"37899e28dbf67e4620a53279a869be3174c02d6f.1339775602.git.jani@nikula.org"
>>>
>>> Hello Jani,
>>>
>>> if I understand correctly, the goal of this patchset is to display the
>>> message with the difference in the number of messages before and after
>>> refresh of notmuch-hello.
>>
>> Difference between two refreshes of notmuch-hello to be specific.
>>
>>> I think the current implementation is unnecessarily complicated. It
>>> would be sufficient to implement this directly in `notmuch-hello'
>>> without any hook. If `notmuch-hello-refresh-status-query' is nil
>>> (default) no message would be shown. If it is configured to something
>>> else, the message would be shown provided that no-display is nil. This
>>> way you don't have to break existing user's hooks and achieve the same
>>> behavior, don't you?
>>
>> I think it would be useful to pass no-display to hooks anyway,
>> regardless of the use here.
>
> I'm strongly against breaking existing user's setups without a good
> reason. The meaning of no-display parameter, as I understand it, is only
> to tells whether the refresh should happen in background or foreground.
> Isn't there any other method for hooks to check for this conditions? For
> example something like checking whether notmuch-hello buffer is shown in
> a window of the active frame? Or a simple (perhaps buffer-local)
> variable can contain this information.

I mentioned using no-display from dynamic scope -- which isn't very
good solution in a long run. Lately I've been leaning to Jani's suggestion
by just calling refresh-hook with no-display arg.... But if there is
well-established way to provide this information to hooks without
adding this argument in the hook call then that could be used..

> I don't know what people use notmuch-hello-refresh-hook for. I suppose
> that your use case (displaying a message outside of hello buffer) is
> rather an exception than a typical use.

In any case, I think the refresh-hook needs to know this no-display
information, somehow. If there is no other good way than giving it
as an argument to the hook then that should be done (by pushing patches
1-3).

>> I don't see that as a big reason to do this one way or the other.
>> Having this implemented directly in notmuch-hello does not make this
>> less complicated either.
>
> That's true, but it would be simpler for users to set it up. With your
> patches, users would have to configure the variable (query) as well as
> the hook. I propose to have only one place to configure this feature.

I've been thinking that Jani's way to provide the information to minibuffer
is a bit better than provide it in hello buffer -- as I think there is a
small "imperfection" that may irritate users: The default '*' is in use.
The hello buffer is refreshed (via emacsclient or something?) with
no-display t -- message count changes but the reference count
(notmuch-hello-refresh-count) keeps the same. Next time the hello buffer is
visible, showing current message count -- and user refreshes it.  In most
cases message count change in hello buffer is different that what will be
shown in minibuffer.

Now, If this "imperfection" reason enough to have this feature available
from wiki/contrib. 

If not, then the decision between hook/part of notmuch-hello is to be done:

1) If hook, then one less if's to be used -- and customization useful for
those who don't (add-hook...) more hooks there.

2) If somewhere in notmuch-hello processing then this "query-variable"
(named appropriately) can be used to check whether this feature is in 
use; if nil then not, otherwise it contains the query variable to be used
in (notmuch-command "count" ...) call.

> -Michal

Tomi

Re: [PATCH v2 2/5] emacs: add no-display arg to notmuch-hello-refresh-hook

[Tomi Ollila]

On Sun, Sep 02 2012, Jani Nikula <jani@nikula.org> wrote:

> Add no-display arg to notmuch-hello-refresh-hook to allow each hook to
> decide what is appropriate when no-display is t, i.e. when not
> switching to the notmuch-hello buffer.
>
> This breaks existing hooks people might have, which will now need to
> accept the argument.
>
> Signed-off-by: Jani Nikula <jani@nikula.org>
> ---
>  emacs/notmuch-hello.el |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 4dfe5d2..fa14443 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -816,7 +816,7 @@ it (i.e. use `set-buffer' rather than `switch-to-buffer')."
>        (unless (widget-at)
>  	(when notmuch-hello-search-pos
>  	  (goto-char notmuch-hello-search-pos)))))
> -  (run-hooks 'notmuch-hello-refresh-hook)
> +  (run-hook-with-args 'notmuch-hello-refresh-hook no-display)
>    (setq notmuch-hello-first-run nil))

I had already written a bit of reply for this (for +1:ng the change), but
luckily fall into some emacs documentation:

http://sunsite.ualberta.ca/Documentation/Gnu/emacs-lisp-ref-20.5/html_node/elisp_357.html

--8<----8<----8<----8<----8<----8<----8<----8<----8<--

23.6 Hooks
==========

...

Most of the hooks in Emacs are normal hooks. These variables contain lists
of functions to be called with no arguments. When the hook name ends in
`-hook', that tells you it is normal. We try to make all hooks normal, as
much as possible, so that you can use them in a uniform way. 

...

If the hook variable's name does not end with `-hook', that indicates it is
probably an abnormal hook; you should look at its documentation to see how
to use the hook properly. 

--8<----8<----8<----8<----8<----8<----8<----8<----8<--

And, indeed, grepping through emacs 24.2 sources when run-hooks-with-args
is used then the variable symbol doesn't end with -hook, for example:

 (run-hook-with-args 'widget-edit-functions widget))


So, Jani -- probably you can use the no-display from the dynamic scope of
the caller in your hook in .emacs -- not nice but...

... or propose a change to the hook name -- or add another hook...

>  (defun notmuch-folder ()
> -- 
> 1.7.9.5


Tomi

Re: [PATCH v2 2/5] emacs: add no-display arg to notmuch-hello-refresh-hook

[Jani Nikula]

On Fri, 07 Sep 2012, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> So, Jani -- probably you can use the no-display from the dynamic scope of
> the caller in your hook in .emacs -- not nice but...

Okay. I'll roll a v2 with the refresh status message displayed directly
from notmuch-hello as suggested by Michal Sojka. I think this is a
useful feature to have upstream, but let's see how that flies.

Jani.