-
Notifications
You must be signed in to change notification settings - Fork 252
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(timeline): introduce the TimelineUniqueId
#4111
Conversation
f0af335
to
cf8b97b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4111 +/- ##
==========================================
- Coverage 84.72% 84.72% -0.01%
==========================================
Files 269 269
Lines 28779 28776 -3
==========================================
- Hits 24384 24381 -3
Misses 4395 4395 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. There's a bunch of changes we will need to make on the iOS side but I think it's about time, they're welcome.
See #4100 (comment). From this PR we can keep the first commit (don't try to reuse unique IDs) + the opaque type, and we'll get rid of the third one likely. It might be better to not use the transaction unique id in the react function first. |
aca7286
to
fa489bb
Compare
Ready for another round of review, after/on-top-of #4127 (but since you're the reviewer, you choose to decide how you review all this :)). Thanks! |
…stall IDs across timeline clears
…r the unique identifier We can now use this type instead of passing a string, which means there's no way to confuse oneself in methods like `toggle_reaction_local`. Changelog: Introduced `TimelineUniqueId`, returned by `TimelineItem::unique_id()` and serving as an opaque identifier to use in other methods modifying the timeline item (e.g. `toggle_reaction`).
fa489bb
to
6f3d880
Compare
… use the new `TimelineUniqueId` type instead of `String` for unique timeline identifiers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tried it out in on this branch and on top of 4127 and everything looks good to me. Nicely done 👍
* Adopt new reaction toggling API introduced in matrix-org/matrix-rust-sdk/pull/4127 * Adopt the changes introduced in matrix-org/matrix-rust-sdk/pull/4111: use the new `TimelineUniqueId` type instead of `String` for unique timeline identifiers. * Bump the RustSDK to v1.0.58. * Fix unit tests
See commit by commit description.
Extracted from #4100.