[RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[David Edmondson]

Latest gnus provides a new HTML renderer entirely in lisp. It requires
some minor but ugly scaffolding to allow use with notmuch.
---

This is pretty horrible, but works in the cases that I tested. It
would be useful if a few other people could test. Note that `cid:'
images included with a text/html part are not inserted in the correct
place, but that also appears to be the case for gnus.

This should be tagged `notmuch::wip' in the new world order!

 emacs/notmuch-show.el |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 63b01e5..f52f233 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -320,6 +320,13 @@ message at DEPTH in the current thread."
 	;; ange-ftp, which is reasonable to use here.
 	(mm-write-region (point-min) (point-max) file nil nil nil 'no-conversion t)))))
 
+;; Nonsense required to have the new gnus `shr' HTML display code
+;; work.
+(defvar gnus-summary-buffer)
+(defvar gnus-inhibit-images)
+(if (not (fboundp 'gnus-blocked-images))
+    (defun gnus-blocked-images () nil))
+
 (defun notmuch-show-mm-display-part-inline (msg part nth content-type)
   "Use the mm-decode/mm-view functions to display a part in the
 current buffer, if possible."
@@ -331,7 +338,12 @@ current buffer, if possible."
 	    (let ((content (notmuch-show-get-bodypart-content msg part nth)))
 	      (insert content)
 	      (set-buffer display-buffer)
-	      (mm-display-part handle)
+
+	      ;; Nonsense required to have the new gnus `shr' HTML
+	      ;; display code work.
+	      (let ((gnus-inhibit-images nil))
+		(makunbound 'gnus-summary-buffer) ; Blech.
+		(mm-display-part handle))
 	      t)
 	  nil)))))
 
-- 
1.7.7.3

Re: [RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[Aaron Ecay]

David,

This patch doesn’t allow users to have their own settings for
shr-{inhibit,block}-images, since it forces these values to nil (via the
corresponding gnus variables).

(For those looking to follow the code, here’s the call path:
notmuch-show-mm-display-part-inline ->
mm-display-part ->
mm-display-inline -> (via lookup table)
mm-inline-text-html -> (via LUT)
mm-shr
The last function is where this objectionable rebinding happens)

Would a better approach be:
- clone the mm-shr function, replacing the vile gnus-specific code with
  something functionally similar that takes the values from notmuch-foo
  variables instead
- add '(notmuch-shr . notmuch-mm-shr) to the mm-text-html-renderer-alist
  variable
- tell notmuch users to customize mm-text-html-renderer to 'notmuch-shr
  if they want to use this renderer

The default values of the notmuch-{inhibit,block}-images variables
should prevent network requests for images being made by default* – your
proposed nil/nil values don’t do this, AFAICT.  Not doing network
requests for HTML email has been Spam Mitigation/Identity Protection 101
for a long time;** notmuch’s default behavior in this area has always
bothered me.

(Also, while we are on the subject of wonky defaults, why is
notmuch-show-all-multipart/alternative-parts set to t by default?  This
is certainly not the best setting.)

FWIW, the patch does seem to allow using shr as the HTML renderer, but
for the reasons expressed I don’t think it solves the broader problem.

Aaron

* I think that inhibit -> nil, block -> ".*" should achieve this, but I’m
  not sure/haven’t tested

** For example, Gmail makes you click a link at the top of an email
   before it will load any network images contained therein.

-- 
Aaron Ecay

Re: [RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[Chris Gray]

On Mon, 19 Dec 2011 15:45:59 +0000, David Edmondson <dme@dme.org> wrote:
> Latest gnus provides a new HTML renderer entirely in lisp. It requires
> some minor but ugly scaffolding to allow use with notmuch.
> ---
> 
> This is pretty horrible, but works in the cases that I tested. It
> would be useful if a few other people could test. Note that `cid:'
> images included with a text/html part are not inserted in the correct
> place, but that also appears to be the case for gnus.
> 
> This should be tagged `notmuch::wip' in the new world order!
> 
>  emacs/notmuch-show.el |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 63b01e5..f52f233 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -320,6 +320,13 @@ message at DEPTH in the current thread."
>  	;; ange-ftp, which is reasonable to use here.
>  	(mm-write-region (point-min) (point-max) file nil nil nil 'no-conversion t)))))
>  
> +;; Nonsense required to have the new gnus `shr' HTML display code
> +;; work.
> +(defvar gnus-summary-buffer)
> +(defvar gnus-inhibit-images)

I believe that gnus-inhibit-images is defined in gnus-art.el.  Perhaps
that file should just be require'd rather than having this workaround.

> +(if (not (fboundp 'gnus-blocked-images))
> +    (defun gnus-blocked-images () nil))
> +
>  (defun notmuch-show-mm-display-part-inline (msg part nth content-type)
>    "Use the mm-decode/mm-view functions to display a part in the
>  current buffer, if possible."
> @@ -331,7 +338,12 @@ current buffer, if possible."
>  	    (let ((content (notmuch-show-get-bodypart-content msg part nth)))
>  	      (insert content)
>  	      (set-buffer display-buffer)
> -	      (mm-display-part handle)
> +
> +	      ;; Nonsense required to have the new gnus `shr' HTML
> +	      ;; display code work.
> +	      (let ((gnus-inhibit-images nil))
> +		(makunbound 'gnus-summary-buffer) ; Blech.

This is working around a bug in gnus.  I think the better solution would
be for gnus to fix the bug.  The following patch against gnus works for
me.  (I have tried submitting it to the gnus bug list, but have not been
able to check if it got through.)

Cheers,
Chris

commit 45e5a06ea63d8d9f9a962db7d739c7d7056ef712 (HEAD, refs/heads/master)
Author: Chris Gray <chrismgray@gmail.com>
Date:   Mon Dec 19 23:33:30 2011 -0700

    mm-decode.el (mm-shr): Added check to bufferp for gnus-summary-buffer
    
    If gnus-summary-buffer is a string, then (buffer-name
    gnus-summary-buffer) gives an error.

	Modified lisp/mm-decode.el
diff --git a/lisp/mm-decode.el b/lisp/mm-decode.el
index 44e2af5..7005dd7 100644
--- a/lisp/mm-decode.el
+++ b/lisp/mm-decode.el
@@ -1724,6 +1724,7 @@ If RECURSIVE, search recursively."
 				      (buffer-string))))))
 	shr-inhibit-images shr-blocked-images charset char)
     (if (and (boundp 'gnus-summary-buffer)
+	     (bufferp gnus-summary-buffer)
 	     (buffer-name gnus-summary-buffer))
 	(with-current-buffer gnus-summary-buffer
 	  (setq shr-inhibit-images gnus-inhibit-images

Re: [RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[David Edmondson]

On Mon, 19 Dec 2011 23:38:36 -0700, Chris Gray <chrismgray@gmail.com> wrote:
> > +		(makunbound 'gnus-summary-buffer) ; Blech.
> 
> This is working around a bug in gnus.  I think the better solution would
> be for gnus to fix the bug.  The following patch against gnus works for
> me.  (I have tried submitting it to the gnus bug list, but have not been
> able to check if it got through.)

I wonder if `boundp' is just a typo for `bufferp'?

More on the other comments after thought.

dme.
-- 
David Edmondson, http://dme.org

Re: [RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[Aaron Ecay]

On Mon, 19 Dec 2011 23:38:36 -0700, Chris Gray <chrismgray@gmail.com> wrote:
> This is working around a bug in gnus.

Arguably this is true, but the real “bug” (conceptual error) is that the
MIME handling libraries and gnus are a little too tightly coupled.  Why
should notmuch users have to load gnus (gnus-art.el does (require 'gnus),
which brings in tens of thousands of lines of Elisp)[1], or customize
gnus-* variables to use general-purpose MIME viewing, HTML rendering
facilities?

The best GNUS-side solution would be to make mm-shr GNUS-agnostic, and
probably to introduce shr-{inhibit,blocked}-images as customizable
variables in their own right (which could inherit their values from the
gnus-* versions under the right circumstances).

I hope that the GNUS folks are receptive to this approach, but if they
aren’t I think it’s better for notmuch to not go the way of requiring
that GNUS be loaded to function.

Aaron

[1] I see that (featurep 'gnus) returns t for me, so that horse is
    already out of the barn.  But it isn’t something we should be
    seeking to perpetuate.

-- 
Aaron Ecay

Re: [RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[Chris Gray]

On Tue, 20 Dec 2011 08:35:42 +0000, David Edmondson <dme@dme.org> wrote:
> On Mon, 19 Dec 2011 23:38:36 -0700, Chris Gray <chrismgray@gmail.com> wrote:
> > > +		(makunbound 'gnus-summary-buffer) ; Blech.
> > 
> > This is working around a bug in gnus.  I think the better solution would
> > be for gnus to fix the bug.  The following patch against gnus works for
> > me.  (I have tried submitting it to the gnus bug list, but have not been
> > able to check if it got through.)
> 
> I wonder if `boundp' is just a typo for `bufferp'?

I originally thought so as well, but bufferp blows up if given an
unbound variable and buffer-name blows up if given a string.

Cheers,
Chris

Re: [RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[David Edmondson]

On Tue, 20 Dec 2011 07:27:24 -0700, Chris Gray <chrismgray@gmail.com> wrote:
> On Tue, 20 Dec 2011 08:35:42 +0000, David Edmondson <dme@dme.org> wrote:
> > On Mon, 19 Dec 2011 23:38:36 -0700, Chris Gray <chrismgray@gmail.com> wrote:
> > > > +		(makunbound 'gnus-summary-buffer) ; Blech.
> > > 
> > > This is working around a bug in gnus.  I think the better solution would
> > > be for gnus to fix the bug.  The following patch against gnus works for
> > > me.  (I have tried submitting it to the gnus bug list, but have not been
> > > able to check if it got through.)
> > 
> > I wonder if `boundp' is just a typo for `bufferp'?
> 
> I originally thought so as well, but bufferp blows up if given an
> unbound variable and buffer-name blows up if given a string.

When would `gnus-summary-buffer' ever be unbound?

Re: [RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[Chris Gray]

On Tue, 20 Dec 2011 14:33:01 +0000, David Edmondson <dme@dme.org> wrote:
> On Tue, 20 Dec 2011 07:27:24 -0700, Chris Gray <chrismgray@gmail.com> wrote:
> > On Tue, 20 Dec 2011 08:35:42 +0000, David Edmondson <dme@dme.org> wrote:
> > > On Mon, 19 Dec 2011 23:38:36 -0700, Chris Gray <chrismgray@gmail.com> wrote:
> > > > > +		(makunbound 'gnus-summary-buffer) ; Blech.
> > > > 
> > > > This is working around a bug in gnus.  I think the better solution would
> > > > be for gnus to fix the bug.  The following patch against gnus works for
> > > > me.  (I have tried submitting it to the gnus bug list, but have not been
> > > > able to check if it got through.)
> > > 
> > > I wonder if `boundp' is just a typo for `bufferp'?
> > 
> > I originally thought so as well, but bufferp blows up if given an
> > unbound variable and buffer-name blows up if given a string.
> 
> When would `gnus-summary-buffer' ever be unbound?

In the optimal situation where we were able to use mm-decode without
requiring gnus.el, then gnus-summary-buffer would be unbound.

Cheers,
Chris

Re: [RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[Chris Gray]

On Tue, 20 Dec 2011 03:40:44 -0500, Aaron Ecay <aaronecay@gmail.com> wrote:
> On Mon, 19 Dec 2011 23:38:36 -0700, Chris Gray <chrismgray@gmail.com> wrote:
> > This is working around a bug in gnus.
> 
> Arguably this is true, but the real “bug” (conceptual error) is that the
> MIME handling libraries and gnus are a little too tightly coupled.  Why
> should notmuch users have to load gnus (gnus-art.el does (require 'gnus),
> which brings in tens of thousands of lines of Elisp)[1], or customize
> gnus-* variables to use general-purpose MIME viewing, HTML rendering
> facilities?

I agree with this 100%.  However, we should still try to use emacs
coding best-practices, which means not defining variables that are
defined in other packages.  Unfortunately, getting the gnus folks to fix
their library might take a while, so it might be okay to relax those
best-practices while we wait, but we should be aware of the fact that
that's what we're doing.

Cheers,
Chris

Re: [RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[Florian Friesdorf]

On Mon, 19 Dec 2011 23:38:36 -0700, Chris Gray <chrismgray@gmail.com> wrote:
> > +;; Nonsense required to have the new gnus `shr' HTML display code
> > +;; work.
> > +(defvar gnus-summary-buffer)
> > +(defvar gnus-inhibit-images)
> 
> I believe that gnus-inhibit-images is defined in gnus-art.el.  Perhaps
> that file should just be require'd rather than having this workaround.

With notmuch-0.11 (require 'gnus-art') "fixes" the void
gnus-inhibit-images error.

> > +(if (not (fboundp 'gnus-blocked-images))
> > +    (defun gnus-blocked-images () nil))
> > +
> >  (defun notmuch-show-mm-display-part-inline (msg part nth content-type)
> >    "Use the mm-decode/mm-view functions to display a part in the
> >  current buffer, if possible."
> > @@ -331,7 +338,12 @@ current buffer, if possible."
> >  	    (let ((content (notmuch-show-get-bodypart-content msg part nth)))
> >  	      (insert content)
> >  	      (set-buffer display-buffer)
> > -	      (mm-display-part handle)
> > +
> > +	      ;; Nonsense required to have the new gnus `shr' HTML
> > +	      ;; display code work.
> > +	      (let ((gnus-inhibit-images nil))
> > +		(makunbound 'gnus-summary-buffer) ; Blech.
> 
> This is working around a bug in gnus.  I think the better solution would
> be for gnus to fix the bug.  The following patch against gnus works for
> me.  (I have tried submitting it to the gnus bug list, but have not been
> able to check if it got through.)

emacs git master has a fix for that, so you seem to have got through.

-- 
Florian Friesdorf <flo@chaoflow.net>
  GPG FPR: 7A13 5EEE 1421 9FC2 108D  BAAF 38F8 99A3 0C45 F083
Jabber/XMPP: flo@chaoflow.net
IRC: chaoflow on freenode,ircnet,blafasel,OFTC

Re: [RFC][PATCH] emacs: Provide scaffolding so that the new `shr' HTML renderer can run.

[Chris Gray]

On Mon, 23 Jan 2012 15:43:08 +0100, Florian Friesdorf <flo@chaoflow.net> wrote:
> With notmuch-0.11 (require 'gnus-art') "fixes" the void
> gnus-inhibit-images error.

This is true, though as Aaron Ecay pointed out in the discussion of this
patch, it does bring in all of gnus.  My current workaround to the bug
of gnus-inhibit-images being void is to load gnus, so that's just as
bad, but it would be great if we didn't have to do that.

> emacs git master has a fix for that, so you seem to have got through.

I got a notification that it went in the gnus git repo a couple of weeks
ago, so it is nice to know that it has been propagated to the emacs repo
as well.

Cheers,
Chris