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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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.

…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 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.

At least this fixes outgoing messages sorting for shared accounts where messages from other devices
should be sorted the same way as incoming ones.
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