New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POC: Respect format=flowed and delsp=yes for viewing plain-text messages #986

Merged
merged 1 commit into from Feb 5, 2017

Conversation

Projects
None yet
3 participants
@stig
Contributor

stig commented Dec 19, 2016

This is my initial attempt at addressing #808, supporting format=flowed (and delsp=yes) when viewing mail.

While this does rewrap some test messages I've tried nicely, I wouldn't recommend merging it in its current form:

  • There are almost certainly may be memory leaks here. Update: may not be true any more, after the last few commits on this branch.
  • I also encountered a situation where some mails didn't load. Update: I was able to track down why, and plugged that particular hole. It's running fine for me now.
  • I suspect we would also want a toggle to say whether to honour reflow, much as we do for sending.

I fear this approach is too simplistic because of the way mu accumulates plain-text bodies. This approach could produce very nasty results if some plain-text bodies are marked format=flowed and others are not, but then everything is format-flowed. This body accumulation happens in C, but I don't relish the idea of re-implementing flow-fill.el in C so we can choose to reflow individual body chunks while accumulating. I feel a better solution would be to delay the accumulation and, instead of providing separate html and text strings in mu-msg-sexp.c, just provide an array of bodies, each with their own content type, and handle the accumulation in emacs-lisp.

@stig stig changed the title from Respect format=flowed and delsp=yes for viewing plain-text messages to POC: Respect format=flowed and delsp=yes for viewing plain-text messages Dec 19, 2016

@Chris00

This comment has been minimized.

Show comment
Hide comment
@Chris00

Chris00 Dec 27, 2016

Contributor

I did not really dig into your code but it looks to me like a display issue so I am surprised there is quite a bit of C code involved. Did you have a look to the recommendations for Gnus? See also Emacs MIME.

Contributor

Chris00 commented Dec 27, 2016

I did not really dig into your code but it looks to me like a display issue so I am surprised there is quite a bit of C code involved. Did you have a look to the recommendations for Gnus? See also Emacs MIME.

@stig

This comment has been minimized.

Show comment
Hide comment
@stig

stig Dec 27, 2016

Contributor

Hi @Chris00 there's quite a bit of C code because mu doesn't currently (as far as I could see) expose any of the content-type params to mu4e; it just gives the text and html bodies without any parameters. As I understand it, without the format=flowed and delsp=yes params mu4e can't determine whether to reflow a message.

I don't quite understand the relevance of the Gnus resource you linked to. It seems to be talking about long lines, which is not what I want. I am aiming for body text to be reflowed to my desired fill column, not wrapping at the frame width.

Contributor

stig commented Dec 27, 2016

Hi @Chris00 there's quite a bit of C code because mu doesn't currently (as far as I could see) expose any of the content-type params to mu4e; it just gives the text and html bodies without any parameters. As I understand it, without the format=flowed and delsp=yes params mu4e can't determine whether to reflow a message.

I don't quite understand the relevance of the Gnus resource you linked to. It seems to be talking about long lines, which is not what I want. I am aiming for body text to be reflowed to my desired fill column, not wrapping at the frame width.

@Chris00

This comment has been minimized.

Show comment
Hide comment
@Chris00

Chris00 Dec 28, 2016

Contributor
Contributor

Chris00 commented Dec 28, 2016

Show outdated Hide outdated lib/mu-msg-sexp.c
Show outdated Hide outdated lib/mu-msg-sexp.c
Show outdated Hide outdated lib/mu-msg-sexp.c
Show outdated Hide outdated lib/mu-msg.c
Show outdated Hide outdated lib/mu-msg.c
Show outdated Hide outdated lib/mu-msg.c
Show outdated Hide outdated mu4e/mu4e-message.el
Show outdated Hide outdated lib/mu-msg.c
Show outdated Hide outdated mu4e/mu4e-message.el
@stig

This comment has been minimized.

Show comment
Hide comment
@stig

stig Jan 12, 2017

Contributor

Thanks for all the comments @djcb! I believe I have addressed all of them. Where do I take this from here?

Contributor

stig commented Jan 12, 2017

Thanks for all the comments @djcb! I believe I have addressed all of them. Where do I take this from here?

@djcb

This comment has been minimized.

Show comment
Hide comment
@djcb

djcb Jan 24, 2017

Owner

Ah, sorry for the late response. I like the code and you even added tests, very good!

I would like to merge it sometime soon, but one thing I'd like to do before that is add some generic transform function hook, and your flow-handling would could use that. So, give me a few days to add that; after that, I think we can get your change in (without need to further change mu4e-message-body-text. Cheers!

Owner

djcb commented Jan 24, 2017

Ah, sorry for the late response. I like the code and you even added tests, very good!

I would like to merge it sometime soon, but one thing I'd like to do before that is add some generic transform function hook, and your flow-handling would could use that. So, give me a few days to add that; after that, I think we can get your change in (without need to further change mu4e-message-body-text. Cheers!

@djcb

This comment has been minimized.

Show comment
Hide comment
@djcb

djcb Jan 30, 2017

Owner

There's mu4e-message-body-rewrite-functions now (see mu4e-message-outlook-cleanup for an example); so if you can update your code for that, we can take it in, thanks!

Owner

djcb commented Jan 30, 2017

There's mu4e-message-body-rewrite-functions now (see mu4e-message-outlook-cleanup for an example); so if you can update your code for that, we can take it in, thanks!

@stig

This comment has been minimized.

Show comment
Hide comment
@stig

stig Jan 31, 2017

Contributor

@djcb Thanks, I've rebased onto master and combined all the commits into one. Let me know if there's anything else you want from this!

Contributor

stig commented Jan 31, 2017

@djcb Thanks, I've rebased onto master and combined all the commits into one. Let me know if there's anything else you want from this!

@djcb djcb merged commit ad1a372 into djcb:master Feb 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@djcb

This comment has been minimized.

Show comment
Hide comment
@djcb

djcb Feb 5, 2017

Owner

Merged -- thanks!

Owner

djcb commented Feb 5, 2017

Merged -- thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment