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.

Sign up for GitHub

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

Jump to bottom

Fixes to the sort timestamp of messages #5094

Merged
merged 3 commits into from
Dec 13, 2023
Merged

Fixes to the sort timestamp of messages #5094

merged 3 commits into from
Dec 13, 2023

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Dec 9, 2023

See commit messages.

There's no test yet, verified by hand only. Need to come up with more simple scenario than in #5088.

Fixes #5088

@iequidoo iequidoo requested review from Hocuri and link2xt December 9, 2023 03:10
@iequidoo iequidoo marked this pull request as ready for review December 9, 2023 03:36
Hocuri
Hocuri reviewed Dec 9, 2023
View reviewed changes
src/chat.rs Outdated Show resolved Hide resolved
src/chat.rs Show resolved Hide resolved
src/receive_imf.rs Outdated Show resolved Hide resolved
@iequidoo iequidoo force-pushed the iequidoo/sort-timestamp branch 2 times, most recently from e030237 to 3703628 Compare December 9, 2023 21:04
@iequidoo iequidoo requested a review from Hocuri December 9, 2023 21:10
@iequidoo iequidoo force-pushed the iequidoo/sort-timestamp branch 2 times, most recently from 8bcc866 to f1c8cd1 Compare December 11, 2023 22:43
@iequidoo
Copy link
Collaborator Author

Rebased and added fix: Add forgotten checks for Config::VerifiedOneOnOneChats

@iequidoo iequidoo force-pushed the iequidoo/sort-timestamp branch from f1c8cd1 to dade653 Compare December 12, 2023 00:13
Hocuri
Hocuri reviewed Dec 12, 2023
View reviewed changes
src/securejoin.rs Outdated Show resolved Hide resolved
@Hocuri
Copy link
Collaborator

Hocuri commented Dec 12, 2023

Any progress on trying to add a test?

@iequidoo iequidoo force-pushed the iequidoo/sort-timestamp branch from dade653 to c982425 Compare December 13, 2023 02:25
@iequidoo
fix: Always pass the correct sort timestamp to ChatId::set_protection…
42291bb 
…() (#5088)

Before in some places it was correctly calculated by passing the "sent" timestamp to
`calc_sort_timestamp()`, but in other places just the system time was used. In some complex
scenarios like #5088 (restoration of a backup made before a contact verification) it led to wrong
sort timestamps of protection messages and also messages following by them.

But to reduce number of args passed to functions needing to calculate the sort timestamp, add
message timestamps to `struct MimeMessage` which is anyway passed everywhere.
@iequidoo
fix: calc_sort_timestamp: Skip messages that mustn't affect sorting o…
fa4026d 
…f a new message (#5088)

Drafts mustn't affect sorting of any other messages, they aren't even displayed in the chat
window. Also hidden messages mustn't affect sorting of usual messages. But let hidden messages sort
together with protection messages because hidden messages also can be or not be verified, so let's
preserve this information -- even it's not useful currently, it can be useful in the future
versions.
@iequidoo
refactor: Move calc_{protection_msg_,}sort_timestamp() to impl ChatId
4689437
@iequidoo iequidoo force-pushed the iequidoo/sort-timestamp branch 2 times, most recently from c05d77f to 4689437 Compare December 13, 2023 02:44
@iequidoo
Copy link
Collaborator Author

Any progress on trying to add a test?

There's the same problem as in test_create_protected_grp_multidev() here. I tried to add a Rust test that checks that the protection message has the correct timestamp, but then i need to add large sleep()s to reproduce the bug with a sufficient probability, 3 secs as per my experiments. Otherwise the test has no sense -- it would always pass.

Another option is to add a Python test, but as we have less control over sent and received messages there, we can't write a test just checking a protection message timestamp and need to write some more high-level test, probably repeating the original @gerryfrancis's scenario, but it would be more complex and cumbersome than other tests we have now.

The solution i can imagine here is to mock the system clock for the Rust tests, then we can write any tests checking timestamps of messages w/o adding unnecessary sleep()s.

@iequidoo iequidoo requested a review from Hocuri December 13, 2023 03:05
@iequidoo iequidoo mentioned this pull request Dec 13, 2023
Merged
Hocuri
Hocuri approved these changes Dec 13, 2023
View reviewed changes
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.

Nice stuff! Sad about the missing test, maybe we can add a way to mock the time at some point

@iequidoo iequidoo merged commit 62c1237 into main Dec 13, 2023
38 checks passed
@iequidoo iequidoo deleted the iequidoo/sort-timestamp branch December 13, 2023 23:29
@iequidoo iequidoo mentioned this pull request Dec 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hocuri Hocuri Hocuri approved these changes

@link2xt link2xt Awaiting requested review from link2xt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@iequidoo @Hocuri