[RFC PATCH] emacs: cl-letf enriched-decode-display-prop for text/encriched display

classic Classic list List threaded Threaded
6 messages Options
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

[RFC PATCH] emacs: cl-letf enriched-decode-display-prop for text/encriched display

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

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] emacs: cl-letf enriched-decode-display-prop for text/encriched display

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

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Edmondson David Edmondson
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] emacs: cl-letf enriched-decode-display-prop for text/encriched display

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!.
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] emacs: cl-letf enriched-decode-display-prop for text/encriched display

In reply to this post by Tomi Ollila-2
Tomi Ollila <[hidden email]> 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
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Tomi Ollila-2 Tomi Ollila-2
Reply | Threaded
Open this post in threaded view
|

[PATCH v3] emacs: letf enriched-decode-display-prop for text/encriched display

In reply to this post by Tomi Ollila-2
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:[hidden email]

* 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

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] emacs: letf enriched-decode-display-prop for text/encriched display

Tomi Ollila <[hidden email]> 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

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch