[PATCH] emacs: tree/show remove duplicate function

[PATCH] emacs: tree/show remove duplicate function

[Mark Walters]

tree overrides notmuch-show-get-prop so that it can use many of the
utility function directly. Now that tree is in mainline the version
from tree can be moved to show and the original overridden show
version dropped.
---
I should have done this ages ago but forgot about it.

Best wishes

Mark


 emacs/notmuch-show.el |   12 +++++++++++-
 emacs/notmuch-tree.el |   16 ----------------
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 10fc872..b922a38 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -46,6 +46,7 @@
 (declare-function notmuch-save-attachments "notmuch" (mm-handle &optional queryp))
 (declare-function notmuch-tree "notmuch-tree"
 		  (&optional query query-context target buffer-name open-target))
+(declare-function notmuch-tree-get-message-properties "notmuch-tree" nil)
 
 (defcustom notmuch-message-headers '("Subject" "To" "Cc" "Date")
   "Headers that should be shown in a message, in this order.
@@ -1459,8 +1460,17 @@ (defun notmuch-show-set-prop (prop val &optional props)
     (notmuch-show-set-message-properties props)))
 
 (defun notmuch-show-get-prop (prop &optional props)
+  "Get property PROP from current message in show or tree mode.
+
+It gets property PROP from PROPS or, if PROPS is nil, the current
+message in either tree or show. This means that several utility
+functions in notmuch-show can be used directly by notmuch-tree as
+they just need the correct message properties."
   (let ((props (or props
-		   (notmuch-show-get-message-properties))))
+		   (cond ((eq major-mode 'notmuch-show-mode)
+			  (notmuch-show-get-message-properties))
+			 ((eq major-mode 'notmuch-tree-mode)
+			  (notmuch-tree-get-message-properties))))))
     (plist-get props prop)))
 
 (defun notmuch-show-get-message-id (&optional bare)
diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
index 7d5f475..88f92f2 100644
--- a/emacs/notmuch-tree.el
+++ b/emacs/notmuch-tree.el
@@ -290,22 +290,6 @@ (defun notmuch-tree-get-message-properties ()
     (beginning-of-line)
     (get-text-property (point) :notmuch-message-properties)))
 
-;; XXX This should really be a lib function but we are trying to
-;; reduce impact on the code base.
-(defun notmuch-show-get-prop (prop &optional props)
-  "This is a tree view overridden version of notmuch-show-get-prop
-
-It gets property PROP from PROPS or, if PROPS is nil, the current
-message in either tree or show. This means that several functions
-in notmuch-show now work unchanged in tree as they just need the
-correct message properties."
-  (let ((props (or props
-		   (cond ((eq major-mode 'notmuch-show-mode)
-			  (notmuch-show-get-message-properties))
-			 ((eq major-mode 'notmuch-tree-mode)
-			  (notmuch-tree-get-message-properties))))))
-    (plist-get props prop)))
-
 (defun notmuch-tree-set-message-properties (props)
   (save-excursion
     (beginning-of-line)
-- 
1.7.10.4

Re: [PATCH] emacs: tree/show remove duplicate function

[Tomi Ollila]

On Sat, Jun 07 2014, Mark Walters <markwalters1009@gmail.com> wrote:

> tree overrides notmuch-show-get-prop so that it can use many of the
> utility function directly. Now that tree is in mainline the version
> from tree can be moved to show and the original overridden show
> version dropped.
> ---
> I should have done this ages ago but forgot about it.

LGTM, and 

https://github.com/domo141/nottoomuch/blob/master/make-one-notmuch-el.pl

no longer complains about duplicate function :D

>
> Best wishes
>
> Mark

Tomi


>
>
>  emacs/notmuch-show.el |   12 +++++++++++-
>  emacs/notmuch-tree.el |   16 ----------------
>  2 files changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 10fc872..b922a38 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -46,6 +46,7 @@
>  (declare-function notmuch-save-attachments "notmuch" (mm-handle &optional queryp))
>  (declare-function notmuch-tree "notmuch-tree"
>  		  (&optional query query-context target buffer-name open-target))
> +(declare-function notmuch-tree-get-message-properties "notmuch-tree" nil)
>  
>  (defcustom notmuch-message-headers '("Subject" "To" "Cc" "Date")
>    "Headers that should be shown in a message, in this order.
> @@ -1459,8 +1460,17 @@ (defun notmuch-show-set-prop (prop val &optional props)
>      (notmuch-show-set-message-properties props)))
>  
>  (defun notmuch-show-get-prop (prop &optional props)
> +  "Get property PROP from current message in show or tree mode.
> +
> +It gets property PROP from PROPS or, if PROPS is nil, the current
> +message in either tree or show. This means that several utility
> +functions in notmuch-show can be used directly by notmuch-tree as
> +they just need the correct message properties."
>    (let ((props (or props
> -		   (notmuch-show-get-message-properties))))
> +		   (cond ((eq major-mode 'notmuch-show-mode)
> +			  (notmuch-show-get-message-properties))
> +			 ((eq major-mode 'notmuch-tree-mode)
> +			  (notmuch-tree-get-message-properties))))))
>      (plist-get props prop)))
>  
>  (defun notmuch-show-get-message-id (&optional bare)
> diff --git a/emacs/notmuch-tree.el b/emacs/notmuch-tree.el
> index 7d5f475..88f92f2 100644
> --- a/emacs/notmuch-tree.el
> +++ b/emacs/notmuch-tree.el
> @@ -290,22 +290,6 @@ (defun notmuch-tree-get-message-properties ()
>      (beginning-of-line)
>      (get-text-property (point) :notmuch-message-properties)))
>  
> -;; XXX This should really be a lib function but we are trying to
> -;; reduce impact on the code base.
> -(defun notmuch-show-get-prop (prop &optional props)
> -  "This is a tree view overridden version of notmuch-show-get-prop
> -
> -It gets property PROP from PROPS or, if PROPS is nil, the current
> -message in either tree or show. This means that several functions
> -in notmuch-show now work unchanged in tree as they just need the
> -correct message properties."
> -  (let ((props (or props
> -		   (cond ((eq major-mode 'notmuch-show-mode)
> -			  (notmuch-show-get-message-properties))
> -			 ((eq major-mode 'notmuch-tree-mode)
> -			  (notmuch-tree-get-message-properties))))))
> -    (plist-get props prop)))
> -
>  (defun notmuch-tree-set-message-properties (props)
>    (save-excursion
>      (beginning-of-line)
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: tree/show remove duplicate function

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:
> +		   (cond ((eq major-mode 'notmuch-show-mode)
> +			  (notmuch-show-get-message-properties))
> +			 ((eq major-mode 'notmuch-tree-mode)
> +			  (notmuch-tree-get-message-properties))))

I see this already existed, but it looks weird to me to have a two test
cond with no else. Is it intentional to have the code drop through and
do nothing if neither case matches?  It seems like it might be better to
signal an error.

d

Re: [PATCH] emacs: tree/show remove duplicate function

[Mark Walters]

On Tue, 15 Jul 2014, David Bremner <david@tethera.net> wrote:
> Mark Walters <markwalters1009@gmail.com> writes:
>> +		   (cond ((eq major-mode 'notmuch-show-mode)
>> +			  (notmuch-show-get-message-properties))
>> +			 ((eq major-mode 'notmuch-tree-mode)
>> +			  (notmuch-tree-get-message-properties))))
>
> I see this already existed, but it looks weird to me to have a two test
> cond with no else. Is it intentional to have the code drop through and
> do nothing if neither case matches?  It seems like it might be better to
> signal an error.

I can definitely do that. But as a comparison
notmuch-search-get-result and notmuch-search-find-thread-id "work" in
any buffer in the sense of returning nil but not complaining so perhaps
the current version is more consistent. 

Plausibly a comment and an explicit nil case would be clearer for the other modes.

Best wishes

Mark

Re: [PATCH] emacs: tree/show remove duplicate function

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

>
> Plausibly a comment and an explicit nil case would be clearer for the other modes.
>

Your call. 

d

Re: [PATCH] emacs: tree/show remove duplicate function

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> On Tue, 15 Jul 2014, David Bremner <david@tethera.net> wrote:
>> Mark Walters <markwalters1009@gmail.com> writes:
>>> +		   (cond ((eq major-mode 'notmuch-show-mode)
>>> +			  (notmuch-show-get-message-properties))
>>> +			 ((eq major-mode 'notmuch-tree-mode)
>>> +			  (notmuch-tree-get-message-properties))))
>>
>> I see this already existed, but it looks weird to me to have a two test
>> cond with no else. Is it intentional to have the code drop through and
>> do nothing if neither case matches?  It seems like it might be better to
>> signal an error.
>
> I can definitely do that. But as a comparison
> notmuch-search-get-result and notmuch-search-find-thread-id "work" in
> any buffer in the sense of returning nil but not complaining so perhaps
> the current version is more consistent. 
>

It occurs to me that we can fix all 3 places in a followup patch,
so I'll push this for now.

d

Re: [PATCH] emacs: tree/show remove duplicate function

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> tree overrides notmuch-show-get-prop so that it can use many of the
> utility function directly. Now that tree is in mainline the version
> from tree can be moved to show and the original overridden show
> version dropped.
> ---

Pushed. I hope there will be a followup patch making the current
implicit nil return more explicit.

d

[PATCH] emacs: show: make return value of notmuch-show-get-prop explicit

[Mark Walters]

This makes the fact the notmuch-show-get-prop returns nil if the major
mode is neither show not tree explicit.
---
This is a followup to the patch and review comment at 
id:874myhabi0.fsf@maritornes.cs.unb.ca

Best wishes

Mark

 emacs/notmuch-show.el |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 529baa9..7549fbb 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1485,7 +1485,8 @@ they just need the correct message properties."
 		   (cond ((eq major-mode 'notmuch-show-mode)
 			  (notmuch-show-get-message-properties))
 			 ((eq major-mode 'notmuch-tree-mode)
-			  (notmuch-tree-get-message-properties))))))
+			  (notmuch-tree-get-message-properties))
+			 (t nil)))))
     (plist-get props prop)))
 
 (defun notmuch-show-get-message-id (&optional bare)
-- 
1.7.10.4