pull request

[David Edmondson]

On Wed, 21 Apr 2010 14:03:03 -0700, Carl Worth <cworth at cworth.org> wrote:
> > commit 2b6201fbf9209a875f216d48c30b95a6f583c575
> > 
> >     emacs: Add more functions to clean up text/plain parts
> >     
> >     Add:
> >     - notmuch-wash-wrap-long-lines: Wrap lines longer than the width of
> >       the current window whilst maintaining any citation prefix.
> >     - notmuch-wash-tidy-citations: Tidy up citations by:
> >       - compress repeated otherwise blank citation lines,
> >       - remove otherwise blank citation lines at the head and tail of a
> >         citation and remove blank lines between attribution statements and
> >         the citation,
> >     - notmuch-wash-compress-blanks: Compress repeated blank lines and
> >       remove leading and trailing blank lines.
> 
> I did not push this one. I think this munges messages too much to be on
> by default.

Your concerns appear to be about `notmuch-wash-wrap-long-lines'. I agree
that it can generate unfortunate results in the "deep thread, narrow
terminal" case. Are the other two functions okay?

One of the reasons that I chose a hook-based approach is to allow the
user to decide which body cleaning should be applied. If you don't want
the line-wrapping to be enabled by default we can leave it out but
include the definition so that a user can enable it. The inline-patch
formatting is similar (I'll say more about that in response to your
comments on it).

> Dealing with broken messages that have entire paragraphs unwrapped is a
> separate issue from displaying correct messages nicely. I'd like to make
> correct messages work well first, and then worry about the broken
> messages. (For those, I'm much more willing to accept munged display of
> the message.)

Enabling `notmuch-wash-tidy-citations' and
`notmuch-wash-compress-blanks' by default would fit with this policy, I
think.

Then again, including the functions in the code but enabling none of
them by default would also be acceptable (I already enable them all and
have another function that is not shared).

> > commit 9e193a3998b7503e35d21013c71cc4ecaf6c9d50
> > 
> >     emacs/notmuch-wash.el: Add `notmuch-wash-inline-patch'
> 
> The commit message for this one was enough to make me skip it for now:
> 
> >     Due to the scope for error in detecting inline patches (and their
> >     extent), this function is not enabled by default.
> 
> For this entire series it would be very nice to have some emacs-based
> testing in the test suite. And for a patch like this with heuristics
> that are known to not be perfect, I think it's essential to have that
> testing in place first.

Testing for Emacs is something that I'm looking into. There doesn't seem
to be a de-facto best framework.

Aside from the need to test, this particular patch will benefit only
minimally from testing - there will always be false positive cases
however good the analysis used. Are you saying that we can not tolerate
any false-positives, or that we just need to demonstrate that it is down
to some acceptable level? (Though I've no idea how I would achieve the
latter.)

> > commit 444de7e73d988cab9b8d1fef851c8ad26174a996
> > 
> >     emacs/notmuch-show.el: Part headers are real buttons that save the part
> 
> Pushed. This is a delightful improvement to our handling of attachments.
> 
> I'd like the text on the button to give some more instruction, such as:
> 
> 	"Click or press Enter to save."
> 
> And if we could pull it off, (I don't know how flexible the button
> widget is in emacs), it would be nice to have something like this as
> well:
> 
> 	"Or press V to view."
> 
> so that one could easily use an external viewer for a single attachment.

Agreed. There are a bunch of enhancements that I'd like to make here,
including toggling the visibility of part, viewing in another buffer,
view externally, force the type, etc.

> Meanwhile, another issue with the result of this series is that I now
> seem to get rendering for both the text/plain and the text/html
> alternatives when a message has both.

This is a consequence of `notmuch show' squashing the MIME details in
the message before output. In particular most messages which include
both text/plain and text/html parts are declared as
multipart/alternative, but the UI never gets to see that detail. Work on
improving that is happening (slowly) at:
	http://github.com/dme/notmuch/tree/multipart
(please don't start using this - the JSON output is mostly-okay, but
text output is broken and the Emacs code is incomplete!)

multipart/alternative suppor would fix this. multipart/related is
necessary for displaying HTML with inline images that are part of the
message ('cid:' images). multipart/signed and multipart/encrypted are
necessary for PGP (and others). If `notmuch' can be persuaded to include
the structure in the JSON output then I think that the Emacs code is in
reasonable shape to be extended to display things well.

> For now, the paragraphs are wrapped much more nicely in the rendering
> of the html portion, but links are apparently entirely missing. The
> link URLs at least appear in the text/plain rendering, (which is
> pretty ugly, but at least not impossible to use).

The HTML formatting depends somewhat on the tools available on your
system. Do you have w3m and w3m-el[1] installed? I'd be curious to see a
screenshot of a message that you think is poorly displayed.

> If we could get one version or the other working completely, then it
> would be nice to display only one.

See above - multipart/alternative support is required.

> > commit e9d737feb5a49fd59e1f27bccd24cac2fd1ef749
> > 
> >     emacs/notmuch-show.el: Add `notmuch-show-toggle-all' bound to M-RET
> >     
> >     `notmuch-show-toggle-all' changes the visibility all of the messages
> >     in the current thread. By default it makes all of the messages not
> >     visible. With a prefix argument, it makes them all visible.
> 
> I didn't push this one yet.
> 
> The feature is *almost* what I want. It's just that I often want to open
> all messages in a thread, (and I've never found myself wanting to close
> all messages). So I'd like to switch which behavior requires the prefix
> argument here.

Should I just re-submit with the sense reversed? After using it for a
few days the opposite approach seems more useful.

> Anyone, if you haven't tried that mode yet for customizing notmuch, you
> should give it a try. (And we should add some hint somewhere to make it
> easier to find. Perhaps a keybinding to get to the customization
> buffer.)

A button in `notmuch-hello' ?

Footnotes: 
[1]  Debian names - presumably similar on other systems.


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