[WIP PATCH] emacs: hello: bugfix for saved searches defcustom

[WIP PATCH] emacs: hello: bugfix for saved searches defcustom

[Mark Walters]

---
Hi

David found a bug in the recent notmuch hello saved searches and
reduced to a small test case. The problem occurs if the user sets
notmuch-saved-searches and then loads notmuch. The reason is that the
defcustom calls some utility functions and these need to be defined
(not just declared) when the defcustom is encountered.

This patch moves the code around so that they are defined first. I
have marked the patch WIP as I am not sure what the best reordering
is, and we may want to rename some functions which have changed file
for namespace reasons.

Anyway this fixes the test case. If it fixes the original bug too then
I will provide a better version.

Best wishes 

Mark



 emacs/notmuch-hello.el |  128 ++++++++++++++++++++++++++++++++++--------------
 emacs/notmuch-lib.el   |   52 --------------------
 2 files changed, 90 insertions(+), 90 deletions(-)

diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
index 4900a24..27eac72 100644
--- a/emacs/notmuch-hello.el
+++ b/emacs/notmuch-hello.el
@@ -29,6 +29,96 @@
 (declare-function notmuch-search "notmuch" (&optional query oldest-first target-thread target-line continuation))
 (declare-function notmuch-poll "notmuch" ())
 
+(defun notmuch-saved-search-get (saved-search field)
+  "Get FIELD from SAVED-SEARCH.
+
+If SAVED-SEARCH is a plist, this is just `plist-get', but for
+backwards compatibility, this also deals with the two other
+possible formats for SAVED-SEARCH: cons cells (NAME . QUERY) and
+lists (NAME QUERY COUNT-QUERY)."
+  (cond
+   ((keywordp (car saved-search))
+    (plist-get saved-search field))
+   ;; It is not a plist so it is an old-style entry.
+   ((consp (cdr saved-search)) ;; It is a list (NAME QUERY COUNT-QUERY)
+    (case field
+      (:name (first saved-search))
+      (:query (second saved-search))
+      (:count-query (third saved-search))
+      (t nil)))
+   (t  ;; It is a cons-cell (NAME . QUERY)
+    (case field
+      (:name (car saved-search))
+      (:query (cdr saved-search))
+      (t nil)))))
+
+(defun notmuch-hello-saved-search-to-plist (saved-search)
+  "Return a copy of SAVED-SEARCH in plist form.
+
+If saved search is a plist then just return a copy. In other
+cases, for backwards compatability, convert to plist form and
+return that."
+  (if (keywordp (car saved-search))
+      (copy-seq saved-search)
+    (let ((fields (list :name :query :count-query))
+	  plist-search)
+      (dolist (field fields plist-search)
+	(let ((string (notmuch-saved-search-get saved-search field)))
+	  (when string
+	    (setq plist-search (append plist-search (list field string)))))))))
+
+(defun notmuch--saved-searches-to-plist (symbol)
+  "Extract a saved-search variable into plist form.
+
+The new style saved search is just a plist, but for backwards
+compatatibility we use this function to extract old style saved
+searches so they still work in customize."
+  (let ((saved-searches (default-value symbol)))
+    (mapcar #'notmuch-hello-saved-search-to-plist saved-searches)))
+
+(define-widget 'notmuch-saved-search-plist 'list
+  "A single saved search property list."
+  :tag "Saved Search"
+  :args '((list :inline t
+		:format "%v"
+		(group :format "%v" :inline t (const :format "   Name: " :name) (string :format "%v"))
+		(group :format "%v" :inline t (const :format "  Query: " :query) (string :format "%v")))
+	  (checklist :inline t
+		     :format "%v"
+		     (group :format "%v" :inline t (const :format "Count-Query: " :count-query) (string :format "%v"))
+		     (group :format "%v" :inline t (const :format "" :sort-order)
+			    (choice :tag " Sort Order"
+				    (const :tag "Default" nil)
+				    (const :tag "Oldest-first" oldest-first)
+				    (const :tag "Newest-first" newest-first))))))
+
+(defcustom notmuch-saved-searches '((:name "inbox" :query "tag:inbox")
+				    (:name "unread" :query "tag:unread"))
+  "A list of saved searches to display.
+
+The saved search can be given in 3 forms. The preferred way is as
+a plist. Supported properties are
+
+  :name            Name of the search (required).
+  :query           Search to run (required).
+  :count-query     Optional extra query to generate the count
+                   shown. If not present then the :query property
+                   is used.
+  :sort-order      Specify the sort order to be used for the search.
+                   Possible values are 'oldest-first 'newest-first or
+                   nil. Nil means use the default sort order.
+
+Other accepted forms are a cons cell of the form (NAME . QUERY)
+or a list of the form (NAME QUERY COUNT-QUERY)."
+;; The saved-search format is also used by the all-tags notmuch-hello
+;; section. This section generates its own saved-search list in one of
+;; the latter two forms.
+
+  :get 'notmuch--saved-searches-to-plist
+  :type '(repeat notmuch-saved-search-plist)
+  :tag "List of Saved Searches"
+  :group 'notmuch-hello)
+
 (defcustom notmuch-hello-recent-searches-max 10
   "The number of recent searches to display."
   :type 'integer
@@ -269,44 +359,6 @@ afterwards.")
       (add-to-history 'notmuch-search-history search)))
   (notmuch-search search notmuch-search-oldest-first))
 
-(defun notmuch-saved-search-get (saved-search field)
-  "Get FIELD from SAVED-SEARCH.
-
-If SAVED-SEARCH is a plist, this is just `plist-get', but for
-backwards compatibility, this also deals with the two other
-possible formats for SAVED-SEARCH: cons cells (NAME . QUERY) and
-lists (NAME QUERY COUNT-QUERY)."
-  (cond
-   ((keywordp (car saved-search))
-    (plist-get saved-search field))
-   ;; It is not a plist so it is an old-style entry.
-   ((consp (cdr saved-search)) ;; It is a list (NAME QUERY COUNT-QUERY)
-    (case field
-      (:name (first saved-search))
-      (:query (second saved-search))
-      (:count-query (third saved-search))
-      (t nil)))
-   (t  ;; It is a cons-cell (NAME . QUERY)
-    (case field
-      (:name (car saved-search))
-      (:query (cdr saved-search))
-      (t nil)))))
-
-(defun notmuch-hello-saved-search-to-plist (saved-search)
-  "Return a copy of SAVED-SEARCH in plist form.
-
-If saved search is a plist then just return a copy. In other
-cases, for backwards compatability, convert to plist form and
-return that."
-  (if (keywordp (car saved-search))
-      (copy-seq saved-search)
-    (let ((fields (list :name :query :count-query))
-	  plist-search)
-      (dolist (field fields plist-search)
-	(let ((string (notmuch-saved-search-get saved-search field)))
-	  (when string
-	    (setq plist-search (append plist-search (list field string)))))))))
-
 (defun notmuch-hello-add-saved-search (widget)
   (interactive)
   (let ((search (widget-value
diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 3a3c69d..2941da3 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -107,58 +107,6 @@ Note that the recommended way of achieving the same is using
 (defvar notmuch-search-history nil
   "Variable to store notmuch searches history.")
 
-(defun notmuch--saved-searches-to-plist (symbol)
-  "Extract a saved-search variable into plist form.
-
-The new style saved search is just a plist, but for backwards
-compatatibility we use this function to extract old style saved
-searches so they still work in customize."
-  (let ((saved-searches (default-value symbol)))
-    (mapcar #'notmuch-hello-saved-search-to-plist saved-searches)))
-
-(define-widget 'notmuch-saved-search-plist 'list
-  "A single saved search property list."
-  :tag "Saved Search"
-  :args '((list :inline t
-		:format "%v"
-		(group :format "%v" :inline t (const :format "   Name: " :name) (string :format "%v"))
-		(group :format "%v" :inline t (const :format "  Query: " :query) (string :format "%v")))
-	  (checklist :inline t
-		     :format "%v"
-		     (group :format "%v" :inline t (const :format "Count-Query: " :count-query) (string :format "%v"))
-		     (group :format "%v" :inline t (const :format "" :sort-order)
-			    (choice :tag " Sort Order"
-				    (const :tag "Default" nil)
-				    (const :tag "Oldest-first" oldest-first)
-				    (const :tag "Newest-first" newest-first))))))
-
-(defcustom notmuch-saved-searches '((:name "inbox" :query "tag:inbox")
-				    (:name "unread" :query "tag:unread"))
-  "A list of saved searches to display.
-
-The saved search can be given in 3 forms. The preferred way is as
-a plist. Supported properties are
-
-  :name            Name of the search (required).
-  :query           Search to run (required).
-  :count-query     Optional extra query to generate the count
-                   shown. If not present then the :query property
-                   is used.
-  :sort-order      Specify the sort order to be used for the search.
-                   Possible values are 'oldest-first 'newest-first or
-                   nil. Nil means use the default sort order.
-
-Other accepted forms are a cons cell of the form (NAME . QUERY)
-or a list of the form (NAME QUERY COUNT-QUERY)."
-;; The saved-search format is also used by the all-tags notmuch-hello
-;; section. This section generates its own saved-search list in one of
-;; the latter two forms.
-
-  :get 'notmuch--saved-searches-to-plist
-  :type '(repeat notmuch-saved-search-plist)
-  :tag "List of Saved Searches"
-  :group 'notmuch-hello)
-
 (defcustom notmuch-archive-tags '("-inbox")
   "List of tag changes to apply to a message or a thread when it is archived.
 
-- 
1.7.10.4

Re: [WIP PATCH] emacs: hello: bugfix for saved searches defcustom

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> ---
> Hi
>
> David found a bug in the recent notmuch hello saved searches and
> reduced to a small test case. The problem occurs if the user sets
> notmuch-saved-searches and then loads notmuch. The reason is that the
> defcustom calls some utility functions and these need to be defined
> (not just declared) when the defcustom is encountered.
>
> This patch moves the code around so that they are defined first. I
> have marked the patch WIP as I am not sure what the best reordering
> is, and we may want to rename some functions which have changed file
> for namespace reasons.
>
> Anyway this fixes the test case. If it fixes the original bug too then
> I will provide a better version.

Hi Mark;

This fixes the problem for me.

Thanks for your efforts,

d