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