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

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Jul 29, 2024

See commit messages.

My devices go online/offline independently and i use both to send messages, so often i see my messages wrongly ordered around incoming messages.

@iequidoo
Copy link
Collaborator Author

iequidoo commented Oct 12, 2024

The test i added here isn't needed, there's test_old_message_5() already which does the same, but instead records the current (wrong) behaviour. Going to fix it then. EDIT: Done

@iequidoo iequidoo force-pushed the iequidoo/test_sync_sentbox_then_inbox branch from 4ea6d9d to ce4be1d Compare October 12, 2024 01:07
@iequidoo iequidoo changed the title test: Order of messages if Sentbox is synced before Inbox fix: Order of messages if Sentbox is synced before Inbox Oct 12, 2024
@iequidoo iequidoo changed the base branch from main to iequidoo/sort-rcvd-outgoing-msgs-down October 12, 2024 01:09
@iequidoo iequidoo marked this pull request as ready for review October 12, 2024 01:23
@iequidoo iequidoo requested a review from link2xt October 12, 2024 01:24
@iequidoo iequidoo force-pushed the iequidoo/sort-rcvd-outgoing-msgs-down branch 4 times, most recently from 3214eb6 to 6c01309 Compare October 14, 2024 23:49
Base automatically changed from iequidoo/sort-rcvd-outgoing-msgs-down to main October 15, 2024 00:22
@iequidoo iequidoo force-pushed the iequidoo/test_sync_sentbox_then_inbox branch from ce4be1d to da4a84e Compare October 15, 2024 00:27
@iequidoo
Copy link
Collaborator Author

Btw, i think it may be useful to display these outgoing received messages (i.e. from other MUAs/devices) somehow differently e.g. align them to the left as incoming ones. As they still have another brightness (even in Dark mode), it shouldn't be a problem to differ them from incoming messages. May be even useful for shared/community accounts, not sure. CC @adbenitez

@iequidoo
Copy link
Collaborator Author

iequidoo commented Oct 25, 2024

Recently i started to see my messages weirdly ordered around incoming ones every day when i open my laptop lid (i.e. i see my replies above incoming messages). I don't know whether Delta Chat started scheduling smth differently or i changed its usage pattern, but this PR should help it seems.

@iequidoo iequidoo requested a review from Hocuri October 25, 2024 22:41
Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

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

The PR description is quite short, the PR is easier to understand if the PR description has a short explanation in which way the logic was changed.

Btw, i think it may be useful to display these outgoing received messages (i.e. from other MUAs/devices) somehow differently e.g. align them to the left as incoming ones. As they still have another brightness (even in Dark mode), it shouldn't be a problem to differ them from incoming messages.

I'm pretty sure that non-technical users would find this super confusing.

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.

@iequidoo iequidoo force-pushed the iequidoo/test_sync_sentbox_then_inbox branch from da4a84e to c564e75 Compare October 28, 2024 17:10
@iequidoo iequidoo marked this pull request as draft October 28, 2024 17:13
@iequidoo iequidoo force-pushed the iequidoo/test_sync_sentbox_then_inbox branch 2 times, most recently from 91eaf72 to 8bbee4a Compare October 28, 2024 17:23
@iequidoo
Copy link
Collaborator Author

The PR description is quite short, the PR is easier to understand if the PR description has a short explanation in which way the logic was changed.

I put everything to commit messages to avoid updating the PR description every time they change. Probably it's better to leave the PR description empty then or just reference the commit messages.

Btw, i think it may be useful to display these outgoing received messages (i.e. from other MUAs/devices) somehow differently e.g. align them to the left as incoming ones. As they still have another brightness (even in Dark mode), it shouldn't be a problem to differ them from incoming messages.

I'm pretty sure that non-technical users would find this super confusing.

Probably yes, though for shared/community accounts this may be useful, but then also we need to check for presence of override_sender_name to avoid confusion for other users. Anyway currently in the message Info the user can check if it's a sent or received outgoing message, so my proposal doesn't add smth new, this is only a question of usability.

@iequidoo iequidoo marked this pull request as ready for review October 28, 2024 17:44
@iequidoo iequidoo requested a review from Hocuri October 28, 2024 17:45
This fixes a Gmail-like scenario when outgoing messages are saved to Sentbox as well and it is
fetched before Inbox by chance, by adding a new `OutRcvd` message state for received outgoing
messages so that they can mingle with fresh incoming ones and appear after them in chats. As for
`OutPending`, `OutDelivered` etc. messages, they are sent locally by the user and there's no need to
make them more noticeable even if they are newer.

All APIs still return `OutDelivered` instead of `OutRcvd` for compatibility.
@iequidoo iequidoo force-pushed the iequidoo/test_sync_sentbox_then_inbox branch from 8bbee4a to 3470b26 Compare October 29, 2024 22:23
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