Skip to content
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

[WIP] Mimetree and mime part support. Resolve #862. #1475

Closed
wants to merge 13 commits into from

Conversation

ryneeverett
Copy link
Contributor

@ryneeverett ryneeverett commented Mar 13, 2020

This is a rebase and continuation of #894.

  • rebase
  • move pipeto functionality into separate branch/PR
  • default keybindings for togglemimetree and togglemimepart
  • show current mime part

lucc and others added 13 commits March 13, 2020 01:51
The command is implemented analogous to togglesource and similar commands.
But this commit only contains the toggleing logic and no generation of actual
mime trees.
Only minimal information and no indenting is shown until now.
It is suggested in pazz#862:

> something like select to display a rendered version of the current mime part
> (either inside the tree or in a new buffer)

I played around with these options before arriving at the current
behavior. Adding the mime part content to the mimetree seemed made for a
very busy/over-nested screen and didn't seem all that useful. Likewise
opening in another buffer doesn't seem useful and might need a new
Buffer subclass in order to be well labeled.

The `select` behavior I ended up going with was to change the mime part
chosen by default in the Message itself. This should make implementing
other commands (e.g. pipeto) on the mime parts trivial.

I took `select` a step further by also having it conveniently togglemimetree
off. I can't think of any use case for remaining in the mimetree view after
making a selection.
The most notable use case is piping html to a browser without extra
scripts such as those shared in pazz#789.
When the mimetree is toggled on and a mime part is focused, pipeto
should pipe the focused mime part rather than the currently selected
part. This is only applicable to --format's that pipe a single part,
which are decoded and mimepart.
While `pipeto --format mimepart` pipes whichever part is currently
selected, the 'html' and 'plain' formats override the selected mime part
(and preferences in settings).

This is useful because the mime type you want displayed in alot isn't
necessarily the one you want piped and also for setting keybindings to
pipe specific mime types to specific applications (plain -> text editor,
html -> web browser).
Copy link
Owner

@pazz pazz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(moved into review)

Copy link
Owner

@pazz pazz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for pushing this further and saving @lucc's old branch from oblivion!

I have a few suggestions and comments we should address before merging this:

  1. There should be some default keybindings for these new commands.
  2. perhaps we can add a section or paragraph on mime trees in the usage docs?
  3. It'd be great if the there was a way to show which mime part is currently displayed.
    For example, check out how envelope buffers do this: by populating the info dict of the buffer, which is then being picked up by the statusline.
    The trouble with this is that the displayed mime part is not a property of the buffer but the message.
  4. perhaps it is time to finally introduce a single-message buffer? In any case, we should review any changes proposed here under the assumption that this will come in future, and we should try to write code in a way that miminizes later overhead.
  5. I've seen that there is a separate branch dealing with the pipeto command but there are also related commits here. Can we compartementalize and move all those into the later PR?
  6. Unit tests and Codeclimate should not fail before we merge

@ryneeverett
Copy link
Contributor Author

  1. perhaps it is time to finally introduce a single-message buffer? In any case, we should review any changes proposed here under the assumption that this will come in future, and we should try to write code in a way that miminizes later overhead.

I implemented a single message buffer in one of my earliest drafts. As I recall I didn't even have to create a custom buffer I just passed the mimepart into the base Buffer class and it worked perfectly. However I didn't find this to be a useful or pleasant UI so rather than add a new Buffer subclass to add a pretty header I switched to the mime part toggling strategy. Long story short this is fairly trivial now that the mimepart is easily accessible.

  1. Unit tests and Codeclimate should not fail before we merge

It seems like Codeclimate fails if there are any "issues" and any change to a function that codeclimate doesn't like is considered an issue. One of the failures is for adding the new commands to a list of completion variables in a method codeclimate considers too complex. Another is actually for decreasing the complexity by replacing extract_body (Cognitive Complexity 11) with extract_body_part (Cognitive Complexity 8). I doubt you really want those unrelated methods simplified in this branch.

@ryneeverett ryneeverett changed the title Mimetree and mime part support. Resolve #862. [WIP] Mimetree and mime part support. Resolve #862. Mar 22, 2020
@lucc
Copy link
Collaborator

lucc commented Mar 22, 2020

Thank you @ryneeverett for working on this. I had previously given up because I had hit a wall.

I like the idea of a single message buffer where I can see a mime tree and expand and collaps the individual mime parts just like I can currently see a message tree in a thread buffer and expand and collapse the individual messages.

@ryneeverett
Copy link
Contributor Author

I like the idea of a single message buffer where I can see a mime tree and expand and collaps the individual mime parts just like I can currently see a message tree in a thread buffer and expand and collapse the individual messages.

I personally don't see any use for this functionality but it would be pretty easy to implement on top of this branch. I've added a reference to the email body part to the mime tree leaves and I've broken out the extract_body_part function to return the displaystring from an individual body part, so the mimetree itself now has all the data needed to construct such a CollapsibleTree.

@ryneeverett
Copy link
Contributor Author

  1. perhaps we can add a section or paragraph on mime trees in the usage docs?

I'm open to suggestions but I'm not sure where I'd add an additional section beyond the automatic documentation in the "Commands in ‘thread’ mode" section page. Adding a top-level usage page on these features strikes me as excessive.

@ryneeverett
Copy link
Contributor Author

Closing in favor of #1480 and #1481.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants