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

[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 (car all))
       (mapc 'delete-overlay (cdr all)))
 
-    (let (final-target-pos)
-      (mapc
-       (lambda (section)
-	 (let ((point-before (point))
-	       (result (if (functionp section)
-			   (funcall section)
-			 (apply (car section) (cdr section)))))
-	   (if (and (not final-target-pos) (integer-or-marker-p result))
-	       (setq final-target-pos result))
-	   ;; don't insert a newline when the previous section didn't show
-	   ;; anything.
-	   (unless (eq (point) point-before)
-	     (widget-insert "\n"))))
-       notmuch-hello-sections)
-      (widget-setup)
-
-      (when final-target-pos
-	(goto-char final-target-pos)
-	(unless (widget-at)
-	  (widget-forward 1)))
-
-      (unless (widget-at)
-	(when notmuch-hello-search-pos
-	  (goto-char notmuch-hello-search-pos)))))
+    (mapc
+     (lambda (section)
+       (let ((point-before (point)))
+	 (if (functionp section)
+	     (funcall section)
+	   (apply (car section) (cdr section)))
+	 ;; don't insert a newline when the previous section didn't
+	 ;; show anything.
+	 (unless (eq (point) point-before)
+	   (widget-insert "\n"))))
+     notmuch-hello-sections)
+    (widget-setup)
+
+    (goto-char final-target-pos))
   (run-hooks 'notmuch-hello-refresh-hook)
   (setq notmuch-hello-first-run nil))
 
-- 
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 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

[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

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

[Dmitry Kurochkin]

Hi Jani.

Jani Nikula <jani@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?

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 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 (car all))
>        (mapc 'delete-overlay (cdr all)))
>  
> -    (let (final-target-pos)
> -      (mapc
> -       (lambda (section)
> -	 (let ((point-before (point))
> -	       (result (if (functionp section)
> -			   (funcall section)
> -			 (apply (car section) (cdr section)))))
> -	   (if (and (not final-target-pos) (integer-or-marker-p result))
> -	       (setq final-target-pos result))
> -	   ;; don't insert a newline when the previous section didn't show
> -	   ;; anything.
> -	   (unless (eq (point) point-before)
> -	     (widget-insert "\n"))))
> -       notmuch-hello-sections)
> -      (widget-setup)
> -
> -      (when final-target-pos
> -	(goto-char final-target-pos)
> -	(unless (widget-at)
> -	  (widget-forward 1)))
> -
> -      (unless (widget-at)
> -	(when notmuch-hello-search-pos
> -	  (goto-char notmuch-hello-search-pos)))))
> +    (mapc
> +     (lambda (section)
> +       (let ((point-before (point)))
> +	 (if (functionp section)
> +	     (funcall section)
> +	   (apply (car section) (cdr section)))
> +	 ;; don't insert a newline when the previous section didn't
> +	 ;; show anything.
> +	 (unless (eq (point) point-before)
> +	   (widget-insert "\n"))))
> +     notmuch-hello-sections)
> +    (widget-setup)
> +
> +    (goto-char final-target-pos))
>    (run-hooks 'notmuch-hello-refresh-hook)
>    (setq notmuch-hello-first-run nil))
>  
> -- 
> 1.7.1
>
> _______________________________________________
> 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 <jani@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

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

[Dmitry Kurochkin]

Jani Nikula <jani@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

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

[Dmitry Kurochkin]

Jani Nikula <jani@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

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

Jani.



> 
> Regards,
>   Dmitry
> 
> Jani Nikula <jani@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

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

[Dmitry Kurochkin]

Jani Nikula <jani@nikula.org> writes:

> On Tue, 17 Apr 2012 13:06:13 +0400, Dmitry Kurochkin <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 <jani@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

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.kurochkin@gmail.com> wrote:
> Jani Nikula <jani@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

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.kurochkin@gmail.com> wrote:
> Jani Nikula <jani@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

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

[Dmitry Kurochkin]

Jani Nikula <jani@nikula.org> writes:

> On Tue, 17 Apr 2012 13:13:15 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
>> Jani Nikula <jani@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

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.kurochkin@gmail.com> wrote:
> Hi Jani.
> 
> Jani Nikula <jani@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 :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 (car all))
> >        (mapc 'delete-overlay (cdr all)))
> >  
> > -    (let (final-target-pos)
> > -      (mapc
> > -       (lambda (section)
> > -	 (let ((point-before (point))
> > -	       (result (if (functionp section)
> > -			   (funcall section)
> > -			 (apply (car section) (cdr section)))))
> > -	   (if (and (not final-target-pos) (integer-or-marker-p result))
> > -	       (setq final-target-pos result))
> > -	   ;; don't insert a newline when the previous section didn't show
> > -	   ;; anything.
> > -	   (unless (eq (point) point-before)
> > -	     (widget-insert "\n"))))
> > -       notmuch-hello-sections)
> > -      (widget-setup)
> > -
> > -      (when final-target-pos
> > -	(goto-char final-target-pos)
> > -	(unless (widget-at)
> > -	  (widget-forward 1)))
> > -
> > -      (unless (widget-at)
> > -	(when notmuch-hello-search-pos
> > -	  (goto-char notmuch-hello-search-pos)))))
> > +    (mapc
> > +     (lambda (section)
> > +       (let ((point-before (point)))
> > +	 (if (functionp section)
> > +	     (funcall section)
> > +	   (apply (car section) (cdr section)))
> > +	 ;; don't insert a newline when the previous section didn't
> > +	 ;; show anything.
> > +	 (unless (eq (point) point-before)
> > +	   (widget-insert "\n"))))
> > +     notmuch-hello-sections)
> > +    (widget-setup)
> > +
> > +    (goto-char final-target-pos))
> >    (run-hooks 'notmuch-hello-refresh-hook)
> >    (setq notmuch-hello-first-run nil))
> >  
> > -- 
> > 1.7.1
> >
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

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

[Tomi Ollila]

On Tue, Apr 17 2012, Jani Nikula <jani@nikula.org> wrote:

> On Tue, 17 Apr 2012 13:04:39 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
>> Hi Jani.
>> 
>> Jani Nikula <jani@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