Send user read receipts based on scroll position #86 #152
Conversation
|
PR for #86 |
| } | ||
|
|
||
| impl RoomScreen{ | ||
| fn send_user_read_receipts_based_on_scroll_pos(&mut self,cx:&mut Cx,actions: &Vec<Box<dyn ActionTrait + Send>>){ |
There was a problem hiding this comment.
can you refactor this function to not use 12 levels of indentation? It's quite difficult to read.
One way to do so is to use "early return" semantics where you check for a required condition and return from the function early if the condition is not met. For example, instead of wrapping the entire function in a conditional that checks if the portal list has not scrolled, you can just do this instead:
fn send_user_read_receipts_based_on_scroll_pos(...) {
if self.portal_list(id!(list)).scrolled(actions) {
return;
}
// then do the same thing to get the `tl_state` and `room_id`:
let Some(tl_state) = self.tl_state.as_mut() else {
return;
};
let Some(room_id) = self.room_id.as_ref() else {
return;
};
// then continue with the rest of the function here
}| if let Some(ref mut index) = tl_state.prev_first_index{ | ||
| // to detect change of scroll when scroll ends | ||
| if *index != first_index{ |
There was a problem hiding this comment.
The index value here can change arbitrarily upon receiving a TimelineUpdate::NewItems event. So, you'll also need to restart the 5-second timer when that happens in order to make sure that the event at prev_first_index hasn't changed during the 5-second timer interval.
| let time_now = std::time::Instant::now(); | ||
| if first_index> *index{ | ||
| // Implements sending read receipt when scrolling up to see bottom messages | ||
| for r in tl_state.content_drawn_since_last_update.clone(){ |
There was a problem hiding this comment.
Ah, i think you may misunderstand what content_drawn_since_last_update means. That variable tracks the set of items that have been drawn since the last background update of timeline content, which is used for the purpose of caching draw actions. It doesn't have anything to do with read receipts.
So I'm not sure why you're iterating over that set here, but I don't think it's relevant to read receipts. What were you trying to accomplish with this for loop?
| for (event_id_string,(room_id,event_id )) in &tl_state.marked_fully_read_queue{ | ||
| if &e.to_owned() == event_id{ | ||
| submit_async_request(MatrixRequest::FullyReadReceipt { room_id: room_id.clone(), event_id: event_id.clone(),message:msg.body().to_string().clone() }); | ||
| to_remove.push(event_id_string.clone()); | ||
| } | ||
| } | ||
| for r in to_remove{ | ||
| tl_state.marked_fully_read_queue.remove(&r); | ||
| } |
There was a problem hiding this comment.
The submit_async_request() procedure is fairly expensive (and somewhat "high latency"), as it requires pushing a request object onto a queue, and then waiting for tokio to schedule in a background async task to pop it off of the queue and handle it.
Thus, instead of calling that in a loop, you should create a single request that can contain multiple read receipts, and then call submit_async_request only once.
| ReadReceipt{ | ||
| room_id:OwnedRoomId, | ||
| event_id:OwnedEventId, | ||
| message:String, |
There was a problem hiding this comment.
you don't need to include the entire text of the event message here, there's no reason for that.
| FullyReadReceipt{ | ||
| room_id:OwnedRoomId, | ||
| event_id:OwnedEventId, | ||
| message:String, |
There was a problem hiding this comment.
you don't need to include the entire text of the event message here, there's no reason for that.
| log!("BUG: room info not found for send message request {room_id}"); | ||
| continue; | ||
| }; | ||
|
|
||
| room_info.timeline.clone() | ||
| }; | ||
| let _send_message_task = Handle::current().spawn(async move { | ||
| match timeline.send_single_receipt(ReceiptType::Read, ReceiptThread::Unthreaded, event_id.clone()).await { | ||
| Ok(_send_handle) => log!("Sent message to room {room_id}.read_marker {event_id} message {message}"), | ||
| Err(_e) => error!("Failed to send message to room {room_id}: {_e:?}"), | ||
| } | ||
| }); | ||
| }, | ||
| MatrixRequest::FullyReadReceipt { room_id,event_id,message }=>{ | ||
| let timeline = { | ||
| let mut all_room_info = ALL_ROOM_INFO.lock().unwrap(); | ||
| let Some(room_info) = all_room_info.get_mut(&room_id) else { | ||
| log!("BUG: room info not found for send message request {room_id}"); | ||
| continue; | ||
| }; | ||
|
|
||
| room_info.timeline.clone() | ||
| }; | ||
| let _send_message_task = Handle::current().spawn(async move { | ||
| let receipt = Receipts::new().fully_read_marker(event_id.clone()); | ||
| match timeline.send_multiple_receipts(receipt).await { | ||
| Ok(_send_handle) => log!("Sent message to room {room_id}.fully_read_marker {event_id} message {message}"), | ||
| Err(_e) => error!("Failed to send message to room {room_id}: {_e:?}"), |
There was a problem hiding this comment.
the logic here is correct, but can you update the log/error statements to be specific to sending read receipts and not sending messages? (I assume that's a copy-paste artifact based on copying this code structure from the SendMessage variant)
| // Queue to send fully read receipt | ||
| marked_fully_read_queue:HashMap<String,(OwnedRoomId,OwnedEventId)>, |
There was a problem hiding this comment.
Can you explain why a queue is necessary here? I don't quite understand the purpose of this... also, it's not a queue, right? It's just a set of events.
Instead of adding events to this marked_fully_read_queue and then iterating over it later, can't you just directly send the read receipt request from the point in the code where you're checking if the 5-second timer has elapsed? I'm confused by this two-part approach, which seems unnecessary. But I might be missing something, so please let me know what the purpose of this is.
Rework of #124