From 18445c09c2c72aab9e84d59eb7ffc79f0005cdef Mon Sep 17 00:00:00 2001 From: iequidoo Date: Mon, 8 Sep 2025 15:23:32 -0300 Subject: [PATCH] feat: Remove Config::SentboxWatch (#7178) The motivation is to reduce code complexity, get rid of the extra IMAP connection and cases when messages are added to chats by Inbox and Sentbox loops in parallel which leads to various message sorting bugs, particularly to outgoing messages breaking sorting of incoming ones which are fetched later, but may have a smaller "Date". --- python/src/deltachat/testplugin.py | 1 - python/tests/test_1_online.py | 14 +++++++------ scripts/update-provider-database.sh | 2 +- src/config.rs | 19 +---------------- src/configure.rs | 1 - src/context.rs | 8 -------- src/imap.rs | 6 +----- src/imap/imap_tests.rs | 8 -------- src/imap/scan_folders.rs | 3 --- src/provider/data.rs | 21 +++++-------------- src/scheduler.rs | 32 +++++++++++++---------------- src/scheduler/connectivity.rs | 3 +-- src/tests/verified_chats.rs | 6 +++--- 13 files changed, 34 insertions(+), 90 deletions(-) diff --git a/python/src/deltachat/testplugin.py b/python/src/deltachat/testplugin.py index 77a2195f0..e658789ef 100644 --- a/python/src/deltachat/testplugin.py +++ b/python/src/deltachat/testplugin.py @@ -523,7 +523,6 @@ class ACFactory: assert "addr" in configdict and "mail_pw" in configdict, configdict configdict.setdefault("bcc_self", False) configdict.setdefault("mvbox_move", False) - configdict.setdefault("sentbox_watch", False) configdict.setdefault("sync_msgs", False) configdict.setdefault("delete_server_after", 0) ac.update_config(configdict) diff --git a/python/tests/test_1_online.py b/python/tests/test_1_online.py index a52e3bfa2..e0be56cc1 100644 --- a/python/tests/test_1_online.py +++ b/python/tests/test_1_online.py @@ -269,19 +269,21 @@ def test_enable_mvbox_move(acfactory, lp): assert ac2._evtracker.wait_next_incoming_message().text == "message1" -def test_mvbox_sentbox_threads(acfactory, lp): +def test_mvbox_thread_and_sentbox(acfactory, lp): lp.sec("ac1: start with mvbox thread") - ac1 = acfactory.new_online_configuring_account(mvbox_move=True, sentbox_watch=False) + ac1 = acfactory.new_online_configuring_account(mvbox_move=True) - lp.sec("ac2: start without mvbox/sentbox threads") - ac2 = acfactory.new_online_configuring_account(mvbox_move=False, sentbox_watch=False) + lp.sec("ac2: start without a mvbox thread") + ac2 = acfactory.new_online_configuring_account(mvbox_move=False) lp.sec("ac2 and ac1: waiting for configuration") acfactory.bring_accounts_online() - lp.sec("ac1: create and configure sentbox") + lp.sec("ac1: create sentbox") ac1.direct_imap.create_folder("Sent") - ac1.set_config("sentbox_watch", "1") + ac1.set_config("scan_all_folders_debounce_secs", "0") + ac1.stop_io() + ac1.start_io() lp.sec("ac1: send message and wait for ac2 to receive it") acfactory.get_accepted_chat(ac1, ac2).send_text("message1") diff --git a/scripts/update-provider-database.sh b/scripts/update-provider-database.sh index eaed2d97f..4ad079fe8 100755 --- a/scripts/update-provider-database.sh +++ b/scripts/update-provider-database.sh @@ -6,7 +6,7 @@ set -euo pipefail export TZ=UTC # Provider database revision. -REV=1cce91c1f1065b47e4f307d6fe2f4cca68c74d2e +REV=d041136c19a48b493823b46d472f12b9ee94ae80 CORE_ROOT="$PWD" TMP="$(mktemp -d)" diff --git a/src/config.rs b/src/config.rs index 9f071c85d..37429b7ed 100644 --- a/src/config.rs +++ b/src/config.rs @@ -156,10 +156,6 @@ pub enum Config { #[strum(props(default = "1"))] MdnsEnabled, - /// True if "Sent" folder should be watched for changes. - #[strum(props(default = "0"))] - SentboxWatch, - /// True if chat messages should be moved to a separate folder. Auto-sent messages like sync /// ones are moved there anyway. #[strum(props(default = "1"))] @@ -473,10 +469,7 @@ impl Config { /// Whether the config option needs an IO scheduler restart to take effect. pub(crate) fn needs_io_restart(&self) -> bool { - matches!( - self, - Config::MvboxMove | Config::OnlyFetchMvbox | Config::SentboxWatch - ) + matches!(self, Config::MvboxMove | Config::OnlyFetchMvbox) } } @@ -603,15 +596,6 @@ impl Context { || !self.get_config_bool(Config::IsChatmail).await?) } - /// Returns true if sentbox ("Sent" folder) should be watched. - pub(crate) async fn should_watch_sentbox(&self) -> Result { - Ok(self.get_config_bool(Config::SentboxWatch).await? - && self - .get_config(Config::ConfiguredSentboxFolder) - .await? - .is_some()) - } - /// Returns true if sync messages should be sent. pub(crate) async fn should_send_sync_msgs(&self) -> Result { Ok(self.get_config_bool(Config::SyncMsgs).await? @@ -700,7 +684,6 @@ impl Context { | Config::ProxyEnabled | Config::BccSelf | Config::MdnsEnabled - | Config::SentboxWatch | Config::MvboxMove | Config::OnlyFetchMvbox | Config::DeleteToTrash diff --git a/src/configure.rs b/src/configure.rs index c048a7c56..fafe5bfa8 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -549,7 +549,6 @@ async fn configure(ctx: &Context, param: &EnteredLoginParam) -> Result ctx.get_config_bool(Config::IsChatmail).await?, }; if is_chatmail { - ctx.set_config(Config::SentboxWatch, None).await?; ctx.set_config(Config::MvboxMove, Some("0")).await?; ctx.set_config(Config::OnlyFetchMvbox, None).await?; ctx.set_config(Config::ShowEmails, None).await?; diff --git a/src/context.rs b/src/context.rs index 7e2bf9417..3716909db 100644 --- a/src/context.rs +++ b/src/context.rs @@ -845,7 +845,6 @@ impl Context { Err(err) => format!(""), }; - let sentbox_watch = self.get_config_int(Config::SentboxWatch).await?; let mvbox_move = self.get_config_int(Config::MvboxMove).await?; let only_fetch_mvbox = self.get_config_int(Config::OnlyFetchMvbox).await?; let folders_configured = self @@ -949,7 +948,6 @@ impl Context { .await? .to_string(), ); - res.insert("sentbox_watch", sentbox_watch.to_string()); res.insert("mvbox_move", mvbox_move.to_string()); res.insert("only_fetch_mvbox", only_fetch_mvbox.to_string()); res.insert( @@ -1259,12 +1257,6 @@ impl Context { Ok(inbox.as_deref() == Some(folder_name)) } - /// Returns true if given folder name is the name of the "sent" folder. - pub async fn is_sentbox(&self, folder_name: &str) -> Result { - let sentbox = self.get_config(Config::ConfiguredSentboxFolder).await?; - Ok(sentbox.as_deref() == Some(folder_name)) - } - /// Returns true if given folder name is the name of the "DeltaChat" folder. pub async fn is_mvbox(&self, folder_name: &str) -> Result { let mvbox = self.get_config(Config::ConfiguredMvboxFolder).await?; diff --git a/src/imap.rs b/src/imap.rs index 91cdcf199..2c0be0780 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -2044,7 +2044,7 @@ async fn spam_target_folder_cfg( if needs_move_to_mvbox(context, headers).await? // If OnlyFetchMvbox is set, we don't want to move the message to - // the inbox or sentbox where we wouldn't fetch it again: + // the inbox where we wouldn't fetch it again: || context.get_config_bool(Config::OnlyFetchMvbox).await? { Ok(Some(Config::ConfiguredMvboxFolder)) @@ -2568,10 +2568,6 @@ async fn should_ignore_folder( if !context.get_config_bool(Config::OnlyFetchMvbox).await? { return Ok(false); } - if context.is_sentbox(folder).await? { - // Still respect the SentboxWatch setting. - return Ok(!context.get_config_bool(Config::SentboxWatch).await?); - } Ok(!(context.is_mvbox(folder).await? || folder_meaning == FolderMeaning::Spam)) } diff --git a/src/imap/imap_tests.rs b/src/imap/imap_tests.rs index 1aa746ea9..421f7e148 100644 --- a/src/imap/imap_tests.rs +++ b/src/imap/imap_tests.rs @@ -183,10 +183,6 @@ const COMBINATIONS_ACCEPTED_CHAT: &[(&str, bool, bool, &str)] = &[ ("INBOX", false, true, "INBOX"), ("INBOX", true, false, "INBOX"), ("INBOX", true, true, "DeltaChat"), - ("Sent", false, false, "Sent"), - ("Sent", false, true, "Sent"), - ("Sent", true, false, "Sent"), - ("Sent", true, true, "Sent"), ("Spam", false, false, "INBOX"), // Move classical emails in accepted chats from Spam to Inbox, not 100% sure on this, we could also just never move non-chat-msgs ("Spam", false, true, "INBOX"), ("Spam", true, false, "INBOX"), // Move classical emails in accepted chats from Spam to Inbox, not 100% sure on this, we could also just never move non-chat-msgs @@ -199,10 +195,6 @@ const COMBINATIONS_REQUEST: &[(&str, bool, bool, &str)] = &[ ("INBOX", false, true, "INBOX"), ("INBOX", true, false, "INBOX"), ("INBOX", true, true, "DeltaChat"), - ("Sent", false, false, "Sent"), - ("Sent", false, true, "Sent"), - ("Sent", true, false, "Sent"), - ("Sent", true, true, "Sent"), ("Spam", false, false, "Spam"), ("Spam", false, true, "INBOX"), ("Spam", true, false, "Spam"), diff --git a/src/imap/scan_folders.rs b/src/imap/scan_folders.rs index ed4b6f215..e1a6592d4 100644 --- a/src/imap/scan_folders.rs +++ b/src/imap/scan_folders.rs @@ -108,9 +108,6 @@ impl Imap { pub(crate) async fn get_watched_folder_configs(context: &Context) -> Result> { let mut res = vec![Config::ConfiguredInboxFolder]; - if context.get_config_bool(Config::SentboxWatch).await? { - res.push(Config::ConfiguredSentboxFolder); - } if context.should_watch_mvbox().await? { res.push(Config::ConfiguredMvboxFolder); } diff --git a/src/provider/data.rs b/src/provider/data.rs index 1e328b9ff..bfd359f32 100644 --- a/src/provider/data.rs +++ b/src/provider/data.rs @@ -510,10 +510,6 @@ static P_FIVE_CHAT: Provider = Provider { key: Config::BccSelf, value: "1", }, - ConfigDefault { - key: Config::SentboxWatch, - value: "0", - }, ConfigDefault { key: Config::MvboxMove, value: "0", @@ -1084,10 +1080,6 @@ static P_NAUTA_CU: Provider = Provider { key: Config::DeleteServerAfter, value: "1", }, - ConfigDefault { - key: Config::SentboxWatch, - value: "0", - }, ConfigDefault { key: Config::MvboxMove, value: "0", @@ -1629,10 +1621,6 @@ static P_TESTRUN: Provider = Provider { key: Config::BccSelf, value: "1", }, - ConfigDefault { - key: Config::SentboxWatch, - value: "0", - }, ConfigDefault { key: Config::MvboxMove, value: "0", @@ -1898,11 +1886,11 @@ static P_WKPB_DE: Provider = Provider { oauth2_authorizer: None, }; -// yahoo.md: yahoo.com, yahoo.de, yahoo.it, yahoo.fr, yahoo.es, yahoo.se, yahoo.co.uk, yahoo.co.nz, yahoo.com.au, yahoo.com.ar, yahoo.com.br, yahoo.com.mx, ymail.com, rocketmail.com, yahoodns.net +// yahoo.md: yahoo.com, yahoo.de, yahoo.it, yahoo.fr, yahoo.es, yahoo.se, yahoo.co.uk, yahoo.co.nz, yahoo.com.au, yahoo.com.ar, yahoo.com.br, yahoo.com.mx, myyahoo.com, ymail.com, rocketmail.com, yahoodns.net static P_YAHOO: Provider = Provider { id: "yahoo", status: Status::Preparation, - before_login_hint: "To use your Yahoo email address you have to create an \"App-Password\" in the account security screen.", + before_login_hint: "To use your Yahoo email address you have to create an app password in the Yahoo account security screen.", after_login_hint: "", overview_page: "https://providers.delta.chat/yahoo", server: &[ @@ -2041,7 +2029,7 @@ static P_ZOHO: Provider = Provider { oauth2_authorizer: None, }; -pub(crate) static PROVIDER_DATA: [(&str, &Provider); 533] = [ +pub(crate) static PROVIDER_DATA: [(&str, &Provider); 534] = [ ("163.com", &P_163), ("aktivix.org", &P_AKTIVIX_ORG), ("aliyun.com", &P_ALIYUN), @@ -2560,6 +2548,7 @@ pub(crate) static PROVIDER_DATA: [(&str, &Provider); 533] = [ ("yahoo.com.ar", &P_YAHOO), ("yahoo.com.br", &P_YAHOO), ("yahoo.com.mx", &P_YAHOO), + ("myyahoo.com", &P_YAHOO), ("ymail.com", &P_YAHOO), ("rocketmail.com", &P_YAHOO), ("yahoodns.net", &P_YAHOO), @@ -2658,4 +2647,4 @@ pub(crate) static PROVIDER_IDS: LazyLock = - LazyLock::new(|| chrono::NaiveDate::from_ymd_opt(2025, 9, 4).unwrap()); + LazyLock::new(|| chrono::NaiveDate::from_ymd_opt(2025, 9, 10).unwrap()); diff --git a/src/scheduler.rs b/src/scheduler.rs index a778e4fe7..2a3537daf 100644 --- a/src/scheduler.rs +++ b/src/scheduler.rs @@ -254,7 +254,7 @@ impl SchedulerState { } } - /// Interrupt optional boxes (mvbox, sentbox) loops. + /// Interrupt optional boxes (mvbox currently) loops. pub(crate) async fn interrupt_oboxes(&self) { let inner = self.inner.read().await; if let InnerSchedulerState::Started(ref scheduler) = *inner { @@ -333,7 +333,7 @@ struct SchedBox { #[derive(Debug)] pub(crate) struct Scheduler { inbox: SchedBox, - /// Optional boxes -- mvbox, sentbox. + /// Optional boxes -- mvbox. oboxes: Vec, smtp: SmtpConnectionState, smtp_handle: task::JoinHandle<()>, @@ -875,22 +875,18 @@ impl Scheduler { }; start_recvs.push(inbox_start_recv); - for (meaning, should_watch) in [ - (FolderMeaning::Mvbox, ctx.should_watch_mvbox().await), - (FolderMeaning::Sent, ctx.should_watch_sentbox().await), - ] { - if should_watch? { - let (conn_state, handlers) = ImapConnectionState::new(ctx).await?; - let (start_send, start_recv) = oneshot::channel(); - let ctx = ctx.clone(); - let handle = task::spawn(simple_imap_loop(ctx, start_send, handlers, meaning)); - oboxes.push(SchedBox { - meaning, - conn_state, - handle, - }); - start_recvs.push(start_recv); - } + if ctx.should_watch_mvbox().await? { + let (conn_state, handlers) = ImapConnectionState::new(ctx).await?; + let (start_send, start_recv) = oneshot::channel(); + let ctx = ctx.clone(); + let meaning = FolderMeaning::Mvbox; + let handle = task::spawn(simple_imap_loop(ctx, start_send, handlers, meaning)); + oboxes.push(SchedBox { + meaning, + conn_state, + handle, + }); + start_recvs.push(start_recv); } let smtp_handle = { diff --git a/src/scheduler/connectivity.rs b/src/scheduler/connectivity.rs index 64dfccc7a..1e2ede5bf 100644 --- a/src/scheduler/connectivity.rs +++ b/src/scheduler/connectivity.rs @@ -90,7 +90,7 @@ impl DetailedConnectivity { DetailedConnectivity::Preparing => Some(Connectivity::Working), // Just don't return a connectivity, probably the folder is configured not to be - // watched or there is e.g. no "Sent" folder, so we are not interested in it + // watched, so we are not interested in it. DetailedConnectivity::NotConfigured => None, DetailedConnectivity::Idle => Some(Connectivity::Connected), @@ -378,7 +378,6 @@ impl Context { // Add e.g. // Incoming messages // - "Inbox": Connected - // - "Sent": Connected // ============================================================================================= let watched_folders = get_watched_folder_configs(self).await?; diff --git a/src/tests/verified_chats.rs b/src/tests/verified_chats.rs index c120d73ba..41807fff0 100644 --- a/src/tests/verified_chats.rs +++ b/src/tests/verified_chats.rs @@ -207,8 +207,8 @@ async fn test_degrade_verified_oneonone_chat() -> Result<()> { Ok(()) } -/// Alice is offline for some time. -/// When she comes online, first her inbox is synced and then her sentbox. +/// Alice is offline for some time. mvbox_move is 0. +/// When she comes online, first her inbox is synced and then her mvbox. /// This test tests that the messages are still in the right order. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_old_message_4() -> Result<()> { @@ -246,7 +246,7 @@ async fn test_old_message_4() -> Result<()> { } /// Alice is offline for some time. -/// When they come online, first their sentbox is synced and then their inbox. +/// When they come online, first their mvbox is synced and then their inbox. /// This test tests that the messages are still in the right order. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_old_message_5() -> Result<()> {