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: Fix info-message orderings of verified 1:1 chats #4545

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

Hocuri
Copy link
Collaborator

@Hocuri Hocuri commented Jul 14, 2023

Correctly handle messages with old timestamps for verified chats:

  • They must not be sorted over a protection-changed info message
  • If they change the protection, then they must not be sorted over existing other messages, because then the protection-changed info message would also be above these existing messages.

This PR fixes this:

  1. Even seen messages can't be sorted into already-noticed messages anymore. This also changes DC's behavior in the absence of verified 1:1 chats. Before this PR, messages that are marked as seen when they are downloaded will always be sorted by their timestamp, even if it's very old.

  2. protection-changed info messages are always sorted to the bottom.

    Edit:

  3. There is an exception to rule 1: Outgoing messages are still allowed to be sorted purely by their timestamp, and don't influence old messages. This is to the problem described at [*].

Together, these rules also make sure that the protection-changed info message is always right above the message causing the change.

[*] If we receive messages from two different folders, e.g. Sent and Inbox, then this will lead to wrong message ordering in many cases. I need to think about this more, or maybe someone else has an idea. One alternative that came to my mind is:

  • Always sort noticed messages under the newest info message (this PR sorts them under the newest noticed message, master sorts them purely by their sent timestamp)
  • Always sort unnoticed messages under the newest noticed message (that's the same behavior as in this PR and on master)
  • Always sort protection-changed info messages to the bottom (as in this PR)

However, after a talk with @link2xt we instead decided to add rule 3. (see above) because it seemed a little bit easier.

@Hocuri Hocuri marked this pull request as draft July 14, 2023 11:10
@Hocuri Hocuri force-pushed the hoc/fix-verifiedchats-info-msg-orderings branch from e1c705e to 76565e6 Compare July 14, 2023 11:23
@link2xt
Copy link
Collaborator

link2xt commented Jul 14, 2023

Python lint is already fixed on stable, I just merged stable into master so this PR only needs a rebase.

@Hocuri Hocuri force-pushed the hoc/fix-verifiedchats-info-msg-orderings branch from 76565e6 to 17a3791 Compare July 14, 2023 12:20
@Hocuri Hocuri marked this pull request as ready for review July 17, 2023 13:27
@Hocuri Hocuri requested a review from link2xt July 17, 2023 13:27
@@ -758,7 +758,8 @@ async fn add_parts(
// message is `MessageState::InNoticed`, which means that all following
// messages are sorted under it.
let sort_timestamp =
calc_sort_timestamp(context, sent_timestamp, chat_id, true).await?;
calc_sort_timestamp(context, sent_timestamp, chat_id, true, incoming)
Copy link
Collaborator

Choose a reason for hiding this comment

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

incoming == true here, btw. Also i'd suggest to comment passed bool values or use enums instead, bool flags are not well-read

Copy link
Collaborator Author

@Hocuri Hocuri Jul 20, 2023

Choose a reason for hiding this comment

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

incoming == true here, btw

Is this just something you noticed, or you suggest replacing it with true?

Also i'd suggest to comment passed bool values or use enums instead, bool flags are not well-read

For me, it'd be as quick to jump to the method definition and seeing what the parameter does as finding the comment that belongs to it and reading it. An enum would be nicer to read, but seemed overkill for me for a function that's used just two times.

That being said, it's not important to me to keep it as-is, so I can also change it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest to replace it with smth like /* incoming = */ true, otherwise it can be read by mistake as a common code for incoming and outgoing messages. The same for always_sort_to_bottom, no long comments are needed. I wonder why Rust has named struct members initialisation, but no named function args.
Anyway it's minor, so up to you

@Hocuri Hocuri merged commit 682e241 into master Jul 24, 2023
36 checks passed
@Hocuri Hocuri deleted the hoc/fix-verifiedchats-info-msg-orderings branch July 24, 2023 10:16
@link2xt link2xt mentioned this pull request Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants