refactor(timeline): fuse `edit_by_id` (resp `redact_by_id`) into their ID-less counterparts #4100

bnjbvr · 2024-10-09T14:12:47Z

This PR sits on top of #4111 and #4127, and allows us to get rid of the edit_by_id / redact_by_id variants.

I've added changelog entries \o/

codecov · 2024-10-09T14:28:13Z

Codecov Report

Attention: Patch coverage is 87.50000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.73%. Comparing base (a901506) to head (5646e1c).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-ui/src/timeline/mod.rs	87.50%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4100      +/-   ##
==========================================
- Coverage   84.76%   84.73%   -0.03%     
==========================================
  Files         269      269              
  Lines       28848    28813      -35     
==========================================
- Hits        24453    24415      -38     
- Misses       4395     4398       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jmartinesp

Thanks for the work on this! I have a few questions, especially about how we return this unique id.

I'll also try to build a version and test it against Android to see how it works.

jmartinesp · 2024-10-09T14:29:13Z

crates/matrix-sdk-ui/src/timeline/item.rs

+/// represented by the item.
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "uniffi", derive(uniffi::Object))]
+pub struct TimelineUniqueId(pub(crate) String);


I guess this is created as an Object instead of Record so it can't be instantiated from the clients? I'd rather have a plain type instead of passing a pointer as it would be better for debugging purposes though 🫤 .

I suppose Record means the inner string would be available to all, and the intent here is that the identifier is an opaque thingy. Would you like a "gimme a debug string" helper for debugging purposes?

I think that would be enough, yes. Without an event id or transaction id it could be a bit difficult to keep track of an item if it changed while debugging, with the event id back this should be better and having a way to manually check the unique id would be perfect. Thanks!

jmartinesp · 2024-10-09T14:39:23Z

crates/matrix-sdk-ui/src/timeline/mod.rs

-        match event.handle() {
+        let items = &self.controller.items().await;
+        let Some((_item_pos, item)) = rfind_event_by_uid(&items, unique_id) else {
+            warn!("couldn't find timeline item identified with id");


Having a bool indicating whether the event was redacted or not seemed weird when I was looking at this code, same as when editing: if I ask theSDK to do an operation and it can't, I'd expect the issue to be raised as an error instead of a false value, but maybe that's because I'm used to the java way of catching exceptions for unexpected behavior.

I'm not closed to changing the API here; I'd rather have it be a different PR though, ideally with an issue explaining the motivation.

jmartinesp · 2024-10-09T14:42:38Z

bindings/matrix-sdk-ffi/src/timeline/mod.rs

        Self {
-            is_local: value.is_local_echo(),
-            is_remote: !value.is_local_echo(),
-            event_or_transaction_id: value.identifier().into(),


How do we get a unique id without this? Did I miss where we return it?

Ah, I see, it's here.

But to create a ComposerDraftType we'll still need a way to get the related event id, at least.

Added it back in a fixup commit autosquashed.

jmartinesp · 2024-10-10T08:56:49Z

Some things I found while trying to integrate the changes:

Sometimes we add virtual events of our own to the timeline, so those need a 'fake' timeline id too to reuse the same models. Since we already wrap the TimelineUniqueId in a wrapper of our own to be able to create fakes for testing too, this is not that bad for Android, but I don't know how difficult it would be for iOS (@stefanceriu ?).
For the forward action in the clients (as in, taking the content of an event and sending it in another event, maybe in a different room) we need to get this message like content first, and until now we were just passing the associated event id, fetching the content using timeline.getEventTimelineItemByEventId at the last moment, then sending a new message with the contents using the new timeline. However, there is no timeline.getTimelineItemByUniqueId exposed so the only way we could do this now is by getting the content at the point where the action starts and passing it around several screens, which is not impossible on Android, but it is difficult. Having the timeline.getTimelineItemByUniqueId method would be super helpful.
This forces us to remove some APIs from our Room object wrappers, but I think that's fine on most cases since I don't think they were being used anymore and also they relied on the internal timeline implementation, so we should have used the timeline version anyway.
There is a fn Room::try_resend(&self, transaction_id: &TransactionId) which will probably not work anymore since we don't get the transaction id at any point.
I can't pass a TimelineUniqueId between Android screens because it needs to be Parcelable and for that I need access to the actual value inside it. This is needed when opening the 'edit poll' screen. It also means we won't be able to pass timeline events between screens either.

I'm still trying to adapt our current code to the new APIs, I'll post any other issues I run into.

bnjbvr · 2024-10-10T10:09:33Z

@jmartinesp Thanks for the feedback. Latest commits should address most of your comments, please let me know if anything's missing.

stefanceriu

Took me 2 hours to get the app building again but I finally did it. You can see what it took here: element-hq/element-x-ios@develop...stefan/TimelineUniqueId

I found problems in:

methods for retring using txnids (room.tryResend, room.ignoreDeviceTrustAndResend, room.withdrawVerificationAndResend)
message drafts
voice message audio players: we need to keep their states in between timeline resets/updates. The IDs need to be stable.
sending read receipts
editing polls
ending polls
selecting poll options
copying permalinks (tell if it's been sent, use its event id)
replying
pinning, unpinning message and viewing them in the timeline. Checking if a particular timeline item is pinned.
reporting content
composer states (editing, replying)
focusing a timeline on a particular event id
fetching event details through fetchDetailsForEvent(eventId)

While some of them can probably be rewritten some other way even now I don't think this PR considers all of them.

I'm all up for improving APIs but introducing so many changes at the same time is a sure recipe for regressions.

bnjbvr · 2024-10-10T11:52:06Z

@stefanceriu Thanks for trying it out. I'm not sure how I'm supposed to guess what you need based on skimming the list of "problems", since they don't mention any particular APIs or precise issues, except for the voice message item? (The first bullet item should've been addressed, considering I've added back event_id/transaction_id in the EventTimelineItem.)

jmartinesp · 2024-10-10T11:56:37Z

bindings/matrix-sdk-ffi/src/timeline/mod.rs

+    event_id: Option<String>,
+    /// A transaction id, for events waiting in the send queue.
+    ///
+    /// Note: not set for remote-echoes.
+    transaction_id: Option<String>,


This sounds a lot like EventOrTransactionId to me.

We're already losing much static typing safety by using Strings everywhere, maybe we can afford two types to help distinguish one from the other 🥲 Also, one can't use an EventOrTransactionId in any API, it's either an event id OR a transaction id, so using any of those APIs would require passing the event|transaction id and pray your best luck/favorite god that it doesn't fail because it wasn't the expected type.

poljar · 2024-10-10T12:57:55Z

@stefanceriu Thanks for trying it out. I'm not sure how I'm supposed to guess what you need based on skimming the list of "problems", since they don't mention any particular APIs or precise issues, except for the voice message item? (The first bullet item should've been addressed, considering I've added back event_id/transaction_id in the EventTimelineItem.)

Can we perhaps split this PR out into smaller chunks, that would perhaps make it easier to see what introduces certain regressions.

stefanceriu · 2024-10-10T13:10:59Z

I'm not sure how I'm supposed to guess what you need based on skimming the list of "problems", since they don't mention any particular APIs or precise issues

@bnjbvr Oh I'm sorry, I just assumed you know how these things are used.
Try for example to search for event_id on the ffi layer timeline

I would suggest you introduce that TimelineUniqueId next to EventOrTransactionId and slowly migrate away from the latter, method by method, feature by feature. It might be a good idea to get a feel for how these APIs are used in the final clients.

bnjbvr · 2024-10-15T10:39:44Z

We've discussed about it offline, and we think using the event or transaction id instead of the TransactionUniqueId in more places is the right thing to do:

transaction unique IDs aren't stable across timeline resets (when receiving a gappy timeline from the server)
transaction unique IDs aren't stable across timeline instances (e.g. when there's a permalink-focused and a live timelines living at the same time)

So the plan is:

to use transaction-or-event-id in more places (i.e. not use EventTimelineItem or TransactionUniqueId in APIs for edit/react/redact)
get rid of the duplicated APIs at the SDK level
???
fun and profits

stefanceriu

With the other 2 PRs now in place this is looking very nice! 👏 (bar those inline comment of mine).

bindings/matrix-sdk-ffi/src/timeline/mod.rs

stefanceriu

Love it! 👏

Changelog: `Timeline::redact_by_id` has been fused into `Timeline::redact`, which now takes a `TimelineEventItemId` as an identifier of the item (local or remote) to redact.

In particular, this means that trying to edit an event that's not present anymore in a timeline (e.g. after a timeline reset) will fail, while it worked before. Changelog: `Timeline::edit_by_id` has been fused into `Timeline::edit`, which now takes a `TimelineEventItemId` as the identifier for the local or remote item to edit. This also means that editing an event that's not in the timeline anymore will now fail. Callers should manually create the edit event's content, and then send it via the send queue; which the FFI function `Room::edit` does.

…ouldn't be found in the timeline This maintains functionality we had prior to the previous commit: if an event's missing from the timeline (e.g. timeline's been cleared after a gappy sync response), then still allow editing it.

…al echo disappeared I think this can't happen, but the send queue can return an error if a local echo identified by a transaction id doesn't exist anymore in the database. The only reason the latter could happen is because the local echo has been sent, in which case an update to the timeline would be dispatched, and the timeline item would have morphed into a remote echo in the meantime. So it's really rare that this would happen, and the `Timeline::redact()` method doesn't have to return a boolean to indicate success in general. Changelog: `Timeline::redact()` doesn't return a boolean; previously, it would only return false if the internal state was invalid, so a new error `RedactError::InvalidLocalEchoState` has been introduced to represent that.

See previous commit for explanations. This makes for a simpler API anyways. Changelog: `Timeline::edit` doesn't return a bool anymore to indicate it couldn't manage the edit in some cases, but will return errors indicating what the cause of the error is.

bnjbvr requested a review from a team as a code owner October 9, 2024 14:12

bnjbvr requested review from stefanceriu and removed request for a team October 9, 2024 14:12

jmartinesp reviewed Oct 9, 2024

View reviewed changes

bnjbvr force-pushed the bnjbvr/unique-id branch from 812e128 to 5f59ead Compare October 9, 2024 15:24

bnjbvr mentioned this pull request Oct 9, 2024

Use UniqueId as a parameter for timeline APIs #4093

Closed

stefanceriu added a commit to element-hq/element-x-ios that referenced this pull request Oct 10, 2024

See what it takes to adopt matrix-org/matrix-rust-sdk/pull/4100

2daa506

stefanceriu reviewed Oct 10, 2024

View reviewed changes

jmartinesp reviewed Oct 10, 2024

View reviewed changes

bnjbvr force-pushed the bnjbvr/unique-id branch from 7ba240c to cceea94 Compare October 10, 2024 12:04

This was referenced Oct 10, 2024

refactor(ffi): remove two fields in EventTimelineItem #4110

Merged

feat(timeline): introduce the TimelineUniqueId #4111

Merged

bnjbvr force-pushed the bnjbvr/unique-id branch from cceea94 to 998ddcf Compare October 15, 2024 15:18

bnjbvr marked this pull request as draft October 15, 2024 15:18

bnjbvr changed the title ~~refactor(timeline): use TimelineUniqueId for both edit and redact~~ refactor(timeline): fuse edit_by_id (resp redact_by_id) into their ID-less counterparts Oct 15, 2024

bnjbvr mentioned this pull request Oct 15, 2024

refactor(ffi): get rid of the get_event_timeline_item by id methods #4130

Merged

bnjbvr force-pushed the bnjbvr/unique-id branch from ac9b551 to 0490df2 Compare October 16, 2024 16:53

bnjbvr marked this pull request as ready for review October 16, 2024 16:53

stefanceriu requested changes Oct 17, 2024

View reviewed changes

bindings/matrix-sdk-ffi/src/timeline/mod.rs Show resolved Hide resolved

bindings/matrix-sdk-ffi/src/timeline/mod.rs Outdated Show resolved Hide resolved

bindings/matrix-sdk-ffi/src/timeline/mod.rs Outdated Show resolved Hide resolved

bnjbvr requested a review from stefanceriu October 17, 2024 10:15

stefanceriu approved these changes Oct 17, 2024

View reviewed changes

bnjbvr added 2 commits October 17, 2024 14:41

refactor(timeline): fuse redact_by_id() within redact()

8bae930

Changelog: `Timeline::redact_by_id` has been fused into `Timeline::redact`, which now takes a `TimelineEventItemId` as an identifier of the item (local or remote) to redact.

bnjbvr added 3 commits October 17, 2024 14:41

bnjbvr force-pushed the bnjbvr/unique-id branch from 1ded097 to 5646e1c Compare October 17, 2024 12:41

bnjbvr enabled auto-merge (rebase) October 17, 2024 12:41

bnjbvr merged commit 821fa8f into main Oct 17, 2024
40 checks passed

bnjbvr deleted the bnjbvr/unique-id branch October 17, 2024 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(timeline): fuse `edit_by_id` (resp `redact_by_id`) into their ID-less counterparts #4100

refactor(timeline): fuse `edit_by_id` (resp `redact_by_id`) into their ID-less counterparts #4100

bnjbvr commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 9, 2024 •

edited

Loading

jmartinesp left a comment

jmartinesp Oct 9, 2024

bnjbvr Oct 9, 2024

jmartinesp Oct 9, 2024

jmartinesp Oct 9, 2024

bnjbvr Oct 9, 2024

jmartinesp Oct 9, 2024

jmartinesp Oct 9, 2024

jmartinesp Oct 9, 2024

bnjbvr Oct 9, 2024

jmartinesp commented Oct 10, 2024 •

edited

Loading

bnjbvr commented Oct 10, 2024

stefanceriu left a comment

bnjbvr commented Oct 10, 2024

jmartinesp Oct 10, 2024

bnjbvr Oct 10, 2024

poljar commented Oct 10, 2024

stefanceriu commented Oct 10, 2024

bnjbvr commented Oct 15, 2024

stefanceriu left a comment

stefanceriu left a comment

refactor(timeline): fuse edit_by_id (resp redact_by_id) into their ID-less counterparts #4100

refactor(timeline): fuse edit_by_id (resp redact_by_id) into their ID-less counterparts #4100

Conversation

bnjbvr commented Oct 9, 2024 • edited Loading

codecov bot commented Oct 9, 2024 • edited Loading

Codecov Report

jmartinesp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Oct 10, 2024 • edited Loading

bnjbvr commented Oct 10, 2024

stefanceriu left a comment

Choose a reason for hiding this comment

bnjbvr commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poljar commented Oct 10, 2024

stefanceriu commented Oct 10, 2024

bnjbvr commented Oct 15, 2024

stefanceriu left a comment

Choose a reason for hiding this comment

stefanceriu left a comment

Choose a reason for hiding this comment

refactor(timeline): fuse `edit_by_id` (resp `redact_by_id`) into their ID-less counterparts #4100

refactor(timeline): fuse `edit_by_id` (resp `redact_by_id`) into their ID-less counterparts #4100

bnjbvr commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 9, 2024 •

edited

Loading

jmartinesp commented Oct 10, 2024 •

edited

Loading