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

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Jul 25, 2024

No description provided.

@iequidoo iequidoo force-pushed the iequidoo/sort-rcvd-outgoing-msgs-down branch from bee519d to c349908 Compare July 25, 2024 00:42
@iequidoo iequidoo marked this pull request as ready for review July 25, 2024 00:54
@iequidoo iequidoo marked this pull request as draft July 25, 2024 14:01
@iequidoo iequidoo force-pushed the iequidoo/sort-rcvd-outgoing-msgs-down branch from c349908 to 254f6c4 Compare July 25, 2024 21:11
@iequidoo iequidoo marked this pull request as ready for review July 25, 2024 21:12
@link2xt
Copy link
Collaborator

link2xt commented Jul 28, 2024

But if a received outgoing message is older than some non fresh message

Why does this happen? If we download messages in order, then message is usually newer than all messages received so far.

Does this PR try to fix some case where messages are pulled from Sent folder when used together with non-DC client?

@iequidoo
Copy link
Collaborator Author

iequidoo commented Jul 28, 2024

Why does this happen? If we download messages in order, then message is usually newer than all messages received so far.

This may happen if Sentbox is fetched after Inbox, verified_chats::test_old_message_4() simulates this and i don't want to break this scenario now:

    let msg_incoming = receive_imf(
        &alice,
        b"From: Bob <bob@example.net>\n\                                                                                                                                                                             
          To: alice@example.org\n\                                                                                                                                                                                   
          Message-ID: <1234-2-3@example.org>\n\                                                                                                                                                                      
          Date: Sun, 08 Dec 2019 19:00:27 +0000\n\                                                                                                                                                                   
          \n\                                                                                                                                                                                                        
          Thanks, Alice!\n",
        true,
    )
    .await?
    .unwrap();
                                                                                                                                                                                                                     
    let msg_sent = receive_imf(
        &alice,
        b"From: alice@example.org\n\                                                                                                                                                                                 
          To: Bob <bob@example.net>\n\                                                                                                                                                                               
          Message-ID: <1234-2-4@example.org>\n\                                                                                                                                                                      
          Date: Sat, 07 Dec 2019 19:00:27 +0000\n\                                                                                                                                                                   
          \n\                                                                                                                                                                                                        
          Happy birthday, Bob!\n",
        true,
    )
    .await?
    .unwrap();

Of course Delta Chat doesn't always look into folders in this order, it may fetch Sentbox messages first, and if we change the test this way and also give the Sentbox message later Date e.g. 09 Dec 2019, it would appear in the chat before the Inbox message which is wrong and happens because all outgoing messages are currently considered "noticed" (not really, but when it comes to sorting) because they get OutDelivered state in receive_imf::add_parts(). Also this breaks sorting of new incoming messages (as i noted in the code comment). I think this is not quite correct and would suggest to add some MessageState::Out = 17, i.e. somewhat undefined, state for received outgoing messages and sort such messages together with InFresh messages.

Does this PR try to fix some case where messages are pulled from Sent folder when used together with non-DC client?

No, it only tries to add new outgoing messages (e.g. from other devices) to the of the chat when possible, if they are delayed for any reason. At least they should appear in the chat under messages sent from the current device. I rather try to keep the current behaviour when old messages are pulled from Sentbox.

EDIT: Not sure about adding MessageState::Out though, it's not clear how to handle MDNs for them.

@iequidoo
Copy link
Collaborator Author

Not sure about adding MessageState::Out though, it's not clear how to handle MDNs for them.

MessageState::OutMdnRcvd isn't really used in the code, so it can be safely removed. Its value is even more questionable for group chat messages. I.e. it should be left in the enum for compatibility, but there's no need in this state in the db. Having msgs_mdns is sufficient and MessageState::OutMdnRcvd can be easily computed from it w/o significant overhead, there's an index by msg_id. So we can introduce MessageState::Out (or OutRcvd) that doesn't need to be converted to OutMdnRcvd or some other state, and such messages can form message sorting window together with InFresh messages.

@link2xt
Copy link
Collaborator

link2xt commented Jul 29, 2024

No, it only tries to add new outgoing messages (e.g. from other devices) to the of the chat when possible, if they are delayed for any reason. At least they should appear in the chat under messages sent from the current device. I rather try to keep the current behaviour when old messages are pulled from Sentbox.

So this is a fix for Gmail? On most providers outgoing messages from Delta Chat never appears in the Sent folder, they are BCC-ed to self and arrive into the Inbox.

@iequidoo
Copy link
Collaborator Author

So this is a fix for Gmail? On most providers outgoing messages from Delta Chat never appears in the Sent folder, they are BCC-ed to self and arrive into the Inbox.

This PR is a fix for multi-device, it makes received outgoing messages appear in the bottom of the chat even if they have Date earlier than messages sent from the current device, to make them more visible. This may happen if messages are delayed because of network problems or clocks are not synchronised. Before that was done only for incoming messages.

As for Gmail, particularly if Delta Chat is used together with Gmail client, there's another problem -- Sentbox messages may appear before Inbox ones even if the latter are older and this can be fixed by introducing some MessageState::OutRcvd so that such messages can mingle with fresh incoming messages. OutPending, OutDelivered etc. messages shouldn't mingle with them, they are sent locally by the user and there's no need to make them more noticeable.

@iequidoo
Copy link
Collaborator Author

iequidoo commented Jul 29, 2024

But if a received outgoing message is older than some non fresh message, better sort the received
message purely by timestamp.

Actually this is an heuristic in this PR in order not to break the Gmail case simulated by test_old_message_4(). Maybe even better to do the following: if a received outgoing message is older than some InSeen message, sort the received message purely by timestamp. EDIT: Done.

src/chat.rs Outdated Show resolved Hide resolved
src/chat.rs Outdated Show resolved Hide resolved
@iequidoo iequidoo force-pushed the iequidoo/sort-rcvd-outgoing-msgs-down branch from 716e908 to c55967b Compare October 14, 2024 15:54
@iequidoo
Copy link
Collaborator Author

iequidoo commented Oct 14, 2024

Didn't expect that, but this is incompatible with 796b0d7, need to debug.

EDIT: Can't reproduce the test_verified_group_vs_delete_server_after failure on my machine...
EDIT: This PR isn't the reason, the test also fails on main: https://github.com/deltachat/deltachat-core-rust/actions/runs/11329131300/job/31507389864. Probably the reason is a recent CI chatmail server change.
EDIT: On my machine at least all the CFFI Python tests pass.

@iequidoo iequidoo marked this pull request as draft October 14, 2024 16:28
@iequidoo iequidoo force-pushed the iequidoo/sort-rcvd-outgoing-msgs-down branch 2 times, most recently from 9b8a432 to 3214eb6 Compare October 14, 2024 18:25
@iequidoo iequidoo marked this pull request as ready for review October 14, 2024 19:01
…n 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.
@iequidoo iequidoo force-pushed the iequidoo/sort-rcvd-outgoing-msgs-down branch from 3214eb6 to 6c01309 Compare October 14, 2024 23:49
@iequidoo
Copy link
Collaborator Author

Only test_verified_group_vs_delete_server_after and test_prefer_encrypt failed now which i can't reproduce locally with nine.testrun.org, so i think this may be merged taking into account that it passed the CI before

@iequidoo iequidoo merged commit d660f55 into main Oct 15, 2024
33 of 37 checks passed
@iequidoo iequidoo deleted the iequidoo/sort-rcvd-outgoing-msgs-down branch October 15, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants