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

add option to access original message #2125

Merged
merged 50 commits into from
Jan 11, 2021
Merged

add option to access original message #2125

merged 50 commits into from
Jan 11, 2021

Conversation

r10s
Copy link
Member

@r10s r10s commented Dec 31, 2020

while dealing with email-compatibility (https://github.com/orgs/deltachat/projects/31 and also on one-to-one-talks), it turns out that we need an option to read the original message more urgently as i thought before.

  • while this is clear for html-only-mails, unfortunately, also plain-text-alternatives as offered eg. by some newletters are often very poor, having huge links etc.
  • even if we try harder to detect quotes, convert things etc. we will never 100%, there may always be some context loss

therefore, the idea is to have a function to check if a message is "maybe bad" (dc_msg_is_mime_modified() dc_msg_has_html()) and another function to get the message with "rich content", always as html (dc_get_original_mime_html() dc_get_msg_html()). the ui shall offer a button as "Show full message..." or so (there is already a pr for that on android). as the html-part is not shown and not even processed before the function is called, the fear or tracking seems to be already mitigated enough, however, for a first try, i do not want to make that too complicated - also, many html-mails are not really working without reading external content and you will not open "random" mails, but, as of now, only from senders you already started a chat with.

having the option to access the "original" message, finallly also allows us to do handle the generated "short" mail more freely - eg. we can truncate long messages, and stop display before a links spanning 20 lines etc.

ui-wise, this is pretty easy to implement, and already done for android (deltachat/deltachat-android#1763 - there are also some images). desktop already has a similar hack, detecting such messages by scanning for "[...]" and offering a link to the info screen; this should be replaced by the api mentioned above.

  • save original mime-structure
  • set is_mime_modified for html messages
  • set is_mime_modified for plain-text messages if simplify cuts things (this is when [...] gets added to the text)
  • add ffi functions to get original as html
  • return plain text as html
  • handle plain text in different character sets
  • make plain text links clickable
  • plain text: handle quotes and format=flowed
  • check if sanitize html, filter out js is needed and/or add a clear hint to the docs, cmp forbid active content when displaying html-messages #2127
  • handle embedded images
  • do not set mime-modified for encrypted messages

open things:

  • handle encrypted messages (can also go to a subsequent pr)

@r10s r10s force-pushed the save-original branch 2 times, most recently from 7944936 to ce925dd Compare January 1, 2021 16:09
@r10s r10s marked this pull request as draft January 1, 2021 16:24
@r10s r10s force-pushed the save-original branch 5 times, most recently from 10123cb to a8fb946 Compare January 2, 2021 13:14
@r10s r10s force-pushed the save-original branch 5 times, most recently from 114f0d5 to 89c644f Compare January 5, 2021 21:55
@r10s r10s force-pushed the save-original branch 4 times, most recently from a62198d to 05d21f2 Compare January 9, 2021 10:26
@@ -854,7 +867,9 @@ async fn add_parts(
} else {
match ephemeral_timer {
EphemeralTimer::Disabled => 0,
EphemeralTimer::Enabled { duration } => rcvd_timestamp + i64::from(duration)
EphemeralTimer::Enabled { duration } => {
Copy link
Member Author

Choose a reason for hiding this comment

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

this was rewritten by cargo fmt, not sure why. it is not related to this pr.

mime_headers.clone()
} else {
None
},
Copy link
Member Author

Choose a reason for hiding this comment

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

we already had a field that was used for exactly the purpose of saving the raw mime message, although the name mime_headers is a bit misleading - i meanwhile read it as "mime and headers" :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

mime_headers is misleading indeed. Is save_mime_headers used for anything but debugging? Can it be removed?

Copy link
Member Author

@r10s r10s Jan 11, 2021

Choose a reason for hiding this comment

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

save_mime_headers was introduced at deltachat/deltachat-core#388, not sure if it is actually in used, however, it is not much code, also, with this pr not even a wasted column anymore - so i would not remove it in this pr without further discussion.

pub fn simplify(mut input: String, is_chat_message: bool) -> (String, bool, Option<String>) {
pub fn simplify(mut input: String, is_chat_message: bool) -> (String, bool, bool, Option<String>) {
let mut is_cut = false;

Copy link
Member Author

Choose a reason for hiding this comment

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

yip, i also thought that four return values is a value to think over doing that in another way, however, i did not want to make this pr even larget - also, beside tests, simplify() us called only at one place, so i'd leave it as is for now :)

src/originalhtml.rs Outdated Show resolved Hide resolved
} else if mimetype == mime::TEXT_PLAIN {
if let Ok(decoded_data) = mail.get_body() {
self.plain = Some(decoded_data);
self.format_flowed =
Copy link
Member Author

@r10s r10s Jan 9, 2021

Choose a reason for hiding this comment

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

these parameter-getting lines are similar to the ones from mimeparser.rs, maybe we can move them to format_flowed.rs, however, as not much more code from format_flowed.rs is reusable (format_flowed.rs converts to plain text ignoring quotes where we want to convert to html and regard quotes), i think, it is not worth the noise.

@r10s r10s marked this pull request as ready for review January 9, 2021 14:12
@Simon-Laux
Copy link
Member

Simon-Laux commented Jan 9, 2021

I'm not sure if I agree calling it "dc_get_original_mime_html()" when It's clearly returning a modified version to work in browsers / converts text emails to html. Maybe "dc_get_original_mime_as_html()" would be a bit better?
Also the wording of the documentation could emphasize a bit more that the returned html is not to be trusted.
"NEVER TRUST USER INPUT"

Anyways besides that minor naming and documentation wording stuff:
The PR looks good to me (reading code on github, haven't tested it yet.)

@r10s
Copy link
Member Author

r10s commented Jan 10, 2021

@Simon-Laux @link2xt i targeted most of your comments, thank you very much for that.

moreover, i added a commit that explicitly avoid setting mime-modified- aka has-html-flag for encrypted messages.

i'd like to handle/discuss that in a subsequent pr, there is probably no huge intersection between html-mails and encrypted-mails anyway.

deltachat-ffi/deltachat.h Show resolved Hide resolved
deltachat-ffi/deltachat.h Outdated Show resolved Hide resolved
src/html.rs Outdated Show resolved Hide resolved
deltachat-ffi/src/lib.rs Outdated Show resolved Hide resolved
src/dc_receive_imf.rs Outdated Show resolved Hide resolved
src/html.rs Outdated Show resolved Hide resolved
src/html.rs Outdated Show resolved Hide resolved
src/mimeparser.rs Outdated Show resolved Hide resolved
};

self.is_mime_modified = self.is_mime_modified
|| ((is_forwarded || is_cut || top_quote.is_some())
Copy link
Collaborator

Choose a reason for hiding this comment

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

top_quote.is_some() is not necessary. We display the top quote just fine, no reason to show "read more" for each unencrypted message with a quote. If we have removed User ... wrote: top line, then is_cut should be set. remove_top_quote has an is_quoted_headline variable internally which tracks this.

Copy link
Member Author

Choose a reason for hiding this comment

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

i also thought over that bit.

we do that only for non-delta-messages anyway - and then, there is the risk that the quote is not really a quoted messages but sth. the user entered/edited manually, so a tap on the quote cannot go to the original message and there is no chance the see the full quote (in the message, only the first lines are shown)

this risk is potentially also there also for delta-to-delta communication, eg. if the message was deleted, however, it is much larger when the user can edit the quote manually.

so, i'd leave that in for now, it is better to show one "full message" button too much than one too few :)

src/simplify.rs Show resolved Hide resolved
let lines = split_lines(&self.text);

let mut ret =
"<!DOCTYPE html>\n<html><head><meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\" /></head><body>\n".to_string();
Copy link
Collaborator

Choose a reason for hiding this comment

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

html, head, meta and body tags are all not necessary for valid HTML5 (identified by <!DOCTYPE html>), could be removed.

At least, is meta necessary here?

Copy link
Member Author

@r10s r10s Jan 11, 2021

Choose a reason for hiding this comment

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

without the meta-tag, some browsers do display utf-8 wrong, so yes, that is needed.

as <meta> needs to be placed in <head> i would also leave them in, it is just a few characters and i am not sure, if all browsers will handle things correctly when we rely on html5 here.

@r10s r10s merged commit e2688f6 into master Jan 11, 2021
@r10s r10s deleted the save-original branch January 11, 2021 16:40
@r10s r10s mentioned this pull request Jan 19, 2021
4 tasks
r10s added a commit that referenced this pull request Mar 12, 2023
standard footers meanwhile go the "contact status",
so they are no longer a reason to trigger "full message view".

this was already discussed when the HTML view was introduced at #2125
however, forgotten to change when the "contact status" was added at #2218

this pr will result in a cleaner chat view
with less "Show Full Message..." buttons
and will also save some storage
(in fact, i came over that when reviewing #4129 )
r10s added a commit that referenced this pull request Mar 12, 2023
standard footers meanwhile go the "contact status",
so they are no longer a reason to trigger "full message view".

this was already discussed when the HTML view was introduced at #2125
however, forgotten to change when the "contact status" was added at #2218

this pr will result in a cleaner chat view
with less "Show Full Message..." buttons
and will also save some storage
(in fact, i came over that when reviewing #4129 )
r10s added a commit that referenced this pull request Mar 13, 2023
standard footers meanwhile go the "contact status",
so they are no longer a reason to trigger "full message view".

this was already discussed when the HTML view was introduced at #2125
however, forgotten to change when the "contact status" was added at #2218

this pr will result in a cleaner chat view
with less "Show Full Message..." buttons
and will also save some storage
(in fact, i came over that when reviewing #4129 )
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