feat: Sort received outgoing message down if it's fresher than all non fresh messages

Received messages shouldn't mingle with just sent ones and appear somewhere in the middle of the
chat, so we go after the newest non fresh message.

But if a received outgoing message is older than some `InSeen` message, better sort the received
message purely by timestamp (this is an heuristic in order not to break the Gmail-like case
simulated by `verified_chats::test_old_message_4()`). We could place the received message just
before that `InSeen` message, but anyway the user may not notice it.

At least this fixes outgoing messages sorting for shared accounts where messages from other devices
should be sorted the same way as incoming ones.
This commit is contained in:
iequidoo
2024-07-24 21:14:00 -03:00
committed by iequidoo
parent f1ca689f99
commit d660f55a99
5 changed files with 68 additions and 14 deletions

View File

@@ -616,8 +616,9 @@ impl ChatId {
contact_id: Option<ContactId>, contact_id: Option<ContactId>,
) -> Result<()> { ) -> Result<()> {
let sort_to_bottom = true; let sort_to_bottom = true;
let (received, incoming) = (false, false);
let ts = self let ts = self
.calc_sort_timestamp(context, timestamp_sent, sort_to_bottom, false) .calc_sort_timestamp(context, timestamp_sent, sort_to_bottom, received, incoming)
.await? .await?
// Always sort protection messages below `SystemMessage::SecurejoinWait{,Timeout}` ones // Always sort protection messages below `SystemMessage::SecurejoinWait{,Timeout}` ones
// in case of race conditions. // in case of race conditions.
@@ -1392,12 +1393,14 @@ impl ChatId {
/// corresponding event in case of a system message (usually the current system time). /// corresponding event in case of a system message (usually the current system time).
/// `always_sort_to_bottom` makes this ajust the returned timestamp up so that the message goes /// `always_sort_to_bottom` makes this ajust the returned timestamp up so that the message goes
/// to the chat bottom. /// to the chat bottom.
/// `received` -- whether the message is received. Otherwise being sent.
/// `incoming` -- whether the message is incoming. /// `incoming` -- whether the message is incoming.
pub(crate) async fn calc_sort_timestamp( pub(crate) async fn calc_sort_timestamp(
self, self,
context: &Context, context: &Context,
message_timestamp: i64, message_timestamp: i64,
always_sort_to_bottom: bool, always_sort_to_bottom: bool,
received: bool,
incoming: bool, incoming: bool,
) -> Result<i64> { ) -> Result<i64> {
let mut sort_timestamp = cmp::min(message_timestamp, smeared_time(context)); let mut sort_timestamp = cmp::min(message_timestamp, smeared_time(context));
@@ -1418,25 +1421,38 @@ impl ChatId {
(self, MessageState::OutDraft), (self, MessageState::OutDraft),
) )
.await? .await?
} else if incoming { } else if received {
// get newest non fresh message for this chat. // Received messages shouldn't mingle with just sent ones and appear somewhere in the
// middle of the chat, so we go after the newest non fresh message.
// If a user hasn't been online for some time, the Inbox is fetched first and then the //
// Sentbox. In order for Inbox and Sent messages to be allowed to mingle, outgoing // But if a received outgoing message is older than some seen message, better sort the
// messages are purely sorted by their sent timestamp. NB: The Inbox must be fetched // received message purely by timestamp. We could place it just before that seen
// first otherwise Inbox messages would be always below old Sentbox messages. We could // message, but anyway the user may not notice it.
// take in the query below only incoming messages, but then new incoming messages would //
// mingle with just sent outgoing ones and apear somewhere in the middle of the chat. // NB: Received outgoing messages may break sorting of fresh incoming ones, but this
// shouldn't happen frequently. Seen incoming messages don't really break sorting of
// fresh ones, they rather mean that older incoming messages are actually seen as well.
context context
.sql .sql
.query_get_value( .query_row_optional(
"SELECT MAX(timestamp) "SELECT MAX(timestamp), MAX(IIF(state=?,timestamp_sent,0))
FROM msgs FROM msgs
WHERE chat_id=? AND hidden=0 AND state>? WHERE chat_id=? AND hidden=0 AND state>?
HAVING COUNT(*) > 0", HAVING COUNT(*) > 0",
(self, MessageState::InFresh), (MessageState::InSeen, self, MessageState::InFresh),
|row| {
let ts: i64 = row.get(0)?;
let ts_sent_seen: i64 = row.get(1)?;
Ok((ts, ts_sent_seen))
},
) )
.await? .await?
.and_then(|(ts, ts_sent_seen)| {
match incoming || ts_sent_seen <= message_timestamp {
true => Some(ts),
false => None,
}
})
} else { } else {
None None
}; };

View File

@@ -1255,11 +1255,13 @@ async fn add_parts(
let in_fresh = state == MessageState::InFresh; let in_fresh = state == MessageState::InFresh;
let sort_to_bottom = false; let sort_to_bottom = false;
let received = true;
let sort_timestamp = chat_id let sort_timestamp = chat_id
.calc_sort_timestamp( .calc_sort_timestamp(
context, context,
mime_parser.timestamp_sent, mime_parser.timestamp_sent,
sort_to_bottom, sort_to_bottom,
received,
mime_parser.incoming, mime_parser.incoming,
) )
.await?; .await?;

View File

@@ -4811,6 +4811,36 @@ async fn test_protected_group_add_remove_member_missing_key() -> Result<()> {
Ok(()) Ok(())
} }
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_older_message_from_2nd_device() -> Result<()> {
let mut tcm = TestContextManager::new();
let alice = &tcm.alice().await;
let chat_id = alice
.create_chat_with_contact("", "bob@example.net")
.await
.id;
alice.send_text(chat_id, "We share this account").await;
let received = receive_imf(
alice,
b"From: alice@example.org\n\
To: bob@example.net\n\
Message-ID: <1234-2-4@example.org>\n\
Date: Sat, 07 Dec 1970 19:00:26 +0000\n\
\n\
I'm Alice too\n",
true,
)
.await?
.unwrap();
alice
.golden_test_chat(
received.chat_id,
"receive_imf_older_message_from_2nd_device",
)
.await;
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_dont_create_adhoc_group_on_member_removal() -> Result<()> { async fn test_dont_create_adhoc_group_on_member_removal() -> Result<()> {
let mut tcm = TestContextManager::new(); let mut tcm = TestContextManager::new();

View File

@@ -73,8 +73,9 @@ pub(super) async fn start_protocol(context: &Context, invite: QrInvite) -> Resul
// Calculate the sort timestamp before checking the chat protection status so that if we // Calculate the sort timestamp before checking the chat protection status so that if we
// race with its change, we don't add our message below the protection message. // race with its change, we don't add our message below the protection message.
let sort_to_bottom = true; let sort_to_bottom = true;
let (received, incoming) = (false, false);
let ts_sort = chat_id let ts_sort = chat_id
.calc_sort_timestamp(context, 0, sort_to_bottom, false) .calc_sort_timestamp(context, 0, sort_to_bottom, received, incoming)
.await?; .await?;
if chat_id.is_protected(context).await? == ProtectionStatus::Unprotected { if chat_id.is_protected(context).await? == ProtectionStatus::Unprotected {
let ts_start = time(); let ts_start = time();

View File

@@ -0,0 +1,5 @@
Single#Chat#10: bob@example.net [bob@example.net]
--------------------------------------------------------------------------------
Msg#10: Me (Contact#Contact#Self): We share this account √
Msg#11: Me (Contact#Contact#Self): I'm Alice too √
--------------------------------------------------------------------------------