* [RFC PATCH] emacs: cl-letf enriched-decode-display-prop for text/encriched display
@ 2017-09-16 9:18 Tomi Ollila
2017-09-20 5:25 ` [PATCH v2] " Tomi Ollila
0 siblings, 1 reply; 6+ messages in thread
From: Tomi Ollila @ 2017-09-16 9:18 UTC (permalink / raw)
To: notmuch; +Cc: tomi.ollila
Dynamically bind enriched-decode-display-prop when inserting
text/enriched part. This complements commit 9b0582383833 for
emacs versions before 24.4 which do not have advice-add functionality.
---
This is sent as RFC, as I did not (yet) have time to generate/find some
data to test it... anyway to me this looks good (on digital paper ;)
A couple of days ago I spent a little time to find how cl-flet & cl-letf
works -- cl-flet cannot be used as replacement for flet, since former
works in lexical scope... and accorging to this page
http://endlessparentheses.com/understanding-letf-and-how-it-replaces-flet.html
the (cl-letf (((symbol-function 'enriched-decode-display-prop) ...
should do the trick...
(ok, now I grepped notmuch code (again). It seens we're already using
letf & cl-letf and there is no reference to flet ;/ (I thought there were
when I grepped last time) -- well good learning experience...)
Tomi
emacs/notmuch-show.el | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 99390277..9ebd50ed 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -773,14 +773,11 @@ (defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth b
(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth button)
(notmuch-show-insert-part-text/calendar msg part content-type nth depth button))
-;; https://bugs.gnu.org/28350
-(defun notmuch-show--enriched-decode-display-prop (start end &optional param)
- (list start end))
-
(defun notmuch-show-insert-part-text/enriched (msg part content-type nth depth button)
- (advice-add 'enriched-decode-display-prop :override
- #'notmuch-show--enriched-decode-display-prop)
- nil)
+ ;; https://bugs.gnu.org/28350
+ (cl-letf (((symbol-function 'enriched-decode-display-prop)
+ (lambda (start end &optional param) (list start end))))
+ (notmuch-show-insert-part-*/* msg part content-type nth depth button)))
(defun notmuch-show-get-mime-type-of-application/octet-stream (part)
;; If we can deduce a MIME type from the filename of the attachment,
--
2.13.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] emacs: cl-letf enriched-decode-display-prop for text/encriched display
2017-09-16 9:18 [RFC PATCH] emacs: cl-letf enriched-decode-display-prop for text/encriched display Tomi Ollila
@ 2017-09-20 5:25 ` Tomi Ollila
2017-09-20 7:49 ` David Edmondson
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Tomi Ollila @ 2017-09-20 5:25 UTC (permalink / raw)
To: notmuch; +Cc: tomi.ollila
Dynamically bind enriched-decode-display-prop when inserting
text/enriched part. This complements commit 9b0582383833 for
emacs versions before 24.4 which do not have advice-add functionality.
Note the (require 'enriched). Without that if 'enriched were not
already provided, the part processing via mm-display-part would
eventually load that, providing enriched-decode-display-prop
overriding our temporary definition (for the first time
text/enriched part is handled).
---
I felt mentioning this (require 'enriched) important enough to be
stored in the blockchain of notmuch commit history -- knowing such
a subtle behaviour may prevent related bugs somewhere, sometime...
I've now tested this on Emacs 25.2.1 I also tried on Emacs 23.1.1
but there my environment gave:
$ EMACS=/usr/bin/emacs ./devel/try-emacs-mua -Q
+ exec /usr/bin/emacs --debug-init --load ./devel/try-emacs-mua -Q
Fatal error (6)zsh: abort EMACS=/usr/bin/emacs ./devel/try-emacs-mua -Q
(not immediately, but when trying to see search output...)
anyway, now I've checked:
$ less emacs-25.2/lisp/textmodes/enriched.el
$ less emacs-24.3/lisp/textmodes/enriched.el
$ less emacs-23.1/lisp/textmodes/enriched.el
and the implementation is same enough to know that this works
similarly on all of these emacs versions...
For 0.26 it is time to explicitly drop support for emacs 23
and deprecate emacs versions before 24.4...
emacs/notmuch-show.el | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 993902770095..1514eca57f43 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -773,14 +773,12 @@ (defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth b
(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth button)
(notmuch-show-insert-part-text/calendar msg part content-type nth depth button))
-;; https://bugs.gnu.org/28350
-(defun notmuch-show--enriched-decode-display-prop (start end &optional param)
- (list start end))
-
(defun notmuch-show-insert-part-text/enriched (msg part content-type nth depth button)
- (advice-add 'enriched-decode-display-prop :override
- #'notmuch-show--enriched-decode-display-prop)
- nil)
+ ;; https://bugs.gnu.org/28350
+ (require 'enriched)
+ (cl-letf (((symbol-function 'enriched-decode-display-prop)
+ (lambda (start end &optional param) (list start end))))
+ (notmuch-show-insert-part-*/* msg part content-type nth depth button)))
(defun notmuch-show-get-mime-type-of-application/octet-stream (part)
;; If we can deduce a MIME type from the filename of the attachment,
--
2.13.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] emacs: cl-letf enriched-decode-display-prop for text/encriched display
2017-09-20 5:25 ` [PATCH v2] " Tomi Ollila
@ 2017-09-20 7:49 ` David Edmondson
2017-09-20 11:12 ` David Bremner
2017-10-04 20:24 ` [PATCH v3] emacs: letf " Tomi Ollila
2 siblings, 0 replies; 6+ messages in thread
From: David Edmondson @ 2017-09-20 7:49 UTC (permalink / raw)
To: Tomi Ollila, notmuch
On Wednesday, 2017-09-20 at 08:25:44 +0300, Tomi Ollila wrote:
> I felt mentioning this (require 'enriched) important enough to be
> stored in the blockchain of notmuch commit history -- knowing such
> a subtle behaviour may prevent related bugs somewhere, sometime...
Could you add a comment in the code as well, please?
dme.
--
Freedom is just a song by Wham!.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] emacs: cl-letf enriched-decode-display-prop for text/encriched display
2017-09-20 5:25 ` [PATCH v2] " Tomi Ollila
2017-09-20 7:49 ` David Edmondson
@ 2017-09-20 11:12 ` David Bremner
2017-10-04 20:24 ` [PATCH v3] emacs: letf " Tomi Ollila
2 siblings, 0 replies; 6+ messages in thread
From: David Bremner @ 2017-09-20 11:12 UTC (permalink / raw)
To: Tomi Ollila, notmuch; +Cc: tomi.ollila
Tomi Ollila <tomi.ollila@iki.fi> writes:
> Dynamically bind enriched-decode-display-prop when inserting
> text/enriched part. This complements commit 9b0582383833 for
> emacs versions before 24.4 which do not have advice-add functionality.
>
> Note the (require 'enriched). Without that if 'enriched were not
> already provided, the part processing via mm-display-part would
> eventually load that, providing enriched-decode-display-prop
> overriding our temporary definition (for the first time
> text/enriched part is handled).
> ---
What about a test to show that extending support to older versions of
emacs doesn't cause a regression?
d
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] emacs: letf enriched-decode-display-prop for text/encriched display
2017-09-20 5:25 ` [PATCH v2] " Tomi Ollila
2017-09-20 7:49 ` David Edmondson
2017-09-20 11:12 ` David Bremner
@ 2017-10-04 20:24 ` Tomi Ollila
2017-11-08 19:34 ` David Bremner
2 siblings, 1 reply; 6+ messages in thread
From: Tomi Ollila @ 2017-10-04 20:24 UTC (permalink / raw)
To: notmuch; +Cc: tomi.ollila
Dynamically bind enriched-decode-display-prop when inserting
text/enriched part. This complements commit 9b0582383833 for
emacs versions before 24.4 which do not have advice-add
functionality.
Since emacs 25.3 this particular bug is fixed.
---
V3 since id:20170920052544.2893-1-tomi.ollila@iki.fi
* added version< check to apply this fix for emacsen before 25.3
* changed cl-letf to letf for emacs 23.x support
* added comments why (require 'enriched) was needed
(asked by dme)
db asked for test -- after a new moments of brief thinking I
could not come up with good robust way to test this. I now
tested this using debian 7.11 container (emacs 23.4),
centos 7.0 container (emacs 24.3) and then self-compiled
emacs 25.3 for fedora 26. the change worked as expected and
I don't see a way how this could cause a regression.
emacs/notmuch-show.el | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 993902770095..7acdfd7542e5 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -773,14 +773,16 @@ (defun notmuch-show-insert-part-text/calendar (msg part content-type nth depth b
(defun notmuch-show-insert-part-text/x-vcalendar (msg part content-type nth depth button)
(notmuch-show-insert-part-text/calendar msg part content-type nth depth button))
-;; https://bugs.gnu.org/28350
-(defun notmuch-show--enriched-decode-display-prop (start end &optional param)
- (list start end))
-
-(defun notmuch-show-insert-part-text/enriched (msg part content-type nth depth button)
- (advice-add 'enriched-decode-display-prop :override
- #'notmuch-show--enriched-decode-display-prop)
- nil)
+(if (version< emacs-version "25.3")
+ ;; https://bugs.gnu.org/28350
+ (defun notmuch-show-insert-part-text/enriched (msg part content-type nth depth button)
+ ;; By requiring enriched below, we ensure that the function enriched-decode-display-prop
+ ;; is defined before it will be shadowed by the letf below. Otherwise the version
+ ;; in enriched.el may be loaded a bit later and used instead (for the first time).
+ (require 'enriched)
+ (letf (((symbol-function 'enriched-decode-display-prop)
+ (lambda (start end &optional param) (list start end))))
+ (notmuch-show-insert-part-*/* msg part content-type nth depth button))))
(defun notmuch-show-get-mime-type-of-application/octet-stream (part)
;; If we can deduce a MIME type from the filename of the attachment,
--
2.13.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] emacs: letf enriched-decode-display-prop for text/encriched display
2017-10-04 20:24 ` [PATCH v3] emacs: letf " Tomi Ollila
@ 2017-11-08 19:34 ` David Bremner
0 siblings, 0 replies; 6+ messages in thread
From: David Bremner @ 2017-11-08 19:34 UTC (permalink / raw)
To: Tomi Ollila, notmuch; +Cc: tomi.ollila
Tomi Ollila <tomi.ollila@iki.fi> writes:
> db asked for test -- after a new moments of brief thinking I
> could not come up with good robust way to test this. I now
> tested this using debian 7.11 container (emacs 23.4),
> centos 7.0 container (emacs 24.3) and then self-compiled
> emacs 25.3 for fedora 26. the change worked as expected and
> I don't see a way how this could cause a regression.
I found it confusing that notmuch-show-insert-part-text/enriched is only
defined for versions less than 25.3. After a bit I realized this has to
do with notmuch-show-handlers for. I'm not sure the best clarification,
maybe just a comment specifying what the fallback is in the newer case?
d
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-11-08 19:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-16 9:18 [RFC PATCH] emacs: cl-letf enriched-decode-display-prop for text/encriched display Tomi Ollila
2017-09-20 5:25 ` [PATCH v2] " Tomi Ollila
2017-09-20 7:49 ` David Edmondson
2017-09-20 11:12 ` David Bremner
2017-10-04 20:24 ` [PATCH v3] emacs: letf " Tomi Ollila
2017-11-08 19:34 ` David Bremner
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).