[PATCH 2/3] test: emacs: toggle eliding of non-matching messages in `notmuch-show'

[Mark Walters]

On Sun, 19 Feb 2012, Pieter Praet  wrote:
> See commits 44a544ed, 866ce8b1, 668b66ec.
> ---
>  test/emacs |   38 ++
>  .../notmuch-show-elide-non-matching-messages-off   |   79 
> 
>  .../notmuch-show-elide-non-matching-messages-on|   75 +++
>  3 files changed, 192 insertions(+), 0 deletions(-)
>  create mode 100644 
> test/emacs.expected-output/notmuch-show-elide-non-matching-messages-off
>  create mode 100644 
> test/emacs.expected-output/notmuch-show-elide-non-matching-messages-on

This patch looks good to me and with other possible ways of implementing
elide [1] seems well worth having.

It needs [1/3] which I think is ok but I have never used the crypto
stuff so wouldn't count this as a review of that patch.

Patch [3/3] also looks fine.

Best wishes

Mark

[1] id:"1334077496-9172-1-git-send-email-markwalters1009 at gmail.com"
>
> diff --git a/test/emacs b/test/emacs
> index b207d20..320057a 100755
> --- a/test/emacs
> +++ b/test/emacs
> @@ -553,5 +553,43 @@ test_emacs '(let ((notmuch-crypto-process-mime nil))
>   (test-visible-output))'
>  test_expect_equal_file OUTPUT 
> $EXPECTED/notmuch-show-process-crypto-mime-parts-on
>  
> +test_begin_subtest "notmuch-show: don't elide non-matching messages"
> +test_emacs '(let ((notmuch-show-only-matching-messages nil))
> + (notmuch-search "from:lars at seas.harvard.edu and subject:\"Maildir 
> storage\"")
> + (notmuch-test-wait)
> + (notmuch-search-show-thread)
> + (notmuch-test-wait)
> + (test-visible-output))'
> +test_expect_equal_file OUTPUT 
> $EXPECTED/notmuch-show-elide-non-matching-messages-off
> +
> +test_begin_subtest "notmuch-show: elide non-matching messages"
> +test_emacs '(let ((notmuch-show-only-matching-messages t))
> + (notmuch-search "from:lars at seas.harvard.edu and subject:\"Maildir 
> storage\"")
> + (notmuch-test-wait)
> + (notmuch-search-show-thread)
> + (notmuch-test-wait)
> + (test-visible-output))'
> +test_expect_equal_file OUTPUT 
> $EXPECTED/notmuch-show-elide-non-matching-messages-on
> +
> +test_begin_subtest "notmuch-show: elide non-matching messages (w/ 
> notmuch-show-toggle-elide-non-matching)"
> +test_emacs '(let ((notmuch-show-only-matching-messages nil))
> + (notmuch-search "from:lars at seas.harvard.edu and subject:\"Maildir 
> storage\"")
> + (notmuch-test-wait)
> + (notmuch-search-show-thread)
> + (notmuch-test-wait)
> + (notmuch-show-toggle-elide-non-matching)
> + (test-visible-output))'
> +test_expect_equal_file OUTPUT 
> $EXPECTED/notmuch-show-elide-non-matching-messages-on
> +
> +test_begin_subtest "notmuch-show: elide non-matching messages (w/ prefix arg 
> to notmuch-show)"
> +test_emacs '(let ((notmuch-show-only-matching-messages nil))
> + (notmuch-search "from:lars at seas.harvard.edu and subject:\"Maildir 
> storage\"")
> + (notmuch-test-wait)
> + (let ((current-prefix-arg t))
> +   (notmuch-search-show-thread))
> + (notmuch-test-wait)
> + (test-visible-output))'
> +test_expect_equal_file OUTPUT 
> $EXPECTED/notmuch-show-elide-non-matching-messages-on
> +
>  
>  test_done
> diff --git 
> a/test/emacs.expected-output/notmuch-show-elide-non-matching-messages-off 
> b/test/emacs.expected-output/notmuch-show-elide-non-matching-messages-off
> new file mode 100644
> index 000..b31fe62
> --- /dev/null
> +++ b/test/emacs.expected-output/notmuch-show-elide-non-matching-messages-off
> @@ -0,0 +1,79 @@
> +Lars Kellogg-Stedman  (2009-11-17) (inbox signed)
> +Subject: [notmuch] Working with Maildir storage?
> +To: notmuch at notmuchmail.org
> +Date: Tue, 17 Nov 2009 14:00:54 -0500
> +
> +[ multipart/mixed ]
> +[ multipart/signed ]
> +[ text/plain ]
> +I saw the LWN article and decided to take a look at notmuch.  I'm
> +currently using mutt and mairix to index and read a collection of
> +Maildir mail folders (around 40,000 messages total).
> +
> +notmuch indexed the messages without complaint, but my attempt at
> +searching bombed out. Running, for example:
> +
> +  notmuch search storage
> +
> +Resulted in 4604 lines of errors along the lines of:
> +
> +  Error opening
> +  
> /home/lars/Mail/read-messages.2008/cur/1246413773.24928_27334.hostname,U=3026:2,S:
> +  Too many open files
> +
> +I'm curious if this is expected behavior (i.e., notmuch does not work
> +with Maildir) or if something else is going on.
> +
> +Cheers,
> +
> +[ 4-line signature. Click/Enter to show. ]
> +[ application/pgp-signature ]
> +[ text/plain ]
> +[ 4-line signature. Click/Enter to show. ]
> + Mikhail Gusarov  (2009-11-17) (inbox signed 
> unread)
> +  Lars Kellogg-Stedman  (2009-11-17) (inbox signed)
> +  Subject: Re: [notmuch] Working with Maildir storage?
> +  To: Mikhail Gusarov 
> +  Cc: notmuch at notmuchmail.org
> +  Date: Tue, 17 Nov 2009 15:33:01 -0500
> +
> +  [ multipart/mixed ]
> +  [ multipart/signed ]
> +  [ text/plain ]
> +  > See the patch just 

[RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello

[Tomi Ollila]

On Tue, Apr 17 2012, Jani Nikula  wrote:

> On Tue, 17 Apr 2012 13:04:39 +0400, Dmitry Kurochkin  gmail.com> wrote:
>> Hi Jani.
>> 
>> Jani Nikula  writes:

[ stuff deleted ]

> I don't oppose to your plan, but I don't think I'm up to implementing it
> either. I just cooked up something together to fix the #1 annoyance I've
> lately had with notmuch, and Tomi persuaded me to share the patches as
> RFC. I still think it's pretty good, considering the simplicity of it
> all, but certainly not perfect.

Thank you. Last night I spent something like 30 min in bed thinking of
this issue; Now I have more ideas to think of. I'll provide my feedback
in another RFC patch based on all of these ideas (I think this is most
appreciated by Jani as it reduces his workload :D)

>
> BR,
> Jani.
>
>> 
>> Regards,
>>   Dmitry
>> 

Tomi

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

[Dmitry Kurochkin]

Jani Nikula  writes:

> On Tue, 17 Apr 2012 13:13:15 +0400, Dmitry Kurochkin  gmail.com> wrote:
>> Jani Nikula  writes:
>> 
>> > 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).
>> 
>> I like this idea.  But IMO we should avoid another call to notmuch
>> count.  Notmuch-hello buffer already displays the message count on the
>> first line.  I would propose to implement this functionality not as a
>> hook but as part of the section which outputs the first line.  We can
>> add an option to disable it if you prefer but I do not think it is
>> needed.  This is less flexible than a hook, but IMO it is not a big
>> issue.
>
> And what if someone has disabled the header section but would want the
> message?

Well, that is what I meant by "less flexible than a hook".

> Also, you'd have to pass no-display there too.

Sure, we can pass it to sections.

> IMHO one call to
> notmuch count is not a big issue, especially for an optional feature.
> And having it as a hook very nicely isolates the feature from everything
> else.
>

I think this feature should be enabled by default.

I guess you are right that it is not a big issue.  I still think we
would be better without it (and we still can isolate the feature), but I
would not object to having the extra call.

Regards,
  Dmitry

> Jani.
>
>
>> 
>> Regards,
>>   Dmitry
>> 
>> > ---
>> >  emacs/notmuch-hello.el |   23 +++
>> >  1 files changed, 23 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
>> > index 0596bbe..13da146 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)
>> >  
>> > @@ -729,6 +730,28 @@ following:
>> >  (let ((fill-column (- (window-width) notmuch-hello-indent)))
>> >(center-region start (point)
>> >  
>> > +(defvar notmuch-hello-refresh-count 0
>> > +  "Number of messages in the database 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."
>> > +  (unless no-display
>> > +(let* ((new-count
>> > +  (string-to-number (car (process-lines notmuch-command "count"
>> > + (diff-count (- new-count notmuch-hello-refresh-count)))
>> > +  (if (= notmuch-hello-refresh-count 0)
>> > +(message "You have %s messages."
>> > + (notmuch-hello-nice-number new-count))
>> > +  (if (not (= diff-count 0))
>> > +  (if (>= diff-count 0)
>> > +  (message "You have %s more messages since last refresh."
>> > +   (notmuch-hello-nice-number diff-count))
>> > +(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 ( no-display)
>> >"Run notmuch and display saved searches, known tags, etc."
>> > -- 
>> > 1.7.1
>> >
>> > ___
>> > notmuch mailing list
>> > notmuch at notmuchmail.org
>> > http://notmuchmail.org/mailman/listinfo/notmuch

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

[Dmitry Kurochkin]

Jani Nikula  writes:

> On Tue, 17 Apr 2012 13:06:13 +0400, Dmitry Kurochkin  gmail.com> wrote:
>> Can you please give some explanaition why is this needed?  Would this
>> change break custom hooks that do not expect any arguments?
>
> For patch 3/4. We don't want to display a message if someone calls
> (notmuch-hello-update t) from some script, and notmuch-hello buffer
> isn't even visible.
>
> Yes, it would break custom hooks. And a bunch of tests. But I think it
> would be useful for custom hooks too, for the same reason as above.
>

Makes sense.  I think it should be mentioned in the commit message.
Especially the fact that it breaks existing hooks.  We should mention it
in the NEWS later as well.

Regards,
  Dmitry

> Jani.
>
>
>
>> 
>> Regards,
>>   Dmitry
>> 
>> Jani Nikula  writes:
>> 
>> > ---
>> >  emacs/notmuch-hello.el |2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
>> > index 9cd907a..0596bbe 100644
>> > --- a/emacs/notmuch-hello.el
>> > +++ b/emacs/notmuch-hello.el
>> > @@ -776,7 +776,7 @@ following:
>> >  (widget-setup)
>> >  
>> >  (goto-char final-target-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.1
>> >
>> > ___
>> > notmuch mailing list
>> > notmuch at notmuchmail.org
>> > http://notmuchmail.org/mailman/listinfo/notmuch

[RFC PATCH 4/4] emacs: add notmuch hello refresh hook to place point in a widget

[Dmitry Kurochkin]

Jani Nikula  writes:

> Add support for putting point to a widget after refresh through a
> hook. This approximates the old behaviour.

I may be wrong, but this looks to me like a hack that cannot work well.
See my first reply in the thread for ideas on how to better implement
this functionality.

Regards,
  Dmitry

> ---
>  emacs/notmuch-hello.el |8 +++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 13da146..07e64d4 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -148,7 +148,8 @@ 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)
> +  :options '(notmuch-hello-refresh-status-message
> +  notmuch-hello-refresh-point-to-widget)
>:group 'notmuch-hello
>:group 'notmuch-hooks)
>  
> @@ -752,6 +753,11 @@ Used internally by 
> `notmuch-hello-refresh-status-message'.")
>  (notmuch-hello-nice-number (- diff-count))
>(setq notmuch-hello-refresh-count new-count
>  
> +(defun notmuch-hello-refresh-point-to-widget (no-display)
> +  "Hook to place point to widget after notmuch-hello refresh."
> +  (widget-backward 1)
> +  (widget-forward 1))
> +
>  ;;;###autoload
>  (defun notmuch-hello ( no-display)
>"Run notmuch and display saved searches, known tags, etc."
> -- 
> 1.7.1
>
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

[Dmitry Kurochkin]

Jani Nikula  writes:

> 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).

I like this idea.  But IMO we should avoid another call to notmuch
count.  Notmuch-hello buffer already displays the message count on the
first line.  I would propose to implement this functionality not as a
hook but as part of the section which outputs the first line.  We can
add an option to disable it if you prefer but I do not think it is
needed.  This is less flexible than a hook, but IMO it is not a big
issue.

Regards,
  Dmitry

> ---
>  emacs/notmuch-hello.el |   23 +++
>  1 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 0596bbe..13da146 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)
>  
> @@ -729,6 +730,28 @@ following:
>  (let ((fill-column (- (window-width) notmuch-hello-indent)))
>(center-region start (point)
>  
> +(defvar notmuch-hello-refresh-count 0
> +  "Number of messages in the database 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."
> +  (unless no-display
> +(let* ((new-count
> + (string-to-number (car (process-lines notmuch-command "count"
> +(diff-count (- new-count notmuch-hello-refresh-count)))
> +  (if (= notmuch-hello-refresh-count 0)
> +   (message "You have %s messages."
> +(notmuch-hello-nice-number new-count))
> + (if (not (= diff-count 0))
> + (if (>= diff-count 0)
> + (message "You have %s more messages since last refresh."
> +  (notmuch-hello-nice-number diff-count))
> +   (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 ( no-display)
>"Run notmuch and display saved searches, known tags, etc."
> -- 
> 1.7.1
>
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

[Dmitry Kurochkin]

Can you please give some explanaition why is this needed?  Would this
change break custom hooks that do not expect any arguments?

Regards,
  Dmitry

Jani Nikula  writes:

> ---
>  emacs/notmuch-hello.el |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 9cd907a..0596bbe 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -776,7 +776,7 @@ following:
>  (widget-setup)
>  
>  (goto-char final-target-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.1
>
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello

[Dmitry Kurochkin]

Hi Jani.

Jani Nikula  writes:

> notmuch-hello (called also through notmuch-hello-update, bound to '='
> by default) tries to find the widget under or following point before
> refresh, and put the point back to the widget afterwards. The code has
> gotten a bit complicated, and has at least the following issues:
>
> 1) All the individual section functions have to include code to
>support point placement. If there is no such support, point is
>dropped to the search box. Only saved searches and all tags
>sections support point placement.
>
> 2) Point placement is based on widget-value. If there are two widgets
>with the same widget-value (for example a saved search with the
>same name as a tag) the point is moved to the earlier one.
>
> 3) When first entering notmuch-hello notmuch-hello-target is nil, and
>point is dropped to the search box.
>
> This patch simplifies the code by removing all point placement based
> on widgets. Point is simply saved before refresh, and put back to
> where it was. Sometimes, but not very often, this would have the
> appearance of moving the point relative to the nearest widgets. IMHO
> this is a minor problem compared to the issues listed above.
>
> A downside is that there's no visual cue (point movement) to indicate
> that refresh has finished. Then again, neither was there before, if
> point was at the beginning of a widget.

Thanks for looking into this.  This is an annoying issue indeed.  And I
was thinking about fixing it myself.

I am not sure I like the approach of moving the cursor to the same
position.  It is common that buffer content would change significantly
after a refresh (e.g. after I archived all new mail).  That would mean
the cursor would just randomly jump somewhere.  IMO we should allow
smart cursor positioning which means that logic should go to individual
sections.  I would propose the following plan:

1. Remove special case for search box.  No section should be special.
   Moreover it is possible to remove it (I did it) and in that case the
   cursor would be left at the end of the buffer.  By default, the
   cursor should be moved to the beginning of the buffer.

2. Replace current cursor positioning logic with section specific code.
   I.e. `notmuch-hello' would not do any cursor positioning (except for
   item 1) but queries and tags section would save required state when a
   button is clicked and the same section would use this state to
   restore cursor position on refresh.  What state should be saved would
   depend on the section but we should at least save the section
   name/ID.  If during refresh no section set the cursor position, then
   the cursor is moved to the beginning of the buffer.

3. Provide a custom variable to set the default section to move the
   cursor to.  I.e. set the section name/ID part of the state from item
   2.  Again, details on what the default position inside the section is
   would depend on the section.  For search box, it would be the input
   field.  For queries/tags it would be the first tag.

Item 1 is pretty simple.  The rest may be more tricky.  What do you
think?

Regards,
  Dmitry

> ---
>  emacs/notmuch-hello.el |   70 +++
>  1 files changed, 17 insertions(+), 53 deletions(-)
>
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index 71d37b8..9cd907a 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -154,11 +154,6 @@ International Bureau of Weights and Measures."
>  (defvar notmuch-hello-url "http://notmuchmail.org;
>"The `notmuch' web site.")
>  
> -(defvar notmuch-hello-search-pos nil
> -  "Position of search widget, if any.
> -
> -This should only be set by `notmuch-hello-insert-search'.")
> -
>  (defvar notmuch-hello-custom-section-options
>'((:filter (string :tag "Filter for each tag"))
>  (:filter-count (string :tag "Different filter to generate message 
> counts"))
> @@ -209,11 +204,8 @@ function produces a section simply by adding content to 
> the current
>  buffer.  A section should not end with an empty line, because a
>  newline will be inserted after each section by `notmuch-hello'.
>  
> -Each function should take no arguments.  If the produced section
> -includes `notmuch-hello-target' (i.e. cursor should be positioned
> -inside this section), the function should return this element's
> -position.
> -Otherwise, it should return nil.
> +Each function should take no arguments. The return value is
> +ignored.
>  
>  For convenience an element can also be a list of the form (FUNC ARG1
>  ARG2 .. ARGN) in which case FUNC will be applied to the rest of the
> @@ -240,15 +232,6 @@ supported for \"Customized queries section\" items."
>   notmuch-hello-query-section
>   (function :tag "Custom section"
>  
> -(defvar notmuch-hello-target nil
> -  "Button text at position of point before rebuilding the notmuch-buffer.
> -
> -This variable contains the text of the 

[RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello

[Jani Nikula]

On Tue, 17 Apr 2012 13:04:39 +0400, Dmitry Kurochkin  wrote:
> Hi Jani.
> 
> Jani Nikula  writes:
> 
> > notmuch-hello (called also through notmuch-hello-update, bound to '='
> > by default) tries to find the widget under or following point before
> > refresh, and put the point back to the widget afterwards. The code has
> > gotten a bit complicated, and has at least the following issues:
> >
> > 1) All the individual section functions have to include code to
> >support point placement. If there is no such support, point is
> >dropped to the search box. Only saved searches and all tags
> >sections support point placement.
> >
> > 2) Point placement is based on widget-value. If there are two widgets
> >with the same widget-value (for example a saved search with the
> >same name as a tag) the point is moved to the earlier one.
> >
> > 3) When first entering notmuch-hello notmuch-hello-target is nil, and
> >point is dropped to the search box.
> >
> > This patch simplifies the code by removing all point placement based
> > on widgets. Point is simply saved before refresh, and put back to
> > where it was. Sometimes, but not very often, this would have the
> > appearance of moving the point relative to the nearest widgets. IMHO
> > this is a minor problem compared to the issues listed above.
> >
> > A downside is that there's no visual cue (point movement) to indicate
> > that refresh has finished. Then again, neither was there before, if
> > point was at the beginning of a widget.
> 
> Thanks for looking into this.  This is an annoying issue indeed.  And I
> was thinking about fixing it myself.
> 
> I am not sure I like the approach of moving the cursor to the same
> position.  It is common that buffer content would change significantly
> after a refresh (e.g. after I archived all new mail).  That would mean
> the cursor would just randomly jump somewhere.  IMO we should allow
> smart cursor positioning which means that logic should go to individual
> sections.
>  I would propose the following plan:
> 
> 1. Remove special case for search box.  No section should be special.
>Moreover it is possible to remove it (I did it) and in that case the
>cursor would be left at the end of the buffer.  By default, the
>cursor should be moved to the beginning of the buffer.
> 
> 2. Replace current cursor positioning logic with section specific code.
>I.e. `notmuch-hello' would not do any cursor positioning (except for
>item 1) but queries and tags section would save required state when a
>button is clicked and the same section would use this state to
>restore cursor position on refresh.  What state should be saved would
>depend on the section but we should at least save the section
>name/ID.  If during refresh no section set the cursor position, then
>the cursor is moved to the beginning of the buffer.
> 
> 3. Provide a custom variable to set the default section to move the
>cursor to.  I.e. set the section name/ID part of the state from item
>2.  Again, details on what the default position inside the section is
>would depend on the section.  For search box, it would be the input
>field.  For queries/tags it would be the first tag.
> 
> Item 1 is pretty simple.  The rest may be more tricky.  What do you
> think?

My approach was this: keep it extremely simple, and catch the low
hanging fruit. It places the point where I want, say, 90% of the
time. And when it's wrong, it's not far off. In contrast, the current
implementation places the cursor exactly where I do *not* want it about
half the time.

What you describe sounds smart, but complicated. Maybe it just sounds
complicated from my limited lisp skills perspective. Personally I don't
think sections should have to implement their own logic for point
placement. And as a fallback, I prefer keeping the point where it is
instead of moving it to the beginning of buffer.

I don't oppose to your plan, but I don't think I'm up to implementing it
either. I just cooked up something together to fix the #1 annoyance I've
lately had with notmuch, and Tomi persuaded me to share the patches as
RFC. I still think it's pretty good, considering the simplicity of it
all, but certainly not perfect.


BR,
Jani.


> 
> Regards,
>   Dmitry
> 
> > ---
> >  emacs/notmuch-hello.el |   70 
> > +++
> >  1 files changed, 17 insertions(+), 53 deletions(-)
> >
> > diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> > index 71d37b8..9cd907a 100644
> > --- a/emacs/notmuch-hello.el
> > +++ b/emacs/notmuch-hello.el
> > @@ -154,11 +154,6 @@ International Bureau of Weights and Measures."
> >  (defvar notmuch-hello-url "http://notmuchmail.org;
> >"The `notmuch' web site.")
> >  
> > -(defvar notmuch-hello-search-pos nil
> > -  "Position of search widget, if any.
> > -
> > -This should only be set by `notmuch-hello-insert-search'.")
> > -
> >  (defvar 

[RFC PATCH 4/4] emacs: add notmuch hello refresh hook to place point in a widget

[Jani Nikula]

On Tue, 17 Apr 2012 13:16:10 +0400, Dmitry Kurochkin  wrote:
> Jani Nikula  writes:
> 
> > Add support for putting point to a widget after refresh through a
> > hook. This approximates the old behaviour.
> 
> I may be wrong, but this looks to me like a hack that cannot work well.
> See my first reply in the thread for ideas on how to better implement
> this functionality.

This isn't very much unlike how the current code finds a widget before
refreshing. The difference is that this is based on a saved and restored
point, which indeed does have it's inaccuracies.

Jani.

> 
> Regards,
>   Dmitry
> 
> > ---
> >  emacs/notmuch-hello.el |8 +++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> > index 13da146..07e64d4 100644
> > --- a/emacs/notmuch-hello.el
> > +++ b/emacs/notmuch-hello.el
> > @@ -148,7 +148,8 @@ 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)
> > +  :options '(notmuch-hello-refresh-status-message
> > +notmuch-hello-refresh-point-to-widget)
> >:group 'notmuch-hello
> >:group 'notmuch-hooks)
> >  
> > @@ -752,6 +753,11 @@ Used internally by 
> > `notmuch-hello-refresh-status-message'.")
> >(notmuch-hello-nice-number (- diff-count))
> >(setq notmuch-hello-refresh-count new-count
> >  
> > +(defun notmuch-hello-refresh-point-to-widget (no-display)
> > +  "Hook to place point to widget after notmuch-hello refresh."
> > +  (widget-backward 1)
> > +  (widget-forward 1))
> > +
> >  ;;;###autoload
> >  (defun notmuch-hello ( no-display)
> >"Run notmuch and display saved searches, known tags, etc."
> > -- 
> > 1.7.1
> >
> > ___
> > notmuch mailing list
> > notmuch at notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 1/7] Split notmuch_database_close into two functions

[Mark Walters]

On Mon, 16 Apr 2012, Justus Winter <4winter at informatik.uni-hamburg.de> wrote:
> Quoting Mark Walters (2012-03-31 19:17:15)
>> Secondly, I think the patch series could be made clearer and easier to
>> review. If you do it in three steps
>> 
>> 1) change of notmuch_database_close to notmuch_database_destroy (just
>>the function name change)
>> 2) split the new notmuch_database_destroy into two as in the current
>>first patch
>> 3) Make any changes (if there are any) of notmuch_database_destroy to
>>notmuch_database_close.
>> 
>> The advantage is that the first change is easy to test (essentially does
>> it build) and then changes from notmuch_database_destroy to
>> notmuch_database_close in step 3 are explicit rather than the current
>> situation where we need to grep the code to see if some instances of
>> notmuch_database_close were not changed to notmuch_database_destroy.
>
> I don't buy it. The patch series first touches the library and
> documentation and the lib compiles fine. The next patch updates the
> cli tools, all of them compile fine afterwards.
>
> Every patch addresses the issue component wise, this seems rather
> natural for me.

I will try an explain my concern better. I assume that the patch
actually introduces a functional change : that is something somewhere in
the code calls the new notmuch_database_close instead of
notmuch_database_destroy [1]. In your current patch series someone
reading the patches alone can't see the functional change: it comes from
the occurrences of notmuch_database_close that you *don't* change to
notmuch_database_destroy.

Indeed, if the only change is to allow out-of-tree code access to the
new notmuch_database_close function then doing the patch series as

rename notmuch_database_close to notmuch_database_destroy

Then split notmuch_database_destroy make it clearer. (And if the code
compiles after step 1 then I know *all* occurrences of
notmuch_database_close have been changed to notmuch_database_destroy).


Best wishes

Mark

[1] Apart, of course, from notmuch_database_destroy.

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

[Jani Nikula]

On Tue, 17 Apr 2012 13:13:15 +0400, Dmitry Kurochkin  wrote:
> Jani Nikula  writes:
> 
> > 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).
> 
> I like this idea.  But IMO we should avoid another call to notmuch
> count.  Notmuch-hello buffer already displays the message count on the
> first line.  I would propose to implement this functionality not as a
> hook but as part of the section which outputs the first line.  We can
> add an option to disable it if you prefer but I do not think it is
> needed.  This is less flexible than a hook, but IMO it is not a big
> issue.

And what if someone has disabled the header section but would want the
message? Also, you'd have to pass no-display there too. IMHO one call to
notmuch count is not a big issue, especially for an optional feature.
And having it as a hook very nicely isolates the feature from everything
else.

Jani.


> 
> Regards,
>   Dmitry
> 
> > ---
> >  emacs/notmuch-hello.el |   23 +++
> >  1 files changed, 23 insertions(+), 0 deletions(-)
> >
> > diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> > index 0596bbe..13da146 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)
> >  
> > @@ -729,6 +730,28 @@ following:
> >  (let ((fill-column (- (window-width) notmuch-hello-indent)))
> >(center-region start (point)
> >  
> > +(defvar notmuch-hello-refresh-count 0
> > +  "Number of messages in the database 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."
> > +  (unless no-display
> > +(let* ((new-count
> > +   (string-to-number (car (process-lines notmuch-command "count"
> > +  (diff-count (- new-count notmuch-hello-refresh-count)))
> > +  (if (= notmuch-hello-refresh-count 0)
> > + (message "You have %s messages."
> > +  (notmuch-hello-nice-number new-count))
> > +   (if (not (= diff-count 0))
> > +   (if (>= diff-count 0)
> > +   (message "You have %s more messages since last refresh."
> > +(notmuch-hello-nice-number diff-count))
> > + (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 ( no-display)
> >"Run notmuch and display saved searches, known tags, etc."
> > -- 
> > 1.7.1
> >
> > ___
> > notmuch mailing list
> > notmuch at notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

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

[Jani Nikula]

On Tue, 17 Apr 2012 13:06:13 +0400, Dmitry Kurochkin  wrote:
> Can you please give some explanaition why is this needed?  Would this
> change break custom hooks that do not expect any arguments?

For patch 3/4. We don't want to display a message if someone calls
(notmuch-hello-update t) from some script, and notmuch-hello buffer
isn't even visible.

Yes, it would break custom hooks. And a bunch of tests. But I think it
would be useful for custom hooks too, for the same reason as above.

Jani.



> 
> Regards,
>   Dmitry
> 
> Jani Nikula  writes:
> 
> > ---
> >  emacs/notmuch-hello.el |2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> > index 9cd907a..0596bbe 100644
> > --- a/emacs/notmuch-hello.el
> > +++ b/emacs/notmuch-hello.el
> > @@ -776,7 +776,7 @@ following:
> >  (widget-setup)
> >  
> >  (goto-char final-target-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.1
> >
> > ___
> > notmuch mailing list
> > notmuch at notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] configure: test shell parameter substring processing and possibly exec one

[Tomi Ollila]

'configure' script uses parameter substring extensively. It is Posix shell
feature. Original Bourne shell does not have such features. Some systems
still ships such shells as /bin/sh (for compatibility reasons -- shell
scripts written on those platforms are expected to work on 1990's systems...)

To tackle this situation the beginning of configure attemts to do a silent
parameter substitution in a subshell; in case this fails the subshell exits
with nonzero value which is easy to detect.

The || constructs are used twice. The first one is used as Bourne shell
chokes on 'if ! ... ' construct (and if ...; then :; else do_things; fi
looks stupid). The second one(liner) takes care of the possible future
addition of 'set -eu' in the beginning of this script.
---

This patch obsoleted id:"1334589199-25894-1-git-send-email-tomi.ollila at 
iki.fi"
diff:
-+  NOTMUCH_CONFIGURE=1; export _NOTMUCH_CONFIGURE
++  _NOTMUCH_CONFIGURE=1; export _NOTMUCH_CONFIGURE


This patch obsoletes id:"133395-10469-2-git-send-email-Vladimir.Marek at 
oracle.com"

 configure |   14 ++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 71981b7..06fbeff 100755
--- a/configure
+++ b/configure
@@ -1,6 +1,19 @@
 #! /bin/sh

+# Test whether this sh is capable of parameter substring processing.
+# If not, attempt to locate and launch one which probably can.
+( option=option=value; : ${option#*=} ) 2>/dev/null || {
+if test x"${_NOTMUCH_CONFIGURE-}" = x ; then
+   _NOTMUCH_CONFIGURE=1; export _NOTMUCH_CONFIGURE
+   for x in /bin/ksh /bin/bash /usr/bin/bash
+   do test ! -x "$x" || exec "$x" "$0" "$@"
+   done
+fi
+echo "Cannot find compatible shell to execute '$0'" >&2
+exit 1
+}
+unset _NOTMUCH_CONFIGURE
+
 # Store original IFS value so it can be changed (and restored) in many places.
 readonly DEFAULT_IFS=$IFS

-- 
1.7.8.2

[RFC PATCH 4/4] emacs: add notmuch hello refresh hook to place point in a widget

[Jani Nikula]

Add support for putting point to a widget after refresh through a
hook. This approximates the old behaviour.
---
 emacs/notmuch-hello.el |8 +++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 13da146..07e64d4 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -148,7 +148,8 @@ 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)
+  :options '(notmuch-hello-refresh-status-message
+notmuch-hello-refresh-point-to-widget)
   :group 'notmuch-hello
   :group 'notmuch-hooks)

@@ -752,6 +753,11 @@ Used internally by 
`notmuch-hello-refresh-status-message'.")
   (notmuch-hello-nice-number (- diff-count))
   (setq notmuch-hello-refresh-count new-count

+(defun notmuch-hello-refresh-point-to-widget (no-display)
+  "Hook to place point to widget after notmuch-hello refresh."
+  (widget-backward 1)
+  (widget-forward 1))
+
 ;;;###autoload
 (defun notmuch-hello ( no-display)
   "Run notmuch and display saved searches, known tags, etc."
-- 
1.7.1

[RFC PATCH 3/4] 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).
---
 emacs/notmuch-hello.el |   23 +++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 0596bbe..13da146 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)

@@ -729,6 +730,28 @@ following:
 (let ((fill-column (- (window-width) notmuch-hello-indent)))
   (center-region start (point)

+(defvar notmuch-hello-refresh-count 0
+  "Number of messages in the database 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."
+  (unless no-display
+(let* ((new-count
+   (string-to-number (car (process-lines notmuch-command "count"
+  (diff-count (- new-count notmuch-hello-refresh-count)))
+  (if (= notmuch-hello-refresh-count 0)
+ (message "You have %s messages."
+  (notmuch-hello-nice-number new-count))
+   (if (not (= diff-count 0))
+   (if (>= diff-count 0)
+   (message "You have %s more messages since last refresh."
+(notmuch-hello-nice-number diff-count))
+ (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 ( no-display)
   "Run notmuch and display saved searches, known tags, etc."
-- 
1.7.1

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

[Jani Nikula]

---
 emacs/notmuch-hello.el |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 9cd907a..0596bbe 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -776,7 +776,7 @@ following:
 (widget-setup)

 (goto-char final-target-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.1

[RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello

[Jani Nikula]

notmuch-hello (called also through notmuch-hello-update, bound to '='
by default) tries to find the widget under or following point before
refresh, and put the point back to the widget afterwards. The code has
gotten a bit complicated, and has at least the following issues:

1) All the individual section functions have to include code to
   support point placement. If there is no such support, point is
   dropped to the search box. Only saved searches and all tags
   sections support point placement.

2) Point placement is based on widget-value. If there are two widgets
   with the same widget-value (for example a saved search with the
   same name as a tag) the point is moved to the earlier one.

3) When first entering notmuch-hello notmuch-hello-target is nil, and
   point is dropped to the search box.

This patch simplifies the code by removing all point placement based
on widgets. Point is simply saved before refresh, and put back to
where it was. Sometimes, but not very often, this would have the
appearance of moving the point relative to the nearest widgets. IMHO
this is a minor problem compared to the issues listed above.

A downside is that there's no visual cue (point movement) to indicate
that refresh has finished. Then again, neither was there before, if
point was at the beginning of a widget.
---
 emacs/notmuch-hello.el |   70 +++
 1 files changed, 17 insertions(+), 53 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 71d37b8..9cd907a 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -154,11 +154,6 @@ International Bureau of Weights and Measures."
 (defvar notmuch-hello-url "http://notmuchmail.org;
   "The `notmuch' web site.")

-(defvar notmuch-hello-search-pos nil
-  "Position of search widget, if any.
-
-This should only be set by `notmuch-hello-insert-search'.")
-
 (defvar notmuch-hello-custom-section-options
   '((:filter (string :tag "Filter for each tag"))
 (:filter-count (string :tag "Different filter to generate message counts"))
@@ -209,11 +204,8 @@ function produces a section simply by adding content to 
the current
 buffer.  A section should not end with an empty line, because a
 newline will be inserted after each section by `notmuch-hello'.

-Each function should take no arguments.  If the produced section
-includes `notmuch-hello-target' (i.e. cursor should be positioned
-inside this section), the function should return this element's
-position.
-Otherwise, it should return nil.
+Each function should take no arguments. The return value is
+ignored.

 For convenience an element can also be a list of the form (FUNC ARG1
 ARG2 .. ARGN) in which case FUNC will be applied to the rest of the
@@ -240,15 +232,6 @@ supported for \"Customized queries section\" items."
notmuch-hello-query-section
(function :tag "Custom section"

-(defvar notmuch-hello-target nil
-  "Button text at position of point before rebuilding the notmuch-buffer.
-
-This variable contains the text of the button, if any, the
-point was positioned at before the notmuch-hello buffer was
-rebuilt. This should never actually be global and is defined as a
-defvar only for documentation purposes and to avoid a compiler
-warning about it occurring as a free variable.")
-
 (defvar notmuch-hello-hidden-sections nil
   "List of sections titles whose contents are hidden")

@@ -449,8 +432,6 @@ Such a list can be computed with 
`notmuch-hello-query-counts'."
 (msg-count (third elem)))
(widget-insert (format "%8s "
   (notmuch-hello-nice-number msg-count)))
-   (if (string= name notmuch-hello-target)
-   (setq found-target-pos (point-marker)))
(widget-create 'push-button
   :notify #'notmuch-hello-widget-search
   :notmuch-search-terms query
@@ -589,7 +570,6 @@ Complete list of currently available key bindings:
 (defun notmuch-hello-insert-search ()
   "Insert a search widget."
   (widget-insert "Search: ")
-  (setq notmuch-hello-search-pos (point-marker))
   (widget-create 'editable-field
 ;; Leave some space at the start and end of the
 ;; search boxes.
@@ -763,13 +743,7 @@ following:
   (set-buffer "*notmuch-hello*")
 (switch-to-buffer "*notmuch-hello*"))

-  (let ((notmuch-hello-target (if (widget-at)
- (widget-value (widget-at))
-   (condition-case nil
-   (progn
- (widget-forward 1)
- (widget-value (widget-at)))
- (error nil
+  (let ((final-target-pos (point))
(inhibit-read-only t))

 ;; Delete all editable widget fields.  Editable widget fields are
@@ -788,30 +762,20 @@ following:
  

[RFC] Split notmuch_database_close into two functions

[Tomi Ollila]

On Tue, Apr 17 2012, Justus Winter <4winter at informatik.uni-hamburg.de> wrote:

> Hi everyone,
>
> Quoting Patrick Totzke (2012-04-13 10:33:58)
>> Quoting Austin Clements (2012-04-01 04:23:23)
>> >Maybe you could describe your use case in more detail?
>> 
>> Quoting Austin Clements (2012-04-12 17:57:44)
>> >Quoth Justus Winter on Apr 12 at 11:05 am:
>> ...
>> >I see.  TL;DR
>> 
>> .. which should pretty much settle Austins opinion on
>> libnotmuch users being second class citizens.
>
> Na, I think you misinterpreted Austin here, I think he summarized his
> position. Looking back at my mail I think I came across a lot harsher
> than necessary, I'm sorry for that. Let's get back to the issue at
> hand, shall we?
>
> Both Austin and Mark seem to support the split, any other opinions?

I support it too (as did earlyer). Do we already have pushable series
or is there a need for a new one ? It would be good to have this done
so that application writers can start relying this being available
in "official" notmuch (in 0.13 -- SONAME updated -- any other patches
pending requiring SONAME bump to be reviewed ?).

>
> Cheers,
> Justus

Tomi

[PATCH 1/7] Split notmuch_database_close into two functions

[Justus Winter]

Quoting Mark Walters (2012-03-31 19:17:15)
> Secondly, I think the patch series could be made clearer and easier to
> review. If you do it in three steps
> 
> 1) change of notmuch_database_close to notmuch_database_destroy (just
>the function name change)
> 2) split the new notmuch_database_destroy into two as in the current
>first patch
> 3) Make any changes (if there are any) of notmuch_database_destroy to
>notmuch_database_close.
> 
> The advantage is that the first change is easy to test (essentially does
> it build) and then changes from notmuch_database_destroy to
> notmuch_database_close in step 3 are explicit rather than the current
> situation where we need to grep the code to see if some instances of
> notmuch_database_close were not changed to notmuch_database_destroy.

I don't buy it. The patch series first touches the library and
documentation and the lib compiles fine. The next patch updates the
cli tools, all of them compile fine afterwards.

Every patch addresses the issue component wise, this seems rather
natural for me.

Cheers,
Justus

[RFC] Split notmuch_database_close into two functions

[Justus Winter]

Hi everyone,

Quoting Patrick Totzke (2012-04-13 10:33:58)
> Quoting Austin Clements (2012-04-01 04:23:23)
> >Maybe you could describe your use case in more detail?
> 
> Quoting Austin Clements (2012-04-12 17:57:44)
> >Quoth Justus Winter on Apr 12 at 11:05 am:
> ...
> >I see.  TL;DR
> 
> .. which should pretty much settle Austins opinion on
> libnotmuch users being second class citizens.

Na, I think you misinterpreted Austin here, I think he summarized his
position. Looking back at my mail I think I came across a lot harsher
than necessary, I'm sorry for that. Let's get back to the issue at
hand, shall we?

Both Austin and Mark seem to support the split, any other opinions?

Cheers,
Justus

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

[Jani Nikula]

---
 emacs/notmuch-hello.el |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 9cd907a..0596bbe 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -776,7 +776,7 @@ following:
 (widget-setup)
 
 (goto-char final-target-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.1

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello

[Jani Nikula]

notmuch-hello (called also through notmuch-hello-update, bound to '='
by default) tries to find the widget under or following point before
refresh, and put the point back to the widget afterwards. The code has
gotten a bit complicated, and has at least the following issues:

1) All the individual section functions have to include code to
   support point placement. If there is no such support, point is
   dropped to the search box. Only saved searches and all tags
   sections support point placement.

2) Point placement is based on widget-value. If there are two widgets
   with the same widget-value (for example a saved search with the
   same name as a tag) the point is moved to the earlier one.

3) When first entering notmuch-hello notmuch-hello-target is nil, and
   point is dropped to the search box.

This patch simplifies the code by removing all point placement based
on widgets. Point is simply saved before refresh, and put back to
where it was. Sometimes, but not very often, this would have the
appearance of moving the point relative to the nearest widgets. IMHO
this is a minor problem compared to the issues listed above.

A downside is that there's no visual cue (point movement) to indicate
that refresh has finished. Then again, neither was there before, if
point was at the beginning of a widget.
---
 emacs/notmuch-hello.el |   70 +++
 1 files changed, 17 insertions(+), 53 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 71d37b8..9cd907a 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -154,11 +154,6 @@ International Bureau of Weights and Measures.
 (defvar notmuch-hello-url http://notmuchmail.org;
   The `notmuch' web site.)
 
-(defvar notmuch-hello-search-pos nil
-  Position of search widget, if any.
-
-This should only be set by `notmuch-hello-insert-search'.)
-
 (defvar notmuch-hello-custom-section-options
   '((:filter (string :tag Filter for each tag))
 (:filter-count (string :tag Different filter to generate message counts))
@@ -209,11 +204,8 @@ function produces a section simply by adding content to 
the current
 buffer.  A section should not end with an empty line, because a
 newline will be inserted after each section by `notmuch-hello'.
 
-Each function should take no arguments.  If the produced section
-includes `notmuch-hello-target' (i.e. cursor should be positioned
-inside this section), the function should return this element's
-position.
-Otherwise, it should return nil.
+Each function should take no arguments. The return value is
+ignored.
 
 For convenience an element can also be a list of the form (FUNC ARG1
 ARG2 .. ARGN) in which case FUNC will be applied to the rest of the
@@ -240,15 +232,6 @@ supported for \Customized queries section\ items.
notmuch-hello-query-section
(function :tag Custom section
 
-(defvar notmuch-hello-target nil
-  Button text at position of point before rebuilding the notmuch-buffer.
-
-This variable contains the text of the button, if any, the
-point was positioned at before the notmuch-hello buffer was
-rebuilt. This should never actually be global and is defined as a
-defvar only for documentation purposes and to avoid a compiler
-warning about it occurring as a free variable.)
-
 (defvar notmuch-hello-hidden-sections nil
   List of sections titles whose contents are hidden)
 
@@ -449,8 +432,6 @@ Such a list can be computed with 
`notmuch-hello-query-counts'.
 (msg-count (third elem)))
(widget-insert (format %8s 
   (notmuch-hello-nice-number msg-count)))
-   (if (string= name notmuch-hello-target)
-   (setq found-target-pos (point-marker)))
(widget-create 'push-button
   :notify #'notmuch-hello-widget-search
   :notmuch-search-terms query
@@ -589,7 +570,6 @@ Complete list of currently available key bindings:
 (defun notmuch-hello-insert-search ()
   Insert a search widget.
   (widget-insert Search: )
-  (setq notmuch-hello-search-pos (point-marker))
   (widget-create 'editable-field
 ;; Leave some space at the start and end of the
 ;; search boxes.
@@ -763,13 +743,7 @@ following:
   (set-buffer *notmuch-hello*)
 (switch-to-buffer *notmuch-hello*))
 
-  (let ((notmuch-hello-target (if (widget-at)
- (widget-value (widget-at))
-   (condition-case nil
-   (progn
- (widget-forward 1)
- (widget-value (widget-at)))
- (error nil
+  (let ((final-target-pos (point))
(inhibit-read-only t))
 
 ;; Delete all editable widget fields.  Editable widget fields are
@@ -788,30 +762,20 @@ following:
   (mapc 'delete-overlay 

[RFC PATCH 3/4] 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).
---
 emacs/notmuch-hello.el |   23 +++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 0596bbe..13da146 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)
 
@@ -729,6 +730,28 @@ following:
 (let ((fill-column (- (window-width) notmuch-hello-indent)))
   (center-region start (point)
 
+(defvar notmuch-hello-refresh-count 0
+  Number of messages in the database 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.
+  (unless no-display
+(let* ((new-count
+   (string-to-number (car (process-lines notmuch-command count
+  (diff-count (- new-count notmuch-hello-refresh-count)))
+  (if (= notmuch-hello-refresh-count 0)
+ (message You have %s messages.
+  (notmuch-hello-nice-number new-count))
+   (if (not (= diff-count 0))
+   (if (= diff-count 0)
+   (message You have %s more messages since last refresh.
+(notmuch-hello-nice-number diff-count))
+ (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.1

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[RFC PATCH 4/4] emacs: add notmuch hello refresh hook to place point in a widget

[Jani Nikula]

Add support for putting point to a widget after refresh through a
hook. This approximates the old behaviour.
---
 emacs/notmuch-hello.el |8 +++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 13da146..07e64d4 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -148,7 +148,8 @@ 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)
+  :options '(notmuch-hello-refresh-status-message
+notmuch-hello-refresh-point-to-widget)
   :group 'notmuch-hello
   :group 'notmuch-hooks)
 
@@ -752,6 +753,11 @@ Used internally by 
`notmuch-hello-refresh-status-message'.)
   (notmuch-hello-nice-number (- diff-count))
   (setq notmuch-hello-refresh-count new-count
 
+(defun notmuch-hello-refresh-point-to-widget (no-display)
+  Hook to place point to widget after notmuch-hello refresh.
+  (widget-backward 1)
+  (widget-forward 1))
+
 ;;;###autoload
 (defun notmuch-hello (optional no-display)
   Run notmuch and display saved searches, known tags, etc.
-- 
1.7.1

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 1/7] Split notmuch_database_close into two functions

[Mark Walters]

On Mon, 16 Apr 2012, Justus Winter 4win...@informatik.uni-hamburg.de wrote:
 Quoting Mark Walters (2012-03-31 19:17:15)
 Secondly, I think the patch series could be made clearer and easier to
 review. If you do it in three steps
 
 1) change of notmuch_database_close to notmuch_database_destroy (just
the function name change)
 2) split the new notmuch_database_destroy into two as in the current
first patch
 3) Make any changes (if there are any) of notmuch_database_destroy to
notmuch_database_close.
 
 The advantage is that the first change is easy to test (essentially does
 it build) and then changes from notmuch_database_destroy to
 notmuch_database_close in step 3 are explicit rather than the current
 situation where we need to grep the code to see if some instances of
 notmuch_database_close were not changed to notmuch_database_destroy.

 I don't buy it. The patch series first touches the library and
 documentation and the lib compiles fine. The next patch updates the
 cli tools, all of them compile fine afterwards.

 Every patch addresses the issue component wise, this seems rather
 natural for me.

I will try an explain my concern better. I assume that the patch
actually introduces a functional change : that is something somewhere in
the code calls the new notmuch_database_close instead of
notmuch_database_destroy [1]. In your current patch series someone
reading the patches alone can't see the functional change: it comes from
the occurrences of notmuch_database_close that you *don't* change to
notmuch_database_destroy.

Indeed, if the only change is to allow out-of-tree code access to the
new notmuch_database_close function then doing the patch series as

rename notmuch_database_close to notmuch_database_destroy

Then split notmuch_database_destroy make it clearer. (And if the code
compiles after step 1 then I know *all* occurrences of
notmuch_database_close have been changed to notmuch_database_destroy).


Best wishes

Mark

[1] Apart, of course, from notmuch_database_destroy.

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC] Split notmuch_database_close into two functions

[Mark Walters]

On Thu, 12 Apr 2012, Austin Clements amdra...@mit.edu wrote:
 Quoth Justus Winter on Apr 12 at 11:05 am:
 Quoting Austin Clements (2012-04-01 05:23:23)
 Quoth Justus Winter on Mar 21 at  1:55 am:
  I propose to split the function notmuch_database_close into
  notmuch_database_close and notmuch_database_destroy so that long
  running processes like alot can close the database while still using
  data obtained from queries to that database.
 
 Is this actually safe?  My understanding of Xapian::Database::close is
 that, once you've closed the database, basically anything can throw a
 Xapian exception.  A lot of data is retrieved lazily, both by notmuch
 and by Xapian, so simply having, say, a notmuch_message_t object isn't
 enough to guarantee that you'll be able to get data out of it after
 closing the database.  Hence, I don't see how this interface could be
 used correctly.
 
 I do not know how, but both alot and afew (and occasionally the
 notmuch binary) are somehow safely using this interface on my box for
 the last three weeks.

 I see.  TL;DR: This isn't safe, but that's okay if we document it.

 The bug report [0] you pointed to was quite informative.  At its core,
 this is really a memory management issue.  To sum up for the record
 (and to check my own thinking): It sounds like alot is careful not to
 use any notmuch objects after closing the database.  The problem is
 that, currently, closing the database also talloc_free's it, which
 recursively free's everything derived from it.  Python later GCs the
 wrapper objects, which *also* try to free their underlying objects,
 resulting in a double free.

 Before the change to expose notmuch_database_close, the Python
 bindings would only talloc_free from destructors.  Furthermore, they
 prevented the library from recursively freeing things at other times
 by internally maintaining a reverse reference for every library talloc
 reference (e.g., message is a sub-allocation of query, so the bindings
 keep a reference from each message to its query to ensure the query
 doesn't get freed).  The ability to explicitly talloc_free the
 database subverts this mechanism.


 So, I've come around to thinking that splitting notmuch_database_close
 and _destroy is okay.  It certainly parallels the rest of the API
 better.  However, notmuch_database_close needs a big warning similar
 to Xapian::Database::close's warning that retrieving information from
 objects derived from this database may not work after calling close.
 notmuch_database_close is really a specialty interface, and about the
 only thing you can guarantee after closing the database is that you
 can destroy other objects.  This is also going to require a SONAME
 major version bump, as mentioned by others.  Which, to be fair, would
 be a good opportunity to fix some other issues, too, like how
 notmuch_database_open can't return errors and how
 notmuch_database_get_directory is broken on read-only databases.  The
 actual bump should be done at release time, but maybe we should drop a
 note somewhere (NEWS?) so we don't forget.

Can I just check that there is no way to reopen the Xapian database
readonly? (I may be using the wrong term: I mean is there a way of
switching an open read-write database to read-only without losing the
attached structures/messages/threads etc) If I understand it this would
be sufficient as it would free the lock, but could be more generally
useful for long lived notmuch processes.

Best wishes

Mark
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Dmitry Kurochkin]

Can you please give some explanaition why is this needed?  Would this
change break custom hooks that do not expect any arguments?

Regards,
  Dmitry

Jani Nikula j...@nikula.org writes:

 ---
  emacs/notmuch-hello.el |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

 diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
 index 9cd907a..0596bbe 100644
 --- a/emacs/notmuch-hello.el
 +++ b/emacs/notmuch-hello.el
 @@ -776,7 +776,7 @@ following:
  (widget-setup)
  
  (goto-char final-target-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.1

 ___
 notmuch mailing list
 notmuch@notmuchmail.org
 http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC PATCH 3/4] emacs: add notmuch hello refresh hook to display message count change

[Dmitry Kurochkin]

Jani Nikula j...@nikula.org writes:

 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).

I like this idea.  But IMO we should avoid another call to notmuch
count.  Notmuch-hello buffer already displays the message count on the
first line.  I would propose to implement this functionality not as a
hook but as part of the section which outputs the first line.  We can
add an option to disable it if you prefer but I do not think it is
needed.  This is less flexible than a hook, but IMO it is not a big
issue.

Regards,
  Dmitry

 ---
  emacs/notmuch-hello.el |   23 +++
  1 files changed, 23 insertions(+), 0 deletions(-)

 diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
 index 0596bbe..13da146 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)
  
 @@ -729,6 +730,28 @@ following:
  (let ((fill-column (- (window-width) notmuch-hello-indent)))
(center-region start (point)
  
 +(defvar notmuch-hello-refresh-count 0
 +  Number of messages in the database 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.
 +  (unless no-display
 +(let* ((new-count
 + (string-to-number (car (process-lines notmuch-command count
 +(diff-count (- new-count notmuch-hello-refresh-count)))
 +  (if (= notmuch-hello-refresh-count 0)
 +   (message You have %s messages.
 +(notmuch-hello-nice-number new-count))
 + (if (not (= diff-count 0))
 + (if (= diff-count 0)
 + (message You have %s more messages since last refresh.
 +  (notmuch-hello-nice-number diff-count))
 +   (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.1

 ___
 notmuch mailing list
 notmuch@notmuchmail.org
 http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC PATCH 4/4] emacs: add notmuch hello refresh hook to place point in a widget

[Dmitry Kurochkin]

Jani Nikula j...@nikula.org writes:

 Add support for putting point to a widget after refresh through a
 hook. This approximates the old behaviour.

I may be wrong, but this looks to me like a hack that cannot work well.
See my first reply in the thread for ideas on how to better implement
this functionality.

Regards,
  Dmitry

 ---
  emacs/notmuch-hello.el |8 +++-
  1 files changed, 7 insertions(+), 1 deletions(-)

 diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
 index 13da146..07e64d4 100644
 --- a/emacs/notmuch-hello.el
 +++ b/emacs/notmuch-hello.el
 @@ -148,7 +148,8 @@ 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)
 +  :options '(notmuch-hello-refresh-status-message
 +  notmuch-hello-refresh-point-to-widget)
:group 'notmuch-hello
:group 'notmuch-hooks)
  
 @@ -752,6 +753,11 @@ Used internally by 
 `notmuch-hello-refresh-status-message'.)
  (notmuch-hello-nice-number (- diff-count))
(setq notmuch-hello-refresh-count new-count
  
 +(defun notmuch-hello-refresh-point-to-widget (no-display)
 +  Hook to place point to widget after notmuch-hello refresh.
 +  (widget-backward 1)
 +  (widget-forward 1))
 +
  ;;;###autoload
  (defun notmuch-hello (optional no-display)
Run notmuch and display saved searches, known tags, etc.
 -- 
 1.7.1

 ___
 notmuch mailing list
 notmuch@notmuchmail.org
 http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Jani Nikula]

On Tue, 17 Apr 2012 13:06:13 +0400, Dmitry Kurochkin 
dmitry.kuroch...@gmail.com wrote:
 Can you please give some explanaition why is this needed?  Would this
 change break custom hooks that do not expect any arguments?

For patch 3/4. We don't want to display a message if someone calls
(notmuch-hello-update t) from some script, and notmuch-hello buffer
isn't even visible.

Yes, it would break custom hooks. And a bunch of tests. But I think it
would be useful for custom hooks too, for the same reason as above.

Jani.



 
 Regards,
   Dmitry
 
 Jani Nikula j...@nikula.org writes:
 
  ---
   emacs/notmuch-hello.el |2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)
 
  diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
  index 9cd907a..0596bbe 100644
  --- a/emacs/notmuch-hello.el
  +++ b/emacs/notmuch-hello.el
  @@ -776,7 +776,7 @@ following:
   (widget-setup)
   
   (goto-char final-target-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.1
 
  ___
  notmuch mailing list
  notmuch@notmuchmail.org
  http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Dmitry Kurochkin]

Jani Nikula j...@nikula.org writes:

 On Tue, 17 Apr 2012 13:06:13 +0400, Dmitry Kurochkin 
 dmitry.kuroch...@gmail.com wrote:
 Can you please give some explanaition why is this needed?  Would this
 change break custom hooks that do not expect any arguments?

 For patch 3/4. We don't want to display a message if someone calls
 (notmuch-hello-update t) from some script, and notmuch-hello buffer
 isn't even visible.

 Yes, it would break custom hooks. And a bunch of tests. But I think it
 would be useful for custom hooks too, for the same reason as above.


Makes sense.  I think it should be mentioned in the commit message.
Especially the fact that it breaks existing hooks.  We should mention it
in the NEWS later as well.

Regards,
  Dmitry

 Jani.



 
 Regards,
   Dmitry
 
 Jani Nikula j...@nikula.org writes:
 
  ---
   emacs/notmuch-hello.el |2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)
 
  diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
  index 9cd907a..0596bbe 100644
  --- a/emacs/notmuch-hello.el
  +++ b/emacs/notmuch-hello.el
  @@ -776,7 +776,7 @@ following:
   (widget-setup)
   
   (goto-char final-target-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.1
 
  ___
  notmuch mailing list
  notmuch@notmuchmail.org
  http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC PATCH 3/4] emacs: add notmuch hello refresh hook to display message count change

[Jani Nikula]

On Tue, 17 Apr 2012 13:13:15 +0400, Dmitry Kurochkin 
dmitry.kuroch...@gmail.com wrote:
 Jani Nikula j...@nikula.org writes:
 
  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).
 
 I like this idea.  But IMO we should avoid another call to notmuch
 count.  Notmuch-hello buffer already displays the message count on the
 first line.  I would propose to implement this functionality not as a
 hook but as part of the section which outputs the first line.  We can
 add an option to disable it if you prefer but I do not think it is
 needed.  This is less flexible than a hook, but IMO it is not a big
 issue.

And what if someone has disabled the header section but would want the
message? Also, you'd have to pass no-display there too. IMHO one call to
notmuch count is not a big issue, especially for an optional feature.
And having it as a hook very nicely isolates the feature from everything
else.

Jani.


 
 Regards,
   Dmitry
 
  ---
   emacs/notmuch-hello.el |   23 +++
   1 files changed, 23 insertions(+), 0 deletions(-)
 
  diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
  index 0596bbe..13da146 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)
   
  @@ -729,6 +730,28 @@ following:
   (let ((fill-column (- (window-width) notmuch-hello-indent)))
 (center-region start (point)
   
  +(defvar notmuch-hello-refresh-count 0
  +  Number of messages in the database 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.
  +  (unless no-display
  +(let* ((new-count
  +   (string-to-number (car (process-lines notmuch-command count
  +  (diff-count (- new-count notmuch-hello-refresh-count)))
  +  (if (= notmuch-hello-refresh-count 0)
  + (message You have %s messages.
  +  (notmuch-hello-nice-number new-count))
  +   (if (not (= diff-count 0))
  +   (if (= diff-count 0)
  +   (message You have %s more messages since last refresh.
  +(notmuch-hello-nice-number diff-count))
  + (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.1
 
  ___
  notmuch mailing list
  notmuch@notmuchmail.org
  http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC PATCH 4/4] emacs: add notmuch hello refresh hook to place point in a widget

[Jani Nikula]

On Tue, 17 Apr 2012 13:16:10 +0400, Dmitry Kurochkin 
dmitry.kuroch...@gmail.com wrote:
 Jani Nikula j...@nikula.org writes:
 
  Add support for putting point to a widget after refresh through a
  hook. This approximates the old behaviour.
 
 I may be wrong, but this looks to me like a hack that cannot work well.
 See my first reply in the thread for ideas on how to better implement
 this functionality.

This isn't very much unlike how the current code finds a widget before
refreshing. The difference is that this is based on a saved and restored
point, which indeed does have it's inaccuracies.

Jani.

 
 Regards,
   Dmitry
 
  ---
   emacs/notmuch-hello.el |8 +++-
   1 files changed, 7 insertions(+), 1 deletions(-)
 
  diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
  index 13da146..07e64d4 100644
  --- a/emacs/notmuch-hello.el
  +++ b/emacs/notmuch-hello.el
  @@ -148,7 +148,8 @@ 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)
  +  :options '(notmuch-hello-refresh-status-message
  +notmuch-hello-refresh-point-to-widget)
 :group 'notmuch-hello
 :group 'notmuch-hooks)
   
  @@ -752,6 +753,11 @@ Used internally by 
  `notmuch-hello-refresh-status-message'.)
 (notmuch-hello-nice-number (- diff-count))
 (setq notmuch-hello-refresh-count new-count
   
  +(defun notmuch-hello-refresh-point-to-widget (no-display)
  +  Hook to place point to widget after notmuch-hello refresh.
  +  (widget-backward 1)
  +  (widget-forward 1))
  +
   ;;;###autoload
   (defun notmuch-hello (optional no-display)
 Run notmuch and display saved searches, known tags, etc.
  -- 
  1.7.1
 
  ___
  notmuch mailing list
  notmuch@notmuchmail.org
  http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC PATCH 3/4] emacs: add notmuch hello refresh hook to display message count change

[Dmitry Kurochkin]

Jani Nikula j...@nikula.org writes:

 On Tue, 17 Apr 2012 13:13:15 +0400, Dmitry Kurochkin 
 dmitry.kuroch...@gmail.com wrote:
 Jani Nikula j...@nikula.org writes:
 
  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).
 
 I like this idea.  But IMO we should avoid another call to notmuch
 count.  Notmuch-hello buffer already displays the message count on the
 first line.  I would propose to implement this functionality not as a
 hook but as part of the section which outputs the first line.  We can
 add an option to disable it if you prefer but I do not think it is
 needed.  This is less flexible than a hook, but IMO it is not a big
 issue.

 And what if someone has disabled the header section but would want the
 message?

Well, that is what I meant by less flexible than a hook.

 Also, you'd have to pass no-display there too.

Sure, we can pass it to sections.

 IMHO one call to
 notmuch count is not a big issue, especially for an optional feature.
 And having it as a hook very nicely isolates the feature from everything
 else.


I think this feature should be enabled by default.

I guess you are right that it is not a big issue.  I still think we
would be better without it (and we still can isolate the feature), but I
would not object to having the extra call.

Regards,
  Dmitry

 Jani.


 
 Regards,
   Dmitry
 
  ---
   emacs/notmuch-hello.el |   23 +++
   1 files changed, 23 insertions(+), 0 deletions(-)
 
  diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
  index 0596bbe..13da146 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)
   
  @@ -729,6 +730,28 @@ following:
   (let ((fill-column (- (window-width) notmuch-hello-indent)))
 (center-region start (point)
   
  +(defvar notmuch-hello-refresh-count 0
  +  Number of messages in the database 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.
  +  (unless no-display
  +(let* ((new-count
  +  (string-to-number (car (process-lines notmuch-command count
  + (diff-count (- new-count notmuch-hello-refresh-count)))
  +  (if (= notmuch-hello-refresh-count 0)
  +(message You have %s messages.
  + (notmuch-hello-nice-number new-count))
  +  (if (not (= diff-count 0))
  +  (if (= diff-count 0)
  +  (message You have %s more messages since last refresh.
  +   (notmuch-hello-nice-number diff-count))
  +(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.1
 
  ___
  notmuch mailing list
  notmuch@notmuchmail.org
  http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello

[Jani Nikula]

On Tue, 17 Apr 2012 13:04:39 +0400, Dmitry Kurochkin 
dmitry.kuroch...@gmail.com wrote:
 Hi Jani.
 
 Jani Nikula j...@nikula.org writes:
 
  notmuch-hello (called also through notmuch-hello-update, bound to '='
  by default) tries to find the widget under or following point before
  refresh, and put the point back to the widget afterwards. The code has
  gotten a bit complicated, and has at least the following issues:
 
  1) All the individual section functions have to include code to
 support point placement. If there is no such support, point is
 dropped to the search box. Only saved searches and all tags
 sections support point placement.
 
  2) Point placement is based on widget-value. If there are two widgets
 with the same widget-value (for example a saved search with the
 same name as a tag) the point is moved to the earlier one.
 
  3) When first entering notmuch-hello notmuch-hello-target is nil, and
 point is dropped to the search box.
 
  This patch simplifies the code by removing all point placement based
  on widgets. Point is simply saved before refresh, and put back to
  where it was. Sometimes, but not very often, this would have the
  appearance of moving the point relative to the nearest widgets. IMHO
  this is a minor problem compared to the issues listed above.
 
  A downside is that there's no visual cue (point movement) to indicate
  that refresh has finished. Then again, neither was there before, if
  point was at the beginning of a widget.
 
 Thanks for looking into this.  This is an annoying issue indeed.  And I
 was thinking about fixing it myself.
 
 I am not sure I like the approach of moving the cursor to the same
 position.  It is common that buffer content would change significantly
 after a refresh (e.g. after I archived all new mail).  That would mean
 the cursor would just randomly jump somewhere.  IMO we should allow
 smart cursor positioning which means that logic should go to individual
 sections.
  I would propose the following plan:
 
 1. Remove special case for search box.  No section should be special.
Moreover it is possible to remove it (I did it) and in that case the
cursor would be left at the end of the buffer.  By default, the
cursor should be moved to the beginning of the buffer.
 
 2. Replace current cursor positioning logic with section specific code.
I.e. `notmuch-hello' would not do any cursor positioning (except for
item 1) but queries and tags section would save required state when a
button is clicked and the same section would use this state to
restore cursor position on refresh.  What state should be saved would
depend on the section but we should at least save the section
name/ID.  If during refresh no section set the cursor position, then
the cursor is moved to the beginning of the buffer.
 
 3. Provide a custom variable to set the default section to move the
cursor to.  I.e. set the section name/ID part of the state from item
2.  Again, details on what the default position inside the section is
would depend on the section.  For search box, it would be the input
field.  For queries/tags it would be the first tag.
 
 Item 1 is pretty simple.  The rest may be more tricky.  What do you
 think?

My approach was this: keep it extremely simple, and catch the low
hanging fruit. It places the point where I want, say, 90% of the
time. And when it's wrong, it's not far off. In contrast, the current
implementation places the cursor exactly where I do *not* want it about
half the time.

What you describe sounds smart, but complicated. Maybe it just sounds
complicated from my limited lisp skills perspective. Personally I don't
think sections should have to implement their own logic for point
placement. And as a fallback, I prefer keeping the point where it is
instead of moving it to the beginning of buffer.

I don't oppose to your plan, but I don't think I'm up to implementing it
either. I just cooked up something together to fix the #1 annoyance I've
lately had with notmuch, and Tomi persuaded me to share the patches as
RFC. I still think it's pretty good, considering the simplicity of it
all, but certainly not perfect.


BR,
Jani.


 
 Regards,
   Dmitry
 
  ---
   emacs/notmuch-hello.el |   70 
  +++
   1 files changed, 17 insertions(+), 53 deletions(-)
 
  diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
  index 71d37b8..9cd907a 100644
  --- a/emacs/notmuch-hello.el
  +++ b/emacs/notmuch-hello.el
  @@ -154,11 +154,6 @@ International Bureau of Weights and Measures.
   (defvar notmuch-hello-url http://notmuchmail.org;
 The `notmuch' web site.)
   
  -(defvar notmuch-hello-search-pos nil
  -  Position of search widget, if any.
  -
  -This should only be set by `notmuch-hello-insert-search'.)
  -
   (defvar notmuch-hello-custom-section-options
 '((:filter (string :tag Filter for each tag))
   (:filter-count (string 

Re: [RFC PATCH 1/4] emacs: simplify point placement in notmuch-hello

[Tomi Ollila]

On Tue, Apr 17 2012, Jani Nikula j...@nikula.org wrote:

 On Tue, 17 Apr 2012 13:04:39 +0400, Dmitry Kurochkin 
 dmitry.kuroch...@gmail.com wrote:
 Hi Jani.
 
 Jani Nikula j...@nikula.org writes:

[ stuff deleted ]

 I don't oppose to your plan, but I don't think I'm up to implementing it
 either. I just cooked up something together to fix the #1 annoyance I've
 lately had with notmuch, and Tomi persuaded me to share the patches as
 RFC. I still think it's pretty good, considering the simplicity of it
 all, but certainly not perfect.

Thank you. Last night I spent something like 30 min in bed thinking of
this issue; Now I have more ideas to think of. I'll provide my feedback
in another RFC patch based on all of these ideas (I think this is most
appreciated by Jani as it reduces his workload :D)


 BR,
 Jani.

 
 Regards,
   Dmitry
 

Tomi
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 2/3] test: emacs: toggle eliding of non-matching messages in `notmuch-show'

[Mark Walters]

On Sun, 19 Feb 2012, Pieter Praet pie...@praet.org wrote:
 See commits 44a544ed, 866ce8b1, 668b66ec.
 ---
  test/emacs |   38 ++
  .../notmuch-show-elide-non-matching-messages-off   |   79 
 
  .../notmuch-show-elide-non-matching-messages-on|   75 +++
  3 files changed, 192 insertions(+), 0 deletions(-)
  create mode 100644 
 test/emacs.expected-output/notmuch-show-elide-non-matching-messages-off
  create mode 100644 
 test/emacs.expected-output/notmuch-show-elide-non-matching-messages-on

This patch looks good to me and with other possible ways of implementing
elide [1] seems well worth having.

It needs [1/3] which I think is ok but I have never used the crypto
stuff so wouldn't count this as a review of that patch.

Patch [3/3] also looks fine.

Best wishes

Mark

[1] id:1334077496-9172-1-git-send-email-markwalters1...@gmail.com

 diff --git a/test/emacs b/test/emacs
 index b207d20..320057a 100755
 --- a/test/emacs
 +++ b/test/emacs
 @@ -553,5 +553,43 @@ test_emacs '(let ((notmuch-crypto-process-mime nil))
   (test-visible-output))'
  test_expect_equal_file OUTPUT 
 $EXPECTED/notmuch-show-process-crypto-mime-parts-on
  
 +test_begin_subtest notmuch-show: don't elide non-matching messages
 +test_emacs '(let ((notmuch-show-only-matching-messages nil))
 + (notmuch-search from:l...@seas.harvard.edu and subject:\Maildir 
 storage\)
 + (notmuch-test-wait)
 + (notmuch-search-show-thread)
 + (notmuch-test-wait)
 + (test-visible-output))'
 +test_expect_equal_file OUTPUT 
 $EXPECTED/notmuch-show-elide-non-matching-messages-off
 +
 +test_begin_subtest notmuch-show: elide non-matching messages
 +test_emacs '(let ((notmuch-show-only-matching-messages t))
 + (notmuch-search from:l...@seas.harvard.edu and subject:\Maildir 
 storage\)
 + (notmuch-test-wait)
 + (notmuch-search-show-thread)
 + (notmuch-test-wait)
 + (test-visible-output))'
 +test_expect_equal_file OUTPUT 
 $EXPECTED/notmuch-show-elide-non-matching-messages-on
 +
 +test_begin_subtest notmuch-show: elide non-matching messages (w/ 
 notmuch-show-toggle-elide-non-matching)
 +test_emacs '(let ((notmuch-show-only-matching-messages nil))
 + (notmuch-search from:l...@seas.harvard.edu and subject:\Maildir 
 storage\)
 + (notmuch-test-wait)
 + (notmuch-search-show-thread)
 + (notmuch-test-wait)
 + (notmuch-show-toggle-elide-non-matching)
 + (test-visible-output))'
 +test_expect_equal_file OUTPUT 
 $EXPECTED/notmuch-show-elide-non-matching-messages-on
 +
 +test_begin_subtest notmuch-show: elide non-matching messages (w/ prefix arg 
 to notmuch-show)
 +test_emacs '(let ((notmuch-show-only-matching-messages nil))
 + (notmuch-search from:l...@seas.harvard.edu and subject:\Maildir 
 storage\)
 + (notmuch-test-wait)
 + (let ((current-prefix-arg t))
 +   (notmuch-search-show-thread))
 + (notmuch-test-wait)
 + (test-visible-output))'
 +test_expect_equal_file OUTPUT 
 $EXPECTED/notmuch-show-elide-non-matching-messages-on
 +
  
  test_done
 diff --git 
 a/test/emacs.expected-output/notmuch-show-elide-non-matching-messages-off 
 b/test/emacs.expected-output/notmuch-show-elide-non-matching-messages-off
 new file mode 100644
 index 000..b31fe62
 --- /dev/null
 +++ b/test/emacs.expected-output/notmuch-show-elide-non-matching-messages-off
 @@ -0,0 +1,79 @@
 +Lars Kellogg-Stedman l...@seas.harvard.edu (2009-11-17) (inbox signed)
 +Subject: [notmuch] Working with Maildir storage?
 +To: notmuch@notmuchmail.org
 +Date: Tue, 17 Nov 2009 14:00:54 -0500
 +
 +[ multipart/mixed ]
 +[ multipart/signed ]
 +[ text/plain ]
 +I saw the LWN article and decided to take a look at notmuch.  I'm
 +currently using mutt and mairix to index and read a collection of
 +Maildir mail folders (around 40,000 messages total).
 +
 +notmuch indexed the messages without complaint, but my attempt at
 +searching bombed out. Running, for example:
 +
 +  notmuch search storage
 +
 +Resulted in 4604 lines of errors along the lines of:
 +
 +  Error opening
 +  
 /home/lars/Mail/read-messages.2008/cur/1246413773.24928_27334.hostname,U=3026:2,S:
 +  Too many open files
 +
 +I'm curious if this is expected behavior (i.e., notmuch does not work
 +with Maildir) or if something else is going on.
 +
 +Cheers,
 +
 +[ 4-line signature. Click/Enter to show. ]
 +[ application/pgp-signature ]
 +[ text/plain ]
 +[ 4-line signature. Click/Enter to show. ]
 + Mikhail Gusarov dotted...@dottedmag.net (2009-11-17) (inbox signed unread)
 +  Lars Kellogg-Stedman l...@seas.harvard.edu (2009-11-17) (inbox signed)
 +  Subject: Re: [notmuch] Working with Maildir storage?
 +  To: Mikhail Gusarov dotted...@dottedmag.net
 +  Cc: notmuch@notmuchmail.org
 +  Date: Tue, 17 Nov 2009 15:33:01 -0500
 +
 +  [ multipart/mixed ]
 +  [ multipart/signed ]
 +  [ text/plain ]
 +   See the patch just posted here.
 +
 +  Is the list archived anywhere?  The obvious archives
 + 

Re: [PATCH] contrib/nmbug: use resolve merge strategy

[Tomi Ollila]

On Sat, Mar 31 2012, da...@tethera.net wrote:

 From: David Bremner brem...@debian.org

 The recursive merge strategy does rename detection, which yields false
 positives (and hence spurious merge conflicts) when merging trees of
 empty files.

I attempted to search more info about 'recursive' vs 'resolve' merge
but without much luck. Nothing better comes out than MERGE STRATEGIES
section in git-merge namual page.

IMHO text favoring resolve: It tries to carefully detect criss-cross
merge ambiguities and is considered generally safe and fast.

 text unfavoring recursive (in this particular case): Additionally
this can detect and handle merges involving renames.

so LGTM.

Tomi

 ---
  An unresolved issue (ho ho) is the fact that failed merge operations
  are still not detected. This needs more thought, but I thought this 
  patch might save people some pain in the meantime. It isn't very heavily 
  tested, though.

  contrib/nmbug |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

 diff --git a/contrib/nmbug b/contrib/nmbug
 index bb0739f..0ed3c29 100755
 --- a/contrib/nmbug
 +++ b/contrib/nmbug
 @@ -302,7 +302,7 @@ sub do_merge {
  
git ( { GIT_WORK_TREE = $tempwork }, 'checkout', '-f', 'HEAD');
  
 -  git ( { GIT_WORK_TREE = $tempwork }, 'merge', 'FETCH_HEAD');
 +  git ( { GIT_WORK_TREE = $tempwork }, 'merge', '-s', 'resolve', 
 'FETCH_HEAD');
  
do_checkout ();
  }
 -- 
 1.7.9.1

 ___
 notmuch mailing list
 notmuch@notmuchmail.org
 http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch