Skip to content
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

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

Merged
merged 1 commit into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions src/chat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -616,8 +616,9 @@ impl ChatId {
contact_id: Option<ContactId>,
) -> Result<()> {
let sort_to_bottom = true;
let (received, incoming) = (false, false);
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?
// Always sort protection messages below `SystemMessage::SecurejoinWait{,Timeout}` ones
// in case of race conditions.
Expand Down Expand Up @@ -1392,12 +1393,14 @@ impl ChatId {
/// 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
/// to the chat bottom.
/// `received` -- whether the message is received. Otherwise being sent.
/// `incoming` -- whether the message is incoming.
pub(crate) async fn calc_sort_timestamp(
self,
context: &Context,
message_timestamp: i64,
always_sort_to_bottom: bool,
received: bool,
incoming: bool,
) -> Result<i64> {
let mut sort_timestamp = cmp::min(message_timestamp, smeared_time(context));
Expand All @@ -1418,25 +1421,38 @@ impl ChatId {
(self, MessageState::OutDraft),
)
.await?
} else if incoming {
// get newest non fresh message for this chat.

// 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
// messages are purely sorted by their sent timestamp. NB: The Inbox must be fetched
// first otherwise Inbox messages would be always below old Sentbox messages. We could
// 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.
} else if received {
// 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 seen message, better sort the
// received message purely by timestamp. We could place it just before that seen
// message, but anyway the user may not notice it.
//
// 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
.sql
.query_get_value(
"SELECT MAX(timestamp)
.query_row_optional(
"SELECT MAX(timestamp), MAX(IIF(state=?,timestamp_sent,0))
FROM msgs
WHERE chat_id=? AND hidden=0 AND state>?
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?
.and_then(|(ts, ts_sent_seen)| {
match incoming || ts_sent_seen <= message_timestamp {
true => Some(ts),
false => None,
}
})
} else {
None
};
Expand Down
2 changes: 2 additions & 0 deletions src/receive_imf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1255,11 +1255,13 @@ async fn add_parts(

let in_fresh = state == MessageState::InFresh;
let sort_to_bottom = false;
let received = true;
let sort_timestamp = chat_id
.calc_sort_timestamp(
context,
mime_parser.timestamp_sent,
sort_to_bottom,
received,
mime_parser.incoming,
)
.await?;
Expand Down
30 changes: 30 additions & 0 deletions src/receive_imf/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4811,6 +4811,36 @@ async fn test_protected_group_add_remove_member_missing_key() -> Result<()> {
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)]
async fn test_dont_create_adhoc_group_on_member_removal() -> Result<()> {
let mut tcm = TestContextManager::new();
Expand Down
3 changes: 2 additions & 1 deletion src/securejoin/bob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
// race with its change, we don't add our message below the protection message.
let sort_to_bottom = true;
let (received, incoming) = (false, false);
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?;
if chat_id.is_protected(context).await? == ProtectionStatus::Unprotected {
let ts_start = time();
Expand Down
5 changes: 5 additions & 0 deletions test-data/golden/receive_imf_older_message_from_2nd_device
Original file line number Diff line number Diff line change
@@ -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 √
--------------------------------------------------------------------------------
Loading