-
-
Notifications
You must be signed in to change notification settings - Fork 116
feat: pre-messages / next version of download on demand #7371
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
base: main
Are you sure you want to change the base?
Conversation
6fe6723 to
8140ebc
Compare
2f1c383 to
6bcc795
Compare
8443b17 to
702771f
Compare
126c64f to
bb0fd7c
Compare
- Remove partial downloads (remove creation of the stub messages) (#7373) - Remove "Download maximum available until" and remove stock string `DC_STR_DOWNLOAD_AVAILABILITY` (#7369) - Send pre-message on messages with large attachments (#7410) - Pre messages can now get read receipts (#7433) Co-authored-by: Hocuri <[email protected]>
bb0fd7c to
a98fe05
Compare
| "CREATE TABLE download_new ( | ||
| rfc724_mid TEXT NOT NULL DEFAULT '', | ||
| msg_id INTEGER NOT NULL DEFAULT 0 | ||
| ) STRICT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@link2xt I'm wondering whether we should add UNIQUE(rfc724_mid) or UNIQUE(msg_id)? Or even both? I guess it doesn't really make sense to have the same message in there twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both msg_id and rfc724_mid in this table? rfc724_mid can be obtained from msg_id and there's even INDEX msgs_index1 ON msgs (rfc724_mid) if we need to do a reverse lookup
Previously, the uid_next wasn't advanced, which didn't create any problems, but it also was inefficient, because another loop was done trying to fetch the message again (and then finally skipping it, because it's already known).
| * - `download_limit` = Messages up to this number of bytes are downloaded automatically. | ||
| * For larger messages, only the header is downloaded and a placeholder is shown. | ||
| * For messages with large attachments, two messages are sent: | ||
| * a Pre-Message containing metadata and a Post-Message containing the attachment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Full message" was a better term, "post-message" is literally "smth going after the message". And the full message should contain metadata as well, but the documentation doesn't say that
| * Get the size of the file. Returns the size of the file associated with a | ||
| * message, if applicable. | ||
| * message, if applicable. | ||
| * If message is a pre-message, then this returns size of the to be downloaded file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * If message is a pre-message, then this returns size of the to be downloaded file. | |
| * If message is a pre-message, then this returns the size of the file to be downloaded. |
| file_mime: Option<String>, | ||
|
|
||
| /// The size of the file in bytes, if applicable. | ||
| /// If message is a pre-message, then this is the size of the to be downloaded file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// If message is a pre-message, then this is the size of the to be downloaded file. | |
| /// If message is a pre-message, then this is the size of the file to be downloaded. |
| /// Pre-Message is a small message with metadata which announces a larger Post-Message. | ||
| /// Post-Messages are not downloaded in the background. | ||
| /// | ||
| /// If pre-message is not nessesary this returns a normal message instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// If pre-message is not nessesary this returns a normal message instead. | |
| /// If pre-message is not nessesary this returns `None` as the second value. |
But i'd return the pre-message as the first value, this looks more natural to me.
| if needs_pre_message { | ||
| info!( | ||
| context, | ||
| "Message is large and will be split into a pre- and a post-message.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "Message is large and will be split into a pre- and a post-message.", | |
| "Message {} is large and will be split into a pre- and a post-message.", msg.id, |
| } | ||
|
|
||
| Ok(()) | ||
| async fn remove_from_download_table(context: &Context, rfc724_mid: &str) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| async fn remove_from_download_table(context: &Context, rfc724_mid: &str) -> Result<()> { | |
| async fn delete_download_row(context: &Context, rfc724_mid: &str) -> Result<()> { |
| // this is a dedicated method because it is used in multiple places. | ||
| pub(crate) async fn premessage_is_downloaded_for( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // this is a dedicated method because it is used in multiple places. | |
| pub(crate) async fn premessage_is_downloaded_for( | |
| pub(crate) async fn pre_message_is_downloaded_for( |
for consistent naming. The comment isn't necessary, one can easily grep and see that
| // This is probably a classical email that vanished before we could download it | ||
| warn!( | ||
| context, | ||
| "{rfc724_mid} is probably a classical email that vanished before we could download it" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe say what happened exactly? There's already a comment explaining the likely scenario. Logs are for developers mainly anyway
| Ok(()) | ||
| } | ||
|
|
||
| /// Download known post messages without pre_message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Download known post messages without pre_message | |
| /// Download known post-messages without pre-message |
| .await?; | ||
| for rfc724_mid in &rfc724_mids { | ||
| if !premessage_is_downloaded_for(context, rfc724_mid).await? { | ||
| // Download the Post-Message unconditionally, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we the same for non-deltachat messages, encrypted or unencrypted? They don't have pre-messages, but they are important as well, i.e. should be displayed in chats
| /// Renders the Message or splits it into Post-Message and Pre-Message. | ||
| /// | ||
| /// Pre-Message is a small message with metadata which announces a larger Post-Message. | ||
| /// Post-Messages are not downloaded in the background. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the pre-message is missing, shouldn't the "post-message" be downloaded in background (like usual emails)?
|
|
||
| /// Metadata contained in Pre-Message that describes the Post-Message. | ||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
| pub struct PreMsgMetadata { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is PreMsgData, it's metadata for the "post-message" (and even not metadata, but a part thereof)
| pub(crate) filename: String, | ||
| /// Dimensions: width and height of image or video | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub(crate) dimensions: Option<(i32, i32)>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be renamed to wh for more consistency with Param::{Width,Height} and to state that the width goes first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also think about using u32 instead of i32, because dimensions can't be negative, but that's probably sth. for another pr, because it would also touch other existing code.
| _ => { | ||
| warn!(context, "Message misses either width or height."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| _ => { | |
| warn!(context, "Message misses either width or height."); | |
| wh => { | |
| warn!(context, "Message {} misses width or height: {:?}.", message.id, wh); |
|
|
||
| impl Params { | ||
| /// Applies data from pre_msg_metadata to Params | ||
| pub(crate) fn apply_from_pre_msg_metadata( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| pub(crate) fn apply_from_pre_msg_metadata( | |
| pub(crate) fn apply_pre_msg_metadata( |
| .log_err(ctx) | ||
| .ok(); | ||
|
|
||
| download_msgs(ctx, &mut session) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to move this right after download_known_post_messages_without_pre_message() so that it goes before scan_folders(). Scanning folders isn't that important, it's even debounced to once per minute
| Ok(session) | ||
| } | ||
|
|
||
| /// The simplified IMAP IDLE loop to watch non primary folders (non-inbox folders) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// The simplified IMAP IDLE loop to watch non primary folders (non-inbox folders) | |
| /// The simplified IMAP loop to watch non-inbox folders. |
| JOIN msgs m ON d.msg_id = m.id | ||
| WHERE m.rfc724_mid IS NOT NULL AND m.rfc724_mid != ''; | ||
| DROP TABLE download; | ||
| ALTER TABLE download_new RENAME TO download; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming to downloads, we usually use plurals for tables
| Viewtype::File => file(context).await, | ||
| Viewtype::Webxdc => "Mini App".to_owned(), | ||
| Viewtype::Vcard => "👤".to_string(), | ||
| Viewtype::Unknown | Viewtype::Text | Viewtype::Call => self.to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment that for these an untranslated string shouldn't normally appear, otherwise may look as a lack of implementation
| @@ -1,3 +1,4 @@ | |||
| mod account_events; | |||
| mod aeap; | |||
| mod pre_messages; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, i think this is a better layout than e.g. mimeparser/mimeparser_tests.rs. Reminds me of Filesystem Hierarchy Standard which is a more familiar thing
DC_STR_DOWNLOAD_AVAILABILITY#7369This is the branch for #7367
closes #7367
Currently removed tests
Progress of the tests
Overview about the recycled(♻️) and dropped(🗑️) tests of the tests that I removed in #7373
All the recycled tests were already recycled/re-made except for some which are still to do:
Furthermore there need to be new tests to test the downloading/scheduling changes.
TODO Tests
postponed to do tests
discarded/ignored test ideas
maybe we should reconsider / discuss those?