unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH] emacs: tree/show remove duplicate function
@ 2014-06-07  9:51 Mark Walters
  2014-06-07 13:42 ` Tomi Ollila
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Mark Walters @ 2014-06-07  9:51 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] emacs: tree/show remove duplicate function
  2014-06-07  9:51 [PATCH] emacs: tree/show remove duplicate function Mark Walters
@ 2014-06-07 13:42 ` Tomi Ollila
  2014-07-15  1:07 ` David Bremner
  2014-07-16  9:39 ` David Bremner
  2 siblings, 0 replies; 8+ messages in thread
From: Tomi Ollila @ 2014-06-07 13:42 UTC (permalink / raw)
  To: Mark Walters, notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] emacs: tree/show remove duplicate function
  2014-06-07  9:51 [PATCH] emacs: tree/show remove duplicate function Mark Walters
  2014-06-07 13:42 ` Tomi Ollila
@ 2014-07-15  1:07 ` David Bremner
  2014-07-15  7:14   ` Mark Walters
  2014-07-16  9:39 ` David Bremner
  2 siblings, 1 reply; 8+ messages in thread
From: David Bremner @ 2014-07-15  1:07 UTC (permalink / raw)
  To: Mark Walters, notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] emacs: tree/show remove duplicate function
  2014-07-15  1:07 ` David Bremner
@ 2014-07-15  7:14   ` Mark Walters
  2014-07-15  9:43     ` David Bremner
  2014-07-15 23:29     ` David Bremner
  0 siblings, 2 replies; 8+ messages in thread
From: Mark Walters @ 2014-07-15  7:14 UTC (permalink / raw)
  To: David Bremner, notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] emacs: tree/show remove duplicate function
  2014-07-15  7:14   ` Mark Walters
@ 2014-07-15  9:43     ` David Bremner
  2014-07-15 23:29     ` David Bremner
  1 sibling, 0 replies; 8+ messages in thread
From: David Bremner @ 2014-07-15  9:43 UTC (permalink / raw)
  To: Mark Walters, notmuch

Mark Walters <markwalters1009@gmail.com> writes:

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

Your call. 

d

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] emacs: tree/show remove duplicate function
  2014-07-15  7:14   ` Mark Walters
  2014-07-15  9:43     ` David Bremner
@ 2014-07-15 23:29     ` David Bremner
  1 sibling, 0 replies; 8+ messages in thread
From: David Bremner @ 2014-07-15 23:29 UTC (permalink / raw)
  To: Mark Walters, notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] emacs: tree/show remove duplicate function
  2014-06-07  9:51 [PATCH] emacs: tree/show remove duplicate function Mark Walters
  2014-06-07 13:42 ` Tomi Ollila
  2014-07-15  1:07 ` David Bremner
@ 2014-07-16  9:39 ` David Bremner
  2014-07-16 18:03   ` [PATCH] emacs: show: make return value of notmuch-show-get-prop explicit Mark Walters
  2 siblings, 1 reply; 8+ messages in thread
From: David Bremner @ 2014-07-16  9:39 UTC (permalink / raw)
  To: Mark Walters, notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] emacs: show: make return value of notmuch-show-get-prop explicit
  2014-07-16  9:39 ` David Bremner
@ 2014-07-16 18:03   ` Mark Walters
  0 siblings, 0 replies; 8+ messages in thread
From: Mark Walters @ 2014-07-16 18:03 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-07-16 18:03 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-07  9:51 [PATCH] emacs: tree/show remove duplicate function Mark Walters
2014-06-07 13:42 ` Tomi Ollila
2014-07-15  1:07 ` David Bremner
2014-07-15  7:14   ` Mark Walters
2014-07-15  9:43     ` David Bremner
2014-07-15 23:29     ` David Bremner
2014-07-16  9:39 ` David Bremner
2014-07-16 18:03   ` [PATCH] emacs: show: make return value of notmuch-show-get-prop explicit Mark Walters

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).