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

fix: Order of messages if Sentbox is synced before Inbox #5813

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
8 changes: 6 additions & 2 deletions deltachat-ffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use deltachat::context::{Context, ContextBuilder};
use deltachat::ephemeral::Timer as EphemeralTimer;
use deltachat::imex::BackupProvider;
use deltachat::key::preconfigure_keypair;
use deltachat::message::MsgId;
use deltachat::message::{MessageState, MsgId};
use deltachat::qr_code_generator::{create_qr_svg, generate_backup_qr, get_securejoin_qr_svg};
use deltachat::stock_str::StockMessage;
use deltachat::webxdc::StatusUpdateSerial;
Expand Down Expand Up @@ -3294,7 +3294,11 @@ pub unsafe extern "C" fn dc_msg_get_state(msg: *mut dc_msg_t) -> libc::c_int {
return 0;
}
let ffi_msg = &*msg;
ffi_msg.message.get_state() as libc::c_int
let state = match ffi_msg.message.get_state() {
MessageState::OutRcvd => MessageState::OutDelivered,
s => s,
};
state as libc::c_int
}

#[no_mangle]
Expand Down
2 changes: 1 addition & 1 deletion deltachat-ffi/src/lot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ impl From<MessageState> for LotState {
OutDraft => LotState::MsgOutDraft,
OutPending => LotState::MsgOutPending,
OutFailed => LotState::MsgOutFailed,
OutDelivered => LotState::MsgOutDelivered,
OutDelivered | OutRcvd => LotState::MsgOutDelivered,
OutMdnRcvd => LotState::MsgOutMdnRcvd,
}
}
Expand Down
12 changes: 8 additions & 4 deletions deltachat-jsonrpc/src/api/types/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use deltachat::contact::Contact;
use deltachat::context::Context;
use deltachat::download;
use deltachat::message::Message;
use deltachat::message::MessageState;
use deltachat::message::MsgId;
use deltachat::message::Viewtype;
use deltachat::reaction::get_msg_reactions;
Expand Down Expand Up @@ -136,6 +137,12 @@ impl MessageObject {

let parent_id = message.parent(context).await?.map(|m| m.get_id().to_u32());

let state = match message.get_state() {
MessageState::OutRcvd => MessageState::OutDelivered,
s => s,
}
.to_u32()
.context("state conversion to number failed")?;
let download_state = message.download_state().into();

let quote = if let Some(quoted_text) = message.quoted_text() {
Expand Down Expand Up @@ -197,10 +204,7 @@ impl MessageObject {
has_location: message.has_location(),
has_html: message.has_html(),
view_type: message.get_viewtype().into(),
state: message
.get_state()
.to_u32()
.context("state conversion to number failed")?,
state,
error: message.error(),

timestamp: message.get_timestamp(),
Expand Down
10 changes: 8 additions & 2 deletions src/chat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1445,9 +1445,14 @@ impl ChatId {
.query_row_optional(
"SELECT MAX(timestamp), MAX(IIF(state=?,timestamp_sent,0))
FROM msgs
WHERE chat_id=? AND hidden=0 AND state>?
WHERE chat_id=? AND hidden=0 AND state>? AND state!=?
HAVING COUNT(*) > 0",
(MessageState::InSeen, self, MessageState::InFresh),
(
MessageState::InSeen,
self,
MessageState::InFresh,
MessageState::OutRcvd,
),
|row| {
let ts: i64 = row.get(0)?;
let ts_sent_seen: i64 = row.get(1)?;
Expand Down Expand Up @@ -4198,6 +4203,7 @@ pub async fn resend_msgs(context: &Context, msg_ids: &[MsgId]) -> Result<()> {
MessageState::OutPending
| MessageState::OutFailed
| MessageState::OutDelivered
| MessageState::OutRcvd
| MessageState::OutMdnRcvd => {
message::update_msg_state(context, msg.id, MessageState::OutPending).await?
}
Expand Down
12 changes: 9 additions & 3 deletions src/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1365,6 +1365,9 @@ pub enum MessageState {
/// the OutFailed state if we get such a hint from the server.
OutDelivered = 26,

/// Received outgoing message sent by another MUA or device.
OutRcvd = 27,

/// Outgoing message read by the recipient (two checkmarks; this
/// requires goodwill on the receiver's side). Not used in the db for new messages.
OutMdnRcvd = 28,
Expand All @@ -1385,6 +1388,7 @@ impl std::fmt::Display for MessageState {
Self::OutPending => "Pending",
Self::OutFailed => "Failed",
Self::OutDelivered => "Delivered",
Self::OutRcvd => "Other MUA",
Self::OutMdnRcvd => "Read",
}
)
Expand All @@ -1397,7 +1401,9 @@ impl MessageState {
use MessageState::*;
matches!(
self,
OutPreparing | OutPending | OutDelivered | OutMdnRcvd // OutMdnRcvd can still fail because it could be a group message and only some recipients failed.
// OutMdnRcvd can still fail because it could be a group message and only some
// recipients failed.
OutPreparing | OutPending | OutDelivered | OutRcvd | OutMdnRcvd
)
}

Expand All @@ -1406,13 +1412,13 @@ impl MessageState {
use MessageState::*;
matches!(
self,
OutPreparing | OutDraft | OutPending | OutFailed | OutDelivered | OutMdnRcvd
OutPreparing | OutDraft | OutPending | OutFailed | OutDelivered | OutRcvd | 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 {
if has_mdns && self >= MessageState::OutDelivered {
return MessageState::OutMdnRcvd;
}
self
Expand Down
4 changes: 2 additions & 2 deletions src/mimeparser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3805,7 +3805,7 @@ Message.
)
.await?;
let msg = alice.get_last_msg().await;
assert_eq!(msg.state, MessageState::OutDelivered);
assert_eq!(msg.state, MessageState::OutRcvd);

// Due to a bug in the old version running on the other device, Alice receives a read
// receipt from self.
Expand Down Expand Up @@ -3844,7 +3844,7 @@ Message.

// Check that the state has not changed to `MessageState::OutMdnRcvd`.
let msg = Message::load_from_db(&alice, msg.id).await?;
assert_eq!(msg.state, MessageState::OutDelivered);
assert_eq!(msg.state, MessageState::OutRcvd);

Ok(())
}
Expand Down
2 changes: 1 addition & 1 deletion src/reaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ Can we chat at 1pm pacific, today?"
)
.await?;
let msg = alice.get_last_msg().await;
assert_eq!(msg.state, MessageState::OutDelivered);
assert_eq!(msg.state, MessageState::OutRcvd);
let reactions = get_msg_reactions(&alice, msg.id).await?;
let contacts = reactions.contacts();
assert_eq!(contacts.len(), 0);
Expand Down
4 changes: 1 addition & 3 deletions src/receive_imf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1031,9 +1031,7 @@ async fn add_parts(
} else {
// Outgoing

// the mail is on the IMAP server, probably it is also delivered.
// We cannot recreate other states (read, error).
state = MessageState::OutDelivered;
state = MessageState::OutRcvd;
to_id = to_ids.first().copied().unwrap_or_default();

let self_sent = to_ids.len() == 1 && to_ids.contains(&ContactId::SELF);
Expand Down
6 changes: 3 additions & 3 deletions src/receive_imf/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ async fn test_read_receipt_and_unarchive() -> Result<()> {
let msg = get_chat_msg(&t, group_id, 0, 1).await;
assert_eq!(msg.is_dc_message, MessengerMessage::Yes);
assert_eq!(msg.text, "hello");
assert_eq!(msg.state, MessageState::OutDelivered);
assert_eq!(msg.state, MessageState::OutRcvd);
let group = Chat::load_from_db(&t, group_id).await?;
assert!(group.get_visibility() == ChatVisibility::Normal);

Expand Down Expand Up @@ -793,7 +793,7 @@ async fn test_parse_ndn(
if error_msg.is_some() {
MessageState::OutFailed
} else {
MessageState::OutDelivered
MessageState::OutRcvd
}
);

Expand Down Expand Up @@ -4651,7 +4651,7 @@ async fn test_outgoing_msg_forgery() -> Result<()> {

let sent_msg = malice.send_text(malice_chat_id, "hi from malice").await;
let msg = alice.recv_msg(&sent_msg).await;
assert_eq!(msg.state, MessageState::OutDelivered);
assert_eq!(msg.state, MessageState::OutRcvd);
assert!(!msg.get_showpadlock());

Ok(())
Expand Down
2 changes: 1 addition & 1 deletion src/tests/verified_chats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ async fn test_old_message_5() -> Result<()> {
.await?
.unwrap();

assert!(msg_sent.sort_timestamp == msg_incoming.sort_timestamp);
assert!(msg_incoming.sort_timestamp < msg_sent.sort_timestamp);
alice
.golden_test_chat(msg_sent.chat_id, "test_old_message_5")
.await;
Expand Down
2 changes: 1 addition & 1 deletion test-data/golden/receive_imf_older_message_from_2nd_device
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Single#Chat#10: [email protected] [[email protected]]
--------------------------------------------------------------------------------
Msg#10: Me (Contact#Contact#Self): We share this account √
Msg#11: Me (Contact#Contact#Self): I'm Alice too
Msg#11: Me (Contact#Contact#Self): I'm Alice too
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this means that this is a breaking change (i.e. will need UI adaptions)? If so, this PR should start with fix! instead of fix, and the PR description should mention this.

And, it means that outgoing messages that were sent by a different device won't have any checkmarks anymore? I personally don't think that's a good idea because that's not how users know it from other messengers, but if others think otherwise, I'd be happy to be outvoted / convinced otherwise.

Copy link
Collaborator Author

@iequidoo iequidoo Oct 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it shouldn't be a breaking change, i forgot to map OutRcvd to OutDelivered in all APIs. Fixed this.

EDIT: Only for REPL the display of received outgoing messages changes.

--------------------------------------------------------------------------------
2 changes: 1 addition & 1 deletion test-data/golden/test_old_message_5
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Single#Chat#10: Bob [[email protected]]
--------------------------------------------------------------------------------
Msg#10: Me (Contact#Contact#Self): Happy birthday, Bob! √
Msg#11: (Contact#Contact#10): Happy birthday to me, Alice! [FRESH]
Msg#10: Me (Contact#Contact#Self): Happy birthday, Bob!
--------------------------------------------------------------------------------
2 changes: 1 addition & 1 deletion test-data/golden/test_outgoing_mua_msg
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ Single#Chat#10: [email protected] [[email protected]] 🛡️
--------------------------------------------------------------------------------
Msg#10: info (Contact#Contact#Info): Messages are guaranteed to be end-to-end encrypted from now on. [NOTICED][INFO 🛡️]
Msg#11🔒: (Contact#Contact#10): Heyho from DC [FRESH]
Msg#12: Me (Contact#Contact#Self): One classical MUA message
Msg#12: Me (Contact#Contact#Self): One classical MUA message
Msg#13🔒: Me (Contact#Contact#Self): Sending with DC again √
--------------------------------------------------------------------------------
Loading