[PATCH] emacs: show: possible w3m/invisibility workaround

[PATCH] emacs: show: possible w3m/invisibility workaround

[Mark Walters]

There is a bug in the current notmuch code with w3m and invisible
parts. w3m sets a keymap, and if we have a hiddent [text/html] point
at the start of the following line still gets this w3m keymap which
causes some strange effects. For example, RET gives an error "No URL
at Point" rather than hiding the message, <down> goes to the next link
rather than just down a line.

This is only likely to be a problem for emacs 23 as shr is preferred
as html renderer on emacs 24. I do not know the correct solution but
this might be a safe work around for 0.15.

This bug was reported on irc by awg.  
---
 emacs/notmuch-show.el |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5751d98..f526fbf 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -818,6 +818,19 @@ message at DEPTH in the current thread."
 (defun notmuch-show-insert-part-inline-patch-fake-part (msg part content-type nth depth declared-type)
   (notmuch-show-insert-part-*/* msg part "text/x-diff" nth depth "inline patch"))
 
+(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type)
+  ;; text/html to work around bugs in renderers and our invisibile
+  ;; parts code. In particular w3m sets up a keymap which "leaks"
+  ;; outside the invisible region and causes strange effects in
+  ;; notmuch. Thus we override w3m and replace it as w3m-standalone.
+  (let ((mm-text-html-renderer
+	 (if (eq mm-text-html-renderer 'w3m)
+	     'w3m-standalone
+	   mm-text-html-renderer)))
+    (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
+    (notmuch-mm-display-part-inline msg part nth content-type notmuch-show-process-crypto))
+  t)
+
 (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
   ;; This handler _must_ succeed - it is the handler of last resort.
   (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
-- 
1.7.9.1

[PATCH] emacs: Fix point motion in `beginning-of-visual-line'

[Austin Clements]

`beginning-of-visual-line' interacts poorly with our use of invisible
overlays and often moves point into the previous message or part even
though it looks like it's in the next message or part (e.g., if you
press C-a on a header line and the previous message is invisible,
point will move into the previous message, even though it appears to
still be on the header line).  This advises `beginning-of-visual-line'
to address this behavior.
---

This is a completely different approach that should fix the HTML
problem, as well as other problems.  I also have an alternate approach
to this that sets 'field properties on the header line and part
buttons, which has the same effect, but seems to have more
side-effects (e.g., if point is at the end of a part button, C-a won't
go to the beginning).  This patch, OTOH, strikes directly at the
strange (maybe even buggy) behavior of beginning-of-visual-line.  The
downside is that it's advice, and advice always feels a little
sketchy.

 emacs/notmuch-show.el |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5751d98..6f5c53a 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -1120,6 +1120,28 @@ function is used."
     (notmuch-show-goto-first-wanted-message)
     (current-buffer)))
 
+(defadvice beginning-of-visual-line (around constrain-invisible activate)
+  "Fix point motion around invisible overlays.
+
+In `notmuch-show-mode', we often hide overlays that end with a
+newline (e.g., messages, parts, etc).  This has the effect of
+collapsing the overlayed text into the beginning of the next
+line.  Unfortunately, this causes `visual-motion' and hence
+`beginning-of-visual-line' to move to the first character of the
+invisible overlay rather than the more obvious first character of
+the visible line.  Visually, these two points are
+indistinguishable, but this tends to make actions mysteriously
+apply to the previous message when it looks like they should
+apply to the next message.
+
+This advice fixes this behavior in `notmuch-show-mode' by
+restricting the motion of `beginning-of-visual-line'."
+  (if (eq major-mode 'notmuch-show-mode)
+      (save-restriction
+	(narrow-to-region (line-beginning-position) (point-max))
+	ad-do-it)
+    ad-do-it))
+
 (defun notmuch-show-build-buffer ()
   (let ((inhibit-read-only t))
 
-- 
1.7.10.4

Re: [PATCH] emacs: Fix point motion in `beginning-of-visual-line'

[Austin Clements]

After some IRC discussion, Mark and I have found that my patch doesn't
fix everything related to w3m that Mark's patch does (though my patch
does fix other things not related to w3m).

Quoth myself on Jan 10 at  4:59 pm:
> `beginning-of-visual-line' interacts poorly with our use of invisible
> overlays and often moves point into the previous message or part even
> though it looks like it's in the next message or part (e.g., if you
> press C-a on a header line and the previous message is invisible,
> point will move into the previous message, even though it appears to
> still be on the header line).  This advises `beginning-of-visual-line'
> to address this behavior.
> ---
> 
> This is a completely different approach that should fix the HTML
> problem, as well as other problems.  I also have an alternate approach
> to this that sets 'field properties on the header line and part
> buttons, which has the same effect, but seems to have more
> side-effects (e.g., if point is at the end of a part button, C-a won't
> go to the beginning).  This patch, OTOH, strikes directly at the
> strange (maybe even buggy) behavior of beginning-of-visual-line.  The
> downside is that it's advice, and advice always feels a little
> sketchy.
> 
>  emacs/notmuch-show.el |   22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5751d98..6f5c53a 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -1120,6 +1120,28 @@ function is used."
>      (notmuch-show-goto-first-wanted-message)
>      (current-buffer)))
>  
> +(defadvice beginning-of-visual-line (around constrain-invisible activate)
> +  "Fix point motion around invisible overlays.
> +
> +In `notmuch-show-mode', we often hide overlays that end with a
> +newline (e.g., messages, parts, etc).  This has the effect of
> +collapsing the overlayed text into the beginning of the next
> +line.  Unfortunately, this causes `visual-motion' and hence
> +`beginning-of-visual-line' to move to the first character of the
> +invisible overlay rather than the more obvious first character of
> +the visible line.  Visually, these two points are
> +indistinguishable, but this tends to make actions mysteriously
> +apply to the previous message when it looks like they should
> +apply to the next message.
> +
> +This advice fixes this behavior in `notmuch-show-mode' by
> +restricting the motion of `beginning-of-visual-line'."
> +  (if (eq major-mode 'notmuch-show-mode)
> +      (save-restriction
> +	(narrow-to-region (line-beginning-position) (point-max))
> +	ad-do-it)
> +    ad-do-it))
> +
>  (defun notmuch-show-build-buffer ()
>    (let ((inhibit-read-only t))
>  

[PATCH v2] emacs: show: possible w3m/invisibility workaround

[Mark Walters]

There is a bug in the current notmuch code with w3m and invisible
parts. w3m sets a keymap, and if we have a hidden [text/html] point
at the start of the following line still gets this w3m keymap which
causes some strange effects. For example, RET gives an error "No URL
at Point" rather than hiding the message, <down> goes to the next link
rather than just down a line.

These keybinding are also inconvenient when the text/html part is
displayed so we replace the w3m keymap by notmuch-show-mode-map.

This is only likely to be a problem for emacs 23 as shr is preferred
as html renderer on emacs 24.
---
This is probably better than v1 as this keeps the users chosen
renderer and justs overrides the (in my opinion) inconvenient (for
notmuch-show use) keymap w3m provides.

There are some questions:

1) does anyone use w3m with notmuch and really want the keymap not overridden?

2) is copying notmuch-show-mode-map needed?

3) notmuch-show-mode-map is not defined at this point in notmuch-show
(but obviously is when the function is called). What is the best way
to avoid a compile warning?

This seems to me like a plausible basis for a solution for 0.15. This is not a
full fix but does hide the problem. Moreover even with a genuine fix
for the invisibility corner cases I think this might be desirable for
the case when the text/html part is displayed.

Best wishes

Mark


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

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5751d98..8443c54 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -818,6 +818,19 @@ message at DEPTH in the current thread."
 (defun notmuch-show-insert-part-inline-patch-fake-part (msg part content-type nth depth declared-type)
   (notmuch-show-insert-part-*/* msg part "text/x-diff" nth depth "inline patch"))
 
+(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type)
+  ;; text/html to work around bugs in renderers and our invisibile
+  ;; parts code. In particular w3m sets up a keymap which "leaks"
+  ;; outside the invisible region and causes strange effects in
+  ;; notmuch. The w3m keymap is rather odd for notmuch use: several
+  ;; keys conflict with notmuch-show-mode-map so just override by
+  ;; notmuch-show-mode-map.
+  (let ((w3m-minor-mode-map (when (eq mm-text-html-renderer 'w3m)
+			      (copy-keymap notmuch-show-mode-map))))
+    (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
+    (notmuch-mm-display-part-inline msg part nth content-type notmuch-show-process-crypto))
+  t)
+
 (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
   ;; This handler _must_ succeed - it is the handler of last resort.
   (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
-- 
1.7.9.1

[PATCH v3] emacs: show: possible w3m/invisibility workaround

[Mark Walters]

There is a bug in the current notmuch code with w3m and invisible
parts. w3m sets a keymap, and if we have a hidden [text/html] point
at the start of the following line still gets this w3m keymap which
causes some strange effects. For example, RET gives an error "No URL
at Point" rather than hiding the message, <down> goes to the next link
rather than just down a line.

These keybinding are also inconvenient when the text/html part is
displayed so we ask w3m not to install a keymap.

This is only likely to be a problem for emacs 23 as shr is preferred
as html renderer on emacs 24 (although the user can set the renderer
to w3m even on emacs 24).

This solution was suggested by Tomi Ollila <tomi.ollila@iki.fi>
---

On irc Tomi found the correct way of stopping w3m setting a keymap:
there is a variable mm-inline-text-html-with-w3m-keymap which can be
set to nil to tell w3m not to set a keymap.

I think this is the best solution to this bug for 0.15. My view is
that this is correct anyway: w3m does render quickly and nicely (and
will probably be my default renderer from now on) and with this patch
the keybinding problems are the normal notmuch bindings.

Jani pointed out that in the new invisibility setup all text/html
parts are rendered (invisibly) when notmuch-show runs. We may want to
allow the user to supress this: in fact they can by setting
mm-text-html-renderer to nil but we may want to allow the suppression
without affecting gnus etc, or we may want to link this option into
the notmuch-show customization, or maybe just mention in NEWS.

Any such fix is a obviously a separate patch.

Best wishes

Mark



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

diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
index 5751d98..1864dd1 100644
--- a/emacs/notmuch-show.el
+++ b/emacs/notmuch-show.el
@@ -818,6 +818,16 @@ message at DEPTH in the current thread."
 (defun notmuch-show-insert-part-inline-patch-fake-part (msg part content-type nth depth declared-type)
   (notmuch-show-insert-part-*/* msg part "text/x-diff" nth depth "inline patch"))
 
+(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type)
+  ;; text/html handler to work around bugs in renderers and our
+  ;; invisibile parts code. In particular w3m sets up a keymap which
+  ;; "leaks" outside the invisible region and causes strange effects
+  ;; in notmuch. We set mm-inline-text-html-with-w3m-keymap to nil to
+  ;; tell w3m not to set a keymap (so the normal notmuch-show-mode-map
+  ;; remains).
+  (let ((mm-inline-text-html-with-w3m-keymap nil))
+    (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type)))
+
 (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
   ;; This handler _must_ succeed - it is the handler of last resort.
   (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
-- 
1.7.9.1

Re: [PATCH v3] emacs: show: possible w3m/invisibility workaround

[Tomi Ollila]

On Sun, Jan 13 2013, Mark Walters <markwalters1009@gmail.com> wrote:

> There is a bug in the current notmuch code with w3m and invisible
> parts. w3m sets a keymap, and if we have a hidden [text/html] point
> at the start of the following line still gets this w3m keymap which
> causes some strange effects. For example, RET gives an error "No URL
> at Point" rather than hiding the message, <down> goes to the next link
> rather than just down a line.
>
> These keybinding are also inconvenient when the text/html part is
> displayed so we ask w3m not to install a keymap.
>
> This is only likely to be a problem for emacs 23 as shr is preferred
> as html renderer on emacs 24 (although the user can set the renderer
> to w3m even on emacs 24).
>
> This solution was suggested by Tomi Ollila <tomi.ollila@iki.fi>
> ---

LGTM.

> On irc Tomi found the correct way of stopping w3m setting a keymap:
> there is a variable mm-inline-text-html-with-w3m-keymap which can be
> set to nil to tell w3m not to set a keymap.

I was lucky when looking through the list provided by
M-x customize-group mime-display to figure out whether the whole
group could be referenced from notmuch-show customization group
-- and happened to notice the customization variable in question.

I have previously discussed "against" picking up individiual external 
customization variables to notmuch groups (just due that I could not
find it done before). In case of notmuch-send, there is line
  (custom-add-to-group 'notmuch-send 'message 'custom-group)
in notmuch-lib.el -- as message sending uses message mode (and there
are plenty of useful customization there it makes sense to reference
the whole group. In case of mm-display functionality that doesn't
seem to be the case...

Tomi

> I think this is the best solution to this bug for 0.15. My view is
> that this is correct anyway: w3m does render quickly and nicely (and
> will probably be my default renderer from now on) and with this patch
> the keybinding problems are the normal notmuch bindings.
>
> Jani pointed out that in the new invisibility setup all text/html
> parts are rendered (invisibly) when notmuch-show runs. We may want to
> allow the user to supress this: in fact they can by setting
> mm-text-html-renderer to nil but we may want to allow the suppression
> without affecting gnus etc, or we may want to link this option into
> the notmuch-show customization, or maybe just mention in NEWS.
>
> Any such fix is a obviously a separate patch.
>
> Best wishes
>
> Mark
>
>
>
>  emacs/notmuch-show.el |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5751d98..1864dd1 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -818,6 +818,16 @@ message at DEPTH in the current thread."
>  (defun notmuch-show-insert-part-inline-patch-fake-part (msg part content-type nth depth declared-type)
>    (notmuch-show-insert-part-*/* msg part "text/x-diff" nth depth "inline patch"))
>  
> +(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type)
> +  ;; text/html handler to work around bugs in renderers and our
> +  ;; invisibile parts code. In particular w3m sets up a keymap which
> +  ;; "leaks" outside the invisible region and causes strange effects
> +  ;; in notmuch. We set mm-inline-text-html-with-w3m-keymap to nil to
> +  ;; tell w3m not to set a keymap (so the normal notmuch-show-mode-map
> +  ;; remains).
> +  (let ((mm-inline-text-html-with-w3m-keymap nil))
> +    (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type)))
> +
>  (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
>    ;; This handler _must_ succeed - it is the handler of last resort.
>    (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))
> -- 
> 1.7.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3] emacs: show: possible w3m/invisibility workaround

[Austin Clements]

LGTM.  It's too bad the w3m-link-map logic in
mm-inline-text-html-render-with-w3m doesn't appear to work; it looks
like that would make links follow-able without completely taking over.

Quoth Mark Walters on Jan 13 at 12:43 pm:
> There is a bug in the current notmuch code with w3m and invisible
> parts. w3m sets a keymap, and if we have a hidden [text/html] point
> at the start of the following line still gets this w3m keymap which
> causes some strange effects. For example, RET gives an error "No URL
> at Point" rather than hiding the message, <down> goes to the next link
> rather than just down a line.
> 
> These keybinding are also inconvenient when the text/html part is
> displayed so we ask w3m not to install a keymap.
> 
> This is only likely to be a problem for emacs 23 as shr is preferred
> as html renderer on emacs 24 (although the user can set the renderer
> to w3m even on emacs 24).
> 
> This solution was suggested by Tomi Ollila <tomi.ollila@iki.fi>
> ---
> 
> On irc Tomi found the correct way of stopping w3m setting a keymap:
> there is a variable mm-inline-text-html-with-w3m-keymap which can be
> set to nil to tell w3m not to set a keymap.
> 
> I think this is the best solution to this bug for 0.15. My view is
> that this is correct anyway: w3m does render quickly and nicely (and
> will probably be my default renderer from now on) and with this patch
> the keybinding problems are the normal notmuch bindings.
> 
> Jani pointed out that in the new invisibility setup all text/html
> parts are rendered (invisibly) when notmuch-show runs. We may want to
> allow the user to supress this: in fact they can by setting
> mm-text-html-renderer to nil but we may want to allow the suppression
> without affecting gnus etc, or we may want to link this option into
> the notmuch-show customization, or maybe just mention in NEWS.

Could we make it simply render parts on demand when/if they become
visible?  There would be no loss of functionality and things would be
faster overall.

> Any such fix is a obviously a separate patch.
> 
> Best wishes
> 
> Mark
> 
> 
> 
>  emacs/notmuch-show.el |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
> index 5751d98..1864dd1 100644
> --- a/emacs/notmuch-show.el
> +++ b/emacs/notmuch-show.el
> @@ -818,6 +818,16 @@ message at DEPTH in the current thread."
>  (defun notmuch-show-insert-part-inline-patch-fake-part (msg part content-type nth depth declared-type)
>    (notmuch-show-insert-part-*/* msg part "text/x-diff" nth depth "inline patch"))
>  
> +(defun notmuch-show-insert-part-text/html (msg part content-type nth depth declared-type)
> +  ;; text/html handler to work around bugs in renderers and our
> +  ;; invisibile parts code. In particular w3m sets up a keymap which
> +  ;; "leaks" outside the invisible region and causes strange effects
> +  ;; in notmuch. We set mm-inline-text-html-with-w3m-keymap to nil to
> +  ;; tell w3m not to set a keymap (so the normal notmuch-show-mode-map
> +  ;; remains).
> +  (let ((mm-inline-text-html-with-w3m-keymap nil))
> +    (notmuch-show-insert-part-*/* msg part content-type nth depth declared-type)))
> +
>  (defun notmuch-show-insert-part-*/* (msg part content-type nth depth declared-type)
>    ;; This handler _must_ succeed - it is the handler of last resort.
>    (notmuch-show-insert-part-header nth content-type declared-type (plist-get part :filename))

Re: [PATCH v3] emacs: show: possible w3m/invisibility workaround

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> There is a bug in the current notmuch code with w3m and invisible
> parts. w3m sets a keymap, and if we have a hidden [text/html] point
> at the start of the following line still gets this w3m keymap which
> causes some strange effects. For example, RET gives an error "No URL
> at Point" rather than hiding the message, <down> goes to the next link
> rather than just down a line.

pushed to relaese and master.

d

Re: [PATCH v3] emacs: show: possible w3m/invisibility workaround

[Adam Wolfe Gordon]

On Mon, Jan 14, 2013 at 4:52 PM, David Bremner <david@tethera.net> wrote:
> Mark Walters <markwalters1009@gmail.com> writes:
>
>> There is a bug in the current notmuch code with w3m and invisible
>> parts. w3m sets a keymap, and if we have a hidden [text/html] point
>> at the start of the following line still gets this w3m keymap which
>> causes some strange effects. For example, RET gives an error "No URL
>> at Point" rather than hiding the message, <down> goes to the next link
>> rather than just down a line.
>
> pushed to relaese and master.

I can confirm this solves the bug I was seeing.

-- Adam