From 49a26e1c4a0c9b70baec4a3c455e871f4a53b6e5 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Tue, 30 Jul 2024 22:50:52 -0300 Subject: [PATCH] refactor: Don't update message state to `OutMdnRcvd` anymore This state can be computed from the `msgs_mdns` table without significant overhead as we have an index by msg_id there. --- src/message.rs | 44 ++++++++++++++++++++++++++++------- src/mimeparser.rs | 58 +++++++++++++++++++++-------------------------- 2 files changed, 62 insertions(+), 40 deletions(-) diff --git a/src/message.rs b/src/message.rs index ee3b898cdb..b984bb0df6 100644 --- a/src/message.rs +++ b/src/message.rs @@ -81,7 +81,20 @@ impl MsgId { pub async fn get_state(self, context: &Context) -> Result { let result = context .sql - .query_get_value("SELECT state FROM msgs WHERE id=?", (self,)) + .query_row_optional( + concat!( + "SELECT m.state, mdns.msg_id", + " FROM msgs m LEFT JOIN msgs_mdns mdns ON mdns.msg_id=m.id", + " WHERE id=?", + " LIMIT 1", + ), + (self,), + |row| { + let state: MessageState = row.get(0)?; + let mdn_msg_id: Option = row.get(1)?; + Ok(state.with_mdns(mdn_msg_id.is_some())) + }, + ) .await? .unwrap_or_default(); Ok(result) @@ -519,6 +532,7 @@ impl Message { " m.ephemeral_timestamp AS ephemeral_timestamp,", " m.type AS type,", " m.state AS state,", + " mdns.msg_id AS mdn_msg_id,", " m.download_state AS download_state,", " m.error AS error,", " m.msgrmsg AS msgrmsg,", @@ -529,11 +543,16 @@ impl Message { " m.hidden AS hidden,", " m.location_id AS location,", " c.blocked AS blocked", - " FROM msgs m LEFT JOIN chats c ON c.id=m.chat_id", - " WHERE m.id=? AND chat_id!=3;" + " FROM msgs m", + " LEFT JOIN chats c ON c.id=m.chat_id", + " LEFT JOIN msgs_mdns mdns ON mdns.msg_id=m.id", + " WHERE m.id=? AND chat_id!=3", + " LIMIT 1", ), (id,), |row| { + let state: MessageState = row.get("state")?; + let mdn_msg_id: Option = row.get("mdn_msg_id")?; let text = match row.get_ref("txt")? { rusqlite::types::ValueRef::Text(buf) => { match String::from_utf8(buf.to_vec()) { @@ -568,7 +587,7 @@ impl Message { ephemeral_timer: row.get("ephemeral_timer")?, ephemeral_timestamp: row.get("ephemeral_timestamp")?, viewtype: row.get("type")?, - state: row.get("state")?, + state: state.with_mdns(mdn_msg_id.is_some()), download_state: row.get("download_state")?, error: Some(row.get::<_, String>("error")?) .filter(|error| !error.is_empty()), @@ -1353,7 +1372,7 @@ pub enum MessageState { OutDelivered = 26, /// Outgoing message read by the recipient (two checkmarks; this - /// requires goodwill on the receiver's side) + /// requires goodwill on the receiver's side). Not used in the db for new messages. OutMdnRcvd = 28, } @@ -1396,6 +1415,14 @@ impl MessageState { OutPreparing | OutDraft | OutPending | OutFailed | OutDelivered | OutMdnRcvd ) } + + /// Returns adjusted message state if the message has MDNs. + pub(crate) fn with_mdns(self, has_mdns: bool) -> Self { + if self == MessageState::OutDelivered && has_mdns { + return MessageState::OutMdnRcvd; + } + self + } } /// Returns contacts that sent read receipts and the time of reading. @@ -1778,6 +1805,10 @@ pub(crate) async fn update_msg_state( msg_id: MsgId, state: MessageState, ) -> Result<()> { + ensure!( + state != MessageState::OutMdnRcvd, + "Update msgs_mdns table instead!" + ); ensure!(state != MessageState::OutFailed, "use set_msg_failed()!"); let error_subst = match state >= MessageState::OutPending { true => ", error=''", @@ -2565,9 +2596,6 @@ mod tests { let payload = alice.pop_sent_msg().await; assert_state(&alice, alice_msg.id, MessageState::OutDelivered).await; - update_msg_state(&alice, alice_msg.id, MessageState::OutMdnRcvd).await?; - assert_state(&alice, alice_msg.id, MessageState::OutMdnRcvd).await; - set_msg_failed(&alice, &mut alice_msg, "badly failed").await?; assert_state(&alice, alice_msg.id, MessageState::OutFailed).await; diff --git a/src/mimeparser.rs b/src/mimeparser.rs index b4d740c014..027f3bdf0b 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -28,10 +28,7 @@ use crate::dehtml::dehtml; use crate::events::EventType; use crate::headerdef::{HeaderDef, HeaderDefMap}; use crate::key::{self, load_self_secret_keyring, DcKey, Fingerprint, SignedPublicKey}; -use crate::message::{ - self, get_vcard_summary, set_msg_failed, update_msg_state, Message, MessageState, MsgId, - Viewtype, -}; +use crate::message::{self, get_vcard_summary, set_msg_failed, Message, MsgId, Viewtype}; use crate::param::{Param, Params}; use crate::peerstate::Peerstate; use crate::simplify::{simplify, SimplifiedText}; @@ -2157,24 +2154,32 @@ async fn handle_mdn( return Ok(()); } - let Some((msg_id, chat_id, msg_state)) = context + let Some((msg_id, chat_id, has_mdns, is_dup)) = context .sql .query_row_optional( concat!( "SELECT", " m.id AS msg_id,", " c.id AS chat_id,", - " m.state AS state", - " FROM msgs m LEFT JOIN chats c ON m.chat_id=c.id", + " mdns.contact_id AS mdn_contact", + " FROM msgs m ", + " LEFT JOIN chats c ON m.chat_id=c.id", + " LEFT JOIN msgs_mdns mdns ON mdns.msg_id=m.id", " WHERE rfc724_mid=? AND from_id=1", - " ORDER BY m.id" + " ORDER BY msg_id DESC, mdn_contact=? DESC", + " LIMIT 1", ), - (&rfc724_mid,), + (&rfc724_mid, from_id), |row| { let msg_id: MsgId = row.get("msg_id")?; let chat_id: ChatId = row.get("chat_id")?; - let msg_state: MessageState = row.get("state")?; - Ok((msg_id, chat_id, msg_state)) + let mdn_contact: Option = row.get("mdn_contact")?; + Ok(( + msg_id, + chat_id, + mdn_contact.is_some(), + mdn_contact == Some(from_id), + )) }, ) .await? @@ -2186,28 +2191,17 @@ async fn handle_mdn( return Ok(()); }; - if !context + if is_dup { + return Ok(()); + } + context .sql - .exists( - "SELECT COUNT(*) FROM msgs_mdns WHERE msg_id=? AND contact_id=?", - (msg_id, from_id), + .execute( + "INSERT INTO msgs_mdns (msg_id, contact_id, timestamp_sent) VALUES (?, ?, ?)", + (msg_id, from_id, timestamp_sent), ) - .await? - { - context - .sql - .execute( - "INSERT INTO msgs_mdns (msg_id, contact_id, timestamp_sent) VALUES (?, ?, ?)", - (msg_id, from_id, timestamp_sent), - ) - .await?; - } - - if msg_state == MessageState::OutPreparing - || msg_state == MessageState::OutPending - || msg_state == MessageState::OutDelivered - { - update_msg_state(context, msg_id, MessageState::OutMdnRcvd).await?; + .await?; + if !has_mdns { context.emit_event(EventType::MsgRead { chat_id, msg_id }); // note(treefit): only matters if it is the last message in chat (but probably too expensive to check, debounce also solves it) chatlist_events::emit_chatlist_item_changed(context, chat_id); @@ -2315,7 +2309,7 @@ mod tests { chat, chatlist::Chatlist, constants::{Blocked, DC_DESIRED_TEXT_LEN, DC_ELLIPSIS}, - message::MessengerMessage, + message::{MessageState, MessengerMessage}, receive_imf::receive_imf, test_utils::{TestContext, TestContextManager}, tools::time,