Skip to content

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Sep 5, 2025

Replacement for #5813 (github doesn't allow to reopen it).
Fix #7169

This fixes a Gmail-like scenario when outgoing messages are saved to Sentbox as well and it is
fetched before Inbox by chance, by allowing received outgoing messages to mingle with fresh incoming
ones. The reason is that a received outgoing message may be a reply (explicit or implicit) to an
incoming message received later, so it's better to sort them together purely by timestamp. Another
case is the user sharing their account with someone else (using another device) or having some
auto-reply bot.

This fixes the described scenarios w/o introducing a new message state for outgoing messages because
locally sent ones have zero timestamp_sent, so we can filter them in calc_sort_timestamp().

As for messages sent locally, there's no need to make them more noticeable even if they are newer,
so received outgoing messages are added after them.

@iequidoo iequidoo marked this pull request as draft September 5, 2025 05:08
@iequidoo iequidoo force-pushed the iequidoo/test_sync_sentbox_then_inbox branch 2 times, most recently from 79418b5 to 54fe9b5 Compare September 5, 2025 05:51
@iequidoo iequidoo marked this pull request as ready for review September 5, 2025 05:51
@iequidoo iequidoo requested review from link2xt and r10s September 5, 2025 06:09
@r10s
Copy link
Contributor

r10s commented Sep 5, 2025

so this one now changes (and potentially complicates and slow down) sql statement?

also state in the database is no longer the onr reported in the api?

i do not get it - and the missing reasoning is also not helpful :) please always add reasoning and some lines what a pr does to the pr description - even of it is repeared from somewhere else

why not remove "warch sent folder" first, which is a clear way forward, and was agreed on. even if not does not catch all cases, it removes compexity and on previous discussions the issue at hand

@iequidoo iequidoo force-pushed the iequidoo/test_sync_sentbox_then_inbox branch from 54fe9b5 to 5438610 Compare September 5, 2025 07:18
This fixes a Gmail-like scenario when outgoing messages are saved to Sentbox as well and it is
fetched before Inbox by chance, by allowing received outgoing messages to mingle with fresh incoming
ones. The reason is that a received outgoing message may be a reply (explicit or implicit) to an
incoming message received later, so it's better to sort them together purely by timestamp. Another
case is the user sharing their account with someone else (using another device) or having some
auto-reply bot.

This fixes the described scenarios w/o introducing a new message state for outgoing messages because
locally sent ones have zero `timestamp_sent`, so we can filter them in `calc_sort_timestamp()`.

As for messages sent locally, there's no need to make them more noticeable even if they are newer,
so received outgoing messages are added after them.
@iequidoo iequidoo force-pushed the iequidoo/test_sync_sentbox_then_inbox branch from 5438610 to c8af189 Compare September 5, 2025 07:19
@iequidoo
Copy link
Collaborator Author

iequidoo commented Sep 5, 2025

Improved the commit message and added it to the PR description.

so this one now changes (and potentially complicates and slow down) sql statement?

This SQL query already looks at timestamp_sent, so i don't think that the additional condition slows it down a lot. A bigger problem with this SQL query is that it uses existing indexes ineffectively, in fact it uses index by chat_id, but if the chat has many messages, the query is slow. I'm going to optimize this in a separate PR then.

why not remove "warch sent folder" first, which is a clear way forward, and was agreed on. even if not does not catch all cases, it removes compexity and on previous discussions the issue at hand

Sure, this will help in the most cases and i'm going to do that probably after optimizing the mentioned SQL query. But simplifying/dropping SentboxWatch will be a bigger change and sorting of messages is better to be improved anyway to fix possible race conditions and even scenarios like sharing an account among several people or with a bot.

also state in the database is no longer the onr reported in the api?

This PR doesn't change anything here, but MessageId::get_state() already now doesn't just return the state from the db, e.g. OutMdnRcvd is only an API-level state, we don't update messages to it in the db anymore.

@r10s
Copy link
Contributor

r10s commented Sep 5, 2025

But simplifying/dropping SentboxWatch will be a bigger change

it is about dropping, and that is simple. most work is in adapting the python tests (i am not really into python, otherwise if would have done that already :)

my point is that subsequent things things are clearer to discuss if the noise of SentboxWatch (and ideally "upload to Sentbox via imap") is gone and we have some states less to take care for

@iequidoo
Copy link
Collaborator Author

iequidoo commented Sep 8, 2025

Not sure this should be merged, delayed incoming messages may be sorted above received outgoing messages and thus will be less visible. Will reopen if i have an idea how to fix this

@iequidoo iequidoo closed this Sep 8, 2025
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.

Wrong message order when watching sent folder
2 participants