diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index 5b8caa07b..627af2c0f 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -3905,7 +3905,7 @@ int64_t dc_lot_get_timestamp (const dc_lot_t* lot); #define DC_EVENT_SMTP_MESSAGE_SENT 103 /** - * Emitted when a message was successfully marked as deleted on the SMTP server. + * Emitted when a message was successfully marked as deleted on the IMAP server. * * @param data1 0 * @param data2 (const char*) Info string in english language. @@ -3914,6 +3914,15 @@ int64_t dc_lot_get_timestamp (const dc_lot_t* lot); */ #define DC_EVENT_IMAP_MESSAGE_DELETED 104 +/** + * Emitted when a message was successfully moved on IMAP. + * + * @param data1 0 + * @param data2 (const char*) Info string in english language. + * Must not be free()'d or modified and is valid only until the callback returns. + * @return 0 + */ +#define DC_EVENT_IMAP_MESSAGE_MOVED 105 /** * The library-user should write a warning string to the log. diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index e1726159f..265f634c6 100644 --- a/deltachat-ffi/src/lib.rs +++ b/deltachat-ffi/src/lib.rs @@ -126,6 +126,7 @@ impl ContextWrapper { | Event::ImapConnected(msg) | Event::SmtpMessageSent(msg) | Event::ImapMessageDeleted(msg) + | Event::ImapMessageMoved(msg) | Event::Warning(msg) | Event::Error(msg) | Event::ErrorNetwork(msg) diff --git a/python/src/deltachat/const.py b/python/src/deltachat/const.py index 96060c5af..09821bda8 100644 --- a/python/src/deltachat/const.py +++ b/python/src/deltachat/const.py @@ -62,6 +62,7 @@ DC_EVENT_SMTP_CONNECTED = 101 DC_EVENT_IMAP_CONNECTED = 102 DC_EVENT_SMTP_MESSAGE_SENT = 103 DC_EVENT_IMAP_MESSAGE_DELETED = 104 +DC_EVENT_IMAP_MESSAGE_MOVED = 105 DC_EVENT_WARNING = 300 DC_EVENT_ERROR = 400 DC_EVENT_ERROR_NETWORK = 401 diff --git a/python/tests/test_account.py b/python/tests/test_account.py index 3e548b578..aa48397ec 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -394,6 +394,17 @@ class TestOnlineAccount: ev = ac2._evlogger.get_matching("DC_EVENT_INCOMING_MSG|DC_EVENT_MSGS_CHANGED") assert ev[2] > const.DC_CHAT_ID_LAST_SPECIAL + def test_move_works(self, acfactory): + ac1 = acfactory.get_online_configuring_account() + ac2 = acfactory.get_online_configuring_account(mvbox=True) + wait_configuration_progress(ac2, 1000) + wait_configuration_progress(ac1, 1000) + chat = self.get_chat(ac1, ac2) + chat.send_text("message1") + ev = ac2._evlogger.get_matching("DC_EVENT_INCOMING_MSG|DC_EVENT_MSGS_CHANGED") + assert ev[2] > const.DC_CHAT_ID_LAST_SPECIAL + ev = ac2._evlogger.get_matching("DC_EVENT_IMAP_MESSAGE_MOVED") + def test_forward_messages(self, acfactory): ac1, ac2 = acfactory.get_two_online_accounts() chat = self.get_chat(ac1, ac2) diff --git a/src/events.rs b/src/events.rs index 19c43c37a..8ef45aba9 100644 --- a/src/events.rs +++ b/src/events.rs @@ -48,6 +48,12 @@ pub enum Event { #[strum(props(id = "104"))] ImapMessageDeleted(String), + /// Emitted when an IMAP message has been moved + /// + /// @return 0 + #[strum(props(id = "105"))] + ImapMessageMoved(String), + /// The library-user should write a warning string to the log. /// Passed to the callback given to dc_context_new(). /// diff --git a/src/imap.rs b/src/imap.rs index 8207c34da..9380421bd 100644 --- a/src/imap.rs +++ b/src/imap.rs @@ -10,7 +10,7 @@ use crate::context::Context; use crate::dc_receive_imf::dc_receive_imf; use crate::error::Error; use crate::events::Event; -use crate::job::{job_add, Action}; +use crate::job::{connect_to_inbox, job_add, Action}; use crate::login_param::LoginParam; use crate::message::{self, update_msg_move_state, update_server_uid}; use crate::oauth2::dc_get_oauth2_access_token; @@ -930,6 +930,7 @@ impl Imap { } else { let msg = &msgs[0]; + // XXX put flags into a set and pass them to dc_receive_imf let is_deleted = msg .flags() .iter() @@ -1116,107 +1117,76 @@ impl Imap { cvar.notify_one(); } - pub fn mv, S2: AsRef>( + pub fn mv( &self, context: &Context, - folder: S1, + folder: &str, uid: u32, - dest_folder: S2, + dest_folder: &str, dest_uid: &mut u32, ) -> ImapResult { - let mut res = ImapResult::RetryLater; + if folder == dest_folder { + info!( + context, + "Skip moving message; message {}/{} is already in {}...", folder, uid, dest_folder, + ); + return ImapResult::AlreadyDone; + } + if let Some(imapresult) = self.prepare_imap_operation_on_msg(context, folder, uid) { + return imapresult; + } + // we are connected, and the folder is selected + + // XXX Rust-Imap provides no target uid on mv, so just set it to 0 + *dest_uid = 0; + let set = format!("{}", uid); - - if uid == 0 { - res = ImapResult::Failed; - } else if folder.as_ref() == dest_folder.as_ref() { - info!( - context, - "Skip moving message; message {}/{} is already in {}...", - folder.as_ref(), - uid, - dest_folder.as_ref() - ); - - res = ImapResult::AlreadyDone; - } else { - info!( - context, - "Moving message {}/{} to {}...", - folder.as_ref(), - uid, - dest_folder.as_ref() - ); - - if self.select_folder(context, Some(folder.as_ref())) == 0 { - warn!( - context, - "Cannot select folder {} for moving message.", - folder.as_ref() - ); - } else { - let moved = if let Some(ref mut session) = &mut *self.session.lock().unwrap() { - match session.uid_mv(&set, &dest_folder) { - Ok(_) => { - res = ImapResult::Success; - true - } - Err(err) => { - info!( - context, - "Cannot move message, fallback to COPY/DELETE {}/{} to {}: {}", - folder.as_ref(), - uid, - dest_folder.as_ref(), - err - ); - - false - } - } - } else { - unreachable!(); - }; - - if !moved { - let copied = if let Some(ref mut session) = &mut *self.session.lock().unwrap() { - match session.uid_copy(&set, &dest_folder) { - Ok(_) => true, - Err(err) => { - eprintln!("error copy: {:?}", err); - info!(context, "Cannot copy message.",); - - false - } - } - } else { - unreachable!(); - }; - - if copied { - if !self.add_flag_finalized(context, uid, "\\Deleted") { - warn!(context, "Giving up: cannot mark {} as \"Deleted\".", uid); - } - self.config.write().unwrap().selected_folder_needs_expunge = true; - res = ImapResult::Success; - } + let display_folder_id = format!("{}/{}", folder, uid); + if let Some(ref mut session) = &mut *self.session.lock().unwrap() { + match session.uid_mv(&set, &dest_folder) { + Ok(_) => { + emit_event!( + context, + Event::ImapMessageMoved(format!( + "IMAP Message {} moved to {}", + display_folder_id, dest_folder + )) + ); + return ImapResult::Success; + } + Err(err) => { + warn!( + context, + "Cannot move message, fallback to COPY/DELETE {}/{} to {}: {}", + folder, + uid, + dest_folder, + err + ); } } - } + } else { + unreachable!(); + }; - if res == ImapResult::Success { - // TODO: is this correct? - *dest_uid = uid; - } - - if res == ImapResult::RetryLater { - if self.should_reconnect() { - ImapResult::RetryLater - } else { - ImapResult::Failed + if let Some(ref mut session) = &mut *self.session.lock().unwrap() { + match session.uid_copy(&set, &dest_folder) { + Ok(_) => { + if !self.add_flag_finalized(context, uid, "\\Deleted") { + warn!(context, "Cannot mark {} as \"Deleted\" after copy.", uid); + ImapResult::Failed + } else { + self.config.write().unwrap().selected_folder_needs_expunge = true; + ImapResult::Success + } + } + Err(err) => { + warn!(context, "Could not copy message: {}", err); + ImapResult::Failed + } } } else { - res + unreachable!(); } } @@ -1257,10 +1227,14 @@ impl Imap { uid: u32, ) -> Option { if uid == 0 { - Some(ImapResult::Failed) + return Some(ImapResult::Failed); } else if !self.is_connected() { - Some(ImapResult::RetryLater) - } else if self.select_folder(context, Some(&folder)) == 0 { + connect_to_inbox(context, &self); + if !self.is_connected() { + return Some(ImapResult::RetryLater); + } + } + if self.select_folder(context, Some(&folder)) == 0 { warn!( context, "Cannot select folder {} for preparing IMAP operation", folder @@ -1290,96 +1264,76 @@ impl Imap { } // only returns 0 on connection problems; we should try later again in this case * - pub fn delete_msg, S2: AsRef>( + pub fn delete_msg( &self, context: &Context, - message_id: S1, - folder: S2, - server_uid: &mut u32, - ) -> usize { - let mut success = false; - if *server_uid == 0 { - success = true - } else { - info!( - context, - "Marking message \"{}\", {}/{} for deletion...", - message_id.as_ref(), - folder.as_ref(), - server_uid, - ); + message_id: &str, + folder: &str, + uid: &mut u32, + ) -> ImapResult { + if let Some(imapresult) = self.prepare_imap_operation_on_msg(context, folder, *uid) { + return imapresult; + } + // we are connected, and the folder is selected - if self.select_folder(context, Some(&folder)) == 0 { - warn!( - context, - "Cannot select folder {} for deleting message.", - folder.as_ref() - ); - } else { - let set = format!("{}", server_uid); - let display_imap_id = format!("{}/{}", folder.as_ref(), server_uid); + let set = format!("{}", uid); + let display_imap_id = format!("{}/{}", folder, uid); - if let Some(ref mut session) = &mut *self.session.lock().unwrap() { - match session.uid_fetch(set, PREFETCH_FLAGS) { - Ok(msgs) => { - if msgs.is_empty() { - warn!( - context, - "Cannot delete on IMAP, {}: message-id gone '{}'", - display_imap_id, - message_id.as_ref(), - ); - } else { - let remote_message_id = - prefetch_get_message_id(msgs.first().unwrap()) - .unwrap_or_default(); - - if remote_message_id != message_id.as_ref() { - warn!( - context, - "Cannot delete on IMAP, {}: remote message-id '{}' != '{}'", - display_imap_id, - remote_message_id, - message_id.as_ref(), - ); - } - } - *server_uid = 0; - } - Err(err) => { - eprintln!("fetch error: {:?}", err); - - warn!( - context, - "Cannot delete on IMAP, {} not found.", - display_imap_id, - ); - *server_uid = 0; - } + // double-check that we are deleting the correct message-id + // this comes at the expense of another imap query + if let Some(ref mut session) = &mut *self.session.lock().unwrap() { + match session.uid_fetch(set, PREFETCH_FLAGS) { + Ok(msgs) => { + if msgs.is_empty() { + warn!( + context, + "Cannot delete on IMAP, {}: imap entry gone '{}'", + display_imap_id, + message_id, + ); + return ImapResult::Failed; } - } + let remote_message_id = + prefetch_get_message_id(msgs.first().unwrap()).unwrap_or_default(); - // mark the message for deletion - if !self.add_flag_finalized(context, *server_uid, "\\Deleted") { - warn!(context, "Cannot mark message as \"Deleted\"."); - } else { - emit_event!( + if remote_message_id != message_id.as_ref() { + warn!( + context, + "Cannot delete on IMAP, {}: remote message-id '{}' != '{}'", + display_imap_id, + remote_message_id, + message_id, + ); + } + *uid = 0; + } + Err(err) => { + warn!( context, - Event::ImapMessageDeleted(format!( - "IMAP Message {} marked as deleted [{}]", - display_imap_id, message_id.as_ref() - )) + "Cannot delete {} on IMAP: {}", display_imap_id, err ); - self.config.write().unwrap().selected_folder_needs_expunge = true; - success = true + *uid = 0; } } } - if success { - 1 + // mark the message for deletion + if !self.add_flag_finalized(context, *uid, "\\Deleted") { + warn!( + context, + "Cannot mark message {} as \"Deleted\".", display_imap_id + ); + ImapResult::Failed } else { - self.is_connected() as usize + emit_event!( + context, + Event::ImapMessageDeleted(format!( + "IMAP Message {} marked as deleted [{}]", + display_imap_id, message_id + )) + ); + self.config.write().unwrap().selected_folder_needs_expunge = true; + ImapResult::Success } } diff --git a/src/job.rs b/src/job.rs index 1595626cc..52c1af31a 100644 --- a/src/job.rs +++ b/src/job.rs @@ -216,13 +216,6 @@ impl Job { fn do_DC_JOB_MOVE_MSG(&mut self, context: &Context) { let inbox = context.inbox.read().unwrap(); - if !inbox.is_connected() { - connect_to_inbox(context, &inbox); - if !inbox.is_connected() { - self.try_again_later(3, Some("could not connect".into())); - return; - } - } if let Ok(msg) = Message::load_from_db(context, self.foreign_id) { if context .sql @@ -269,26 +262,18 @@ impl Job { if let Ok(mut msg) = Message::load_from_db(context, self.foreign_id) { if !msg.rfc724_mid.is_empty() { /* eg. device messages have no Message-ID */ - let mut delete_from_server = true; - if message::rfc724_mid_cnt(context, &msg.rfc724_mid) != 1 { + if message::rfc724_mid_cnt(context, &msg.rfc724_mid) > 1 { info!( context, "The message is deleted from the server when all parts are deleted.", ); - delete_from_server = false; - } - /* if this is the last existing part of the message, we delete the message from the server */ - if delete_from_server { - if !inbox.is_connected() { - connect_to_inbox(context, &inbox); - if !inbox.is_connected() { - self.try_again_later(3i32, None); - return; - } - } + } else { + /* if this is the last existing part of the message, + we delete the message from the server */ let mid = msg.rfc724_mid; let server_folder = msg.server_folder.as_ref().unwrap(); - if 0 == inbox.delete_msg(context, &mid, server_folder, &mut msg.server_uid) { + let res = inbox.delete_msg(context, &mid, server_folder, &mut msg.server_uid); + if res == ImapResult::RetryLater { self.try_again_later(-1i32, None); return; } @@ -302,25 +287,24 @@ impl Job { fn do_DC_JOB_MARKSEEN_MSG_ON_IMAP(&mut self, context: &Context) { let inbox = context.inbox.read().unwrap(); - if !inbox.is_connected() { - connect_to_inbox(context, &inbox); - if !inbox.is_connected() { - self.try_again_later(3i32, None); - return; - } - } if let Ok(msg) = Message::load_from_db(context, self.foreign_id) { let folder = msg.server_folder.as_ref().unwrap(); match inbox.set_seen(context, folder, msg.server_uid) { - ImapResult::Failed => {} ImapResult::RetryLater => { self.try_again_later(3i32, None); } - _ => { + ImapResult::AlreadyDone => {} + ImapResult::Success | ImapResult::Failed => { + // XXX the message might just have been moved + // we want to send out an MDN anyway + // The job will not be retried so locally + // there is no risk of double-sending MDNs. if 0 != msg.param.get_int(Param::WantsMdn).unwrap_or_default() && context.get_config_bool(Config::MdnsEnabled) { - send_mdn(context, msg.id); + if let Err(err) = send_mdn(context, msg.id) { + warn!(context, "could not send out mdn for {}: {}", msg.id, err); + } } } } @@ -336,16 +320,9 @@ impl Job { .to_string(); let uid = self.param.get_int(Param::ServerUid).unwrap_or_default() as u32; let inbox = context.inbox.read().unwrap(); - - if !inbox.is_connected() { - connect_to_inbox(context, &inbox); - if !inbox.is_connected() { - self.try_again_later(3, None); - return; - } - } - if inbox.set_seen(context, &folder, uid) == ImapResult::Failed { + if inbox.set_seen(context, &folder, uid) == ImapResult::RetryLater { self.try_again_later(3i32, None); + return; } if 0 != self.param.get_int(Param::AlsoMove).unwrap_or_default() { if context @@ -360,7 +337,7 @@ impl Job { if let Some(dest_folder) = dest_folder { let mut dest_uid = 0; if ImapResult::RetryLater - == inbox.mv(context, folder, uid, dest_folder, &mut dest_uid) + == inbox.mv(context, &folder, uid, &dest_folder, &mut dest_uid) { self.try_again_later(3, None); } @@ -941,7 +918,7 @@ fn suspend_smtp_thread(context: &Context, suspend: bool) { } } -fn connect_to_inbox(context: &Context, inbox: &Imap) -> libc::c_int { +pub fn connect_to_inbox(context: &Context, inbox: &Imap) -> libc::c_int { let ret_connected = dc_connect_to_configured_imap(context, inbox); if 0 != ret_connected { inbox.set_watch_folder("INBOX".into());