From ceceba732dc5a77efaa11f44edc0bf2fc5d80cd0 Mon Sep 17 00:00:00 2001 From: Kevin Boos Date: Wed, 14 Aug 2024 16:35:37 -0700 Subject: [PATCH 1/2] fix avatar loading to ensure available avatar images are always used. Improve (reduce) timeline jumping when timeline items are changed via background updates. I think there are still some improvements we can make, e.g., for complete clears of the timeline (after (un)ignore user events). --- resources/icons/back.svg | 4 -- src/avatar_cache.rs | 2 +- src/home/room_screen.rs | 108 ++++++++++++++++++++++++------ src/profile/user_profile.rs | 13 +++- src/profile/user_profile_cache.rs | 2 +- 5 files changed, 101 insertions(+), 28 deletions(-) delete mode 100644 resources/icons/back.svg diff --git a/resources/icons/back.svg b/resources/icons/back.svg deleted file mode 100644 index 235235b9..00000000 --- a/resources/icons/back.svg +++ /dev/null @@ -1,4 +0,0 @@ - - - - \ No newline at end of file diff --git a/src/avatar_cache.rs b/src/avatar_cache.rs index 9dea89d7..98772688 100644 --- a/src/avatar_cache.rs +++ b/src/avatar_cache.rs @@ -3,7 +3,7 @@ use crossbeam_queue::SegQueue; use makepad_widgets::{Cx, SignalToUI}; use matrix_sdk::ruma::{MxcUri, OwnedMxcUri}; -use crate::{shared::avatar::Avatar, sliding_sync::{submit_async_request, MatrixRequest}}; +use crate::sliding_sync::{submit_async_request, MatrixRequest}; thread_local! { diff --git a/src/home/room_screen.rs b/src/home/room_screen.rs index ccfb8bc8..3056c076 100644 --- a/src/home/room_screen.rs +++ b/src/home/room_screen.rs @@ -1,17 +1,17 @@ //! A room screen is the UI page that displays a single Room's timeline of events/messages //! along with a message input bar at the bottom. -use std::{borrow::Cow, collections::BTreeMap, ops::{DerefMut, Range}, sync::{Arc, Mutex}}; +use std::{borrow::Cow, collections::BTreeMap, ops::{Deref, DerefMut, Range}, sync::{Arc, Mutex}}; use imbl::Vector; use makepad_widgets::*; use matrix_sdk::{ruma::{ events::{ room::{ - avatar, guest_access::GuestAccess, history_visibility::HistoryVisibility, join_rules::JoinRule, message::{MessageFormat, MessageType, RoomMessageEventContent}, MediaSource + guest_access::GuestAccess, history_visibility::HistoryVisibility, join_rules::JoinRule, message::{MessageFormat, MessageType, RoomMessageEventContent}, MediaSource }, AnySyncMessageLikeEvent, AnySyncTimelineEvent, FullStateEventContent, SyncMessageLikeEvent, - }, matrix_uri::MatrixId, uint, MatrixToUri, MatrixUri, MilliSecondsSinceUnixEpoch, OwnedRoomId, RoomId, + }, matrix_uri::MatrixId, uint, MatrixToUri, MatrixUri, MilliSecondsSinceUnixEpoch, OwnedEventId, OwnedRoomId, RoomId }, OwnedServerName}; use matrix_sdk_ui::timeline::{ self, AnyOtherFullStateEventContent, EventTimelineItem, MemberProfileChange, MembershipChange, ReactionsByKeyBySender, RoomMembershipChange, TimelineDetails, TimelineItem, TimelineItemContent, TimelineItemKind, VirtualTimelineItem @@ -767,6 +767,8 @@ struct TimelineUiState { /// Whether this room's timeline has been fully paginated, which means /// that the oldest (first) event in the timeline is locally synced and available. /// When `true`, further backwards pagination requests will not be sent. + /// + /// This must be reset to `false` whenever the timeline is fully cleared. fully_paginated: bool, /// The list of items (events) in this room's timeline that our client currently knows about. @@ -813,10 +815,16 @@ struct TimelineUiState { /// and restored when navigating back to a timeline (upon `Show`). #[derive(Default, Debug)] struct SavedState { - /// The ID of the first item in the timeline's PortalList that is currently visible. - /// - /// TODO: expose scroll position from PortalList and use that instead, which is more accurate. - first_id: usize, + /// The index of the first item in the timeline's PortalList that is currently visible, + /// and the scroll offset from the top of the list's viewport to the beginning of that item. + first_index_and_scroll: Option<(usize, f64)>, + /// The unique ID of the event that corresponds to the first item visible in the timeline. + first_event_id: Option, + + /// The content of the message input box. + draft: Option, + /// The position of the cursor head and tail in the message input box. + cursor: (usize, usize), } impl Timeline { @@ -901,8 +909,20 @@ impl Timeline { log!("Timeline::save_state(): skipping due to missing state, room {:?}", self.room_id); return; }; - let first_id = self.portal_list(id!(list)).first_id(); - tl.saved_state.first_id = first_id; + let portal_list = self.portal_list(id!(list)); + let first_index = portal_list.first_id(); + tl.saved_state.first_index_and_scroll = Some(( + first_index, + portal_list.scroll_position(), + )); + tl.saved_state.first_event_id = tl.items + .get(first_index) + .and_then(|item| item + .as_event() + .and_then(|ev| ev.event_id().map(|i| i.to_owned())) + ); + + // Store this Timeline's `TimelineUiState` in the global map of states. TIMELINE_STATES.lock().unwrap().insert(tl.room_id.clone(), tl); } @@ -912,8 +932,12 @@ impl Timeline { /// Note: this accepts a direct reference to the timeline's UI state, /// so this function must not try to re-obtain it by accessing `self.tl_state`. fn restore_state(&mut self, tl_state: &TimelineUiState) { - let first_id = tl_state.saved_state.first_id; - self.portal_list(id!(list)).set_first_id(first_id); + if let Some((first_index, scroll_from_first_id)) = tl_state.saved_state.first_index_and_scroll { + self.portal_list(id!(list)) + .set_first_id_and_scroll(first_index, scroll_from_first_id); + } + + // TODO: restore the message input box's draft text and cursor head/tail positions. } } @@ -973,26 +997,50 @@ impl Widget for Timeline { // that its timeline events have been updated in the background. if let Event::Signal = event { let portal_list = self.portal_list(id!(list)); - let orig_first_id = portal_list.first_id(); let Some(tl) = self.tl_state.as_mut() else { return }; let mut done_loading = false; while let Ok(update) = tl.update_receiver.try_recv() { match update { TimelineUpdate::NewItems { items, changed_indices, clear_cache } => { - // Determine which item is currently visible the top of the screen + // TODO: we can often avoid the following loops that iterate ovoer the `items` list + // by only doing that if `clear_cache` is true, or if `changed_indices` range includes + // any index that comes before (is less than) the above `orig_first_id`. + + let orig_first_id = portal_list.first_id(); + let scroll_from_first_id = portal_list.scroll_position(); + + // Determine which item is currently visible the top of the screen (the first event) // so that we can jump back to that position instantly after applying this update. - if let Some(top_event_id) = tl.items.get(orig_first_id).map(|item| item.unique_id()) { + let current_first_event_id_opt = tl.items + .get(orig_first_id) + .and_then(|item| item.as_event() + .and_then(|ev| ev.event_id().map(|i| i.to_owned())) + ); + + log!("current_first_event_id_opt: {current_first_event_id_opt:?}, orig_first_id: {orig_first_id}"); + if let Some(top_event_id) = current_first_event_id_opt.as_ref() { for (idx, item) in items.iter().enumerate() { - if item.unique_id() == top_event_id { + let Some(item_event_id) = item.as_event().and_then(|ev| ev.event_id()) else { + continue + }; + if top_event_id.deref() == item_event_id { if orig_first_id != idx { - log!("Timeline::handle_event(): jumping view from top event index {orig_first_id} to index {idx}"); - portal_list.set_first_id(idx); + log!("Timeline::handle_event(): jumping view from top event index {orig_first_id} to new index {idx}"); + portal_list.set_first_id_and_scroll(idx, scroll_from_first_id); } break; + } else if tl.saved_state.first_event_id.as_deref() == Some(item_event_id) { + // TODO: should we only do this if `clear_cache` is true? (e.g., after an (un)ignore event) + log!("!!!!!!!!!!!!!!!!!!!!!!! Timeline::handle_event(): jumping view from saved first event ID to index {idx}"); + portal_list.set_first_id_and_scroll(idx, scroll_from_first_id); + break; } } + } else { + warning!("Couldn't get unique event ID for event at the top of room {:?}", tl.room_id); } + if clear_cache { tl.content_drawn_since_last_update.clear(); tl.profile_drawn_since_last_update.clear(); @@ -1050,9 +1098,9 @@ impl Widget for Timeline { let last_item_id = last_item_id + 1; // Add 1 for the TopSpace. // Start the actual drawing procedure. - while let Some(list_item) = self.view.draw_walk(cx, scope, walk).step() { + while let Some(subview) = self.view.draw_walk(cx, scope, walk).step() { // We only care about drawing the portal list. - let portal_list_ref = list_item.as_portal_list(); + let portal_list_ref = subview.as_portal_list(); let Some(mut list_ref) = portal_list_ref.borrow_mut() else { continue }; let list = list_ref.deref_mut(); @@ -1164,6 +1212,28 @@ impl Widget for Timeline { item.draw_all(cx, &mut Scope::empty()); } } + + + // Note: we shouldn't need to save any states here, as the `TimelineUpdate::NewItems` event handler + // will be able to query the event ID of the first/top item in the timeline + // **BEFORE** it actually applies the new items to the timeline's TimelineUiState. + + /* + let first_index = portal_list.first_id(); + let scroll_from_first_id = portal_list.scroll_position(); + + // TODO: the PortalList doesn't support this yet, but we should get the scroll positions + // of other nearby item IDs as well, in case the first item ID corresponds to + // a virtual event or an event that doesn't have a valid `event_id()`, + // such that we can jump back to the same relative position in the timeline after an update. + let first_event_id = tl_items + .get(first_index) + .and_then(|item| item.as_event() + .and_then(|ev| ev.event_id().map(|i| i.to_owned())) + ); + tl_state.saved_state.first_event_id = first_event_id; + */ + DrawStep::done() } } diff --git a/src/profile/user_profile.rs b/src/profile/user_profile.rs index fe337748..62666527 100644 --- a/src/profile/user_profile.rs +++ b/src/profile/user_profile.rs @@ -5,7 +5,7 @@ use crate::{ avatar_cache::{self, AvatarCacheEntry}, shared::avatar::AvatarWidgetExt, sliding_sync::{get_client, is_user_ignored, submit_async_request, MatrixRequest}, utils }; -use super::user_profile_cache::{self, get_user_profile_and_room_members, get_user_room_member_info}; +use super::user_profile_cache::{self, get_user_profile_and_room_member, get_user_room_member_info}; /// The currently-known state of a user's avatar. #[derive(Clone, Debug)] @@ -529,7 +529,7 @@ impl Widget for UserProfileSlidingPane { // Re-fetch the currently-displayed user profile info from the cache in case it was updated. let mut redraw_this_pane = false; if let Some(our_info) = self.info.as_mut() { - if let (Some(new_profile), room_member) = get_user_profile_and_room_members( + if let (Some(new_profile), room_member) = get_user_profile_and_room_member( cx, &our_info.user_id, &our_info.room_id, @@ -669,7 +669,9 @@ impl UserProfileSlidingPane { &info.room_id, ) { log!("Found user {} room member info in cache", info.user_id); - info.avatar_state = AvatarState::Known(room_member.avatar_url().map(|uri| uri.to_owned())); + if let Some(uri) = room_member.avatar_url() { + info.avatar_state = AvatarState::Known(Some(uri.to_owned())); + } info.room_member = Some(room_member); } } @@ -682,6 +684,11 @@ impl UserProfileSlidingPane { local_only: false, }); } + if let AvatarState::Known(Some(uri)) = &info.avatar_state { + if let AvatarCacheEntry::Loaded(data) = avatar_cache::get_or_fetch_avatar(_cx, uri.clone()) { + info.avatar_state = AvatarState::Loaded(data); + } + } self.info = Some(info); } diff --git a/src/profile/user_profile_cache.rs b/src/profile/user_profile_cache.rs index 88099201..6852db84 100644 --- a/src/profile/user_profile_cache.rs +++ b/src/profile/user_profile_cache.rs @@ -203,7 +203,7 @@ pub fn get_user_profile(_cx: &mut Cx, user_id: &UserId) -> Option { /// This function requires passing in a reference to `Cx`, /// which isn't used, but acts as a guarantee that this function /// must only be called by the main UI thread. -pub fn get_user_profile_and_room_members( +pub fn get_user_profile_and_room_member( _cx: &mut Cx, user_id: &UserId, room_id: &RoomId, From cacc4b420ee269f1f72ffdbab187a064495a8048 Mon Sep 17 00:00:00 2001 From: Kevin Boos Date: Wed, 14 Aug 2024 19:09:15 -0700 Subject: [PATCH 2/2] Upon first view, set room timeline PortalList to tail range (show the end) --- src/home/room_screen.rs | 38 +++++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/src/home/room_screen.rs b/src/home/room_screen.rs index 3056c076..429b02f8 100644 --- a/src/home/room_screen.rs +++ b/src/home/room_screen.rs @@ -817,6 +817,8 @@ struct TimelineUiState { struct SavedState { /// The index of the first item in the timeline's PortalList that is currently visible, /// and the scroll offset from the top of the list's viewport to the beginning of that item. + /// If this is `None`, then the timeline has not yet been scrolled by the user + /// and the portal list will be set to "tail" (track) the bottom of the list. first_index_and_scroll: Option<(usize, f64)>, /// The unique ID of the event that corresponds to the first item visible in the timeline. first_event_id: Option, @@ -906,7 +908,7 @@ impl Timeline { /// Note: after calling this function, the timeline's `tl_state` will be `None`. fn save_state(&mut self) { let Some(mut tl) = self.tl_state.take() else { - log!("Timeline::save_state(): skipping due to missing state, room {:?}", self.room_id); + error!("Timeline::save_state(): skipping due to missing state, room {:?}", self.room_id); return; }; let portal_list = self.portal_list(id!(list)); @@ -935,6 +937,10 @@ impl Timeline { if let Some((first_index, scroll_from_first_id)) = tl_state.saved_state.first_index_and_scroll { self.portal_list(id!(list)) .set_first_id_and_scroll(first_index, scroll_from_first_id); + } else { + // If the first index is not set, then the timeline has not yet been scrolled by the user, + // so we set the portal list to "tail" (track) the bottom of the list. + self.portal_list(id!(list)).set_tail_range(true); } // TODO: restore the message input box's draft text and cursor head/tail positions. @@ -997,19 +1003,14 @@ impl Widget for Timeline { // that its timeline events have been updated in the background. if let Event::Signal = event { let portal_list = self.portal_list(id!(list)); + let orig_first_id = portal_list.first_id(); + let scroll_from_first_id = portal_list.scroll_position(); let Some(tl) = self.tl_state.as_mut() else { return }; let mut done_loading = false; while let Ok(update) = tl.update_receiver.try_recv() { match update { TimelineUpdate::NewItems { items, changed_indices, clear_cache } => { - // TODO: we can often avoid the following loops that iterate ovoer the `items` list - // by only doing that if `clear_cache` is true, or if `changed_indices` range includes - // any index that comes before (is less than) the above `orig_first_id`. - - let orig_first_id = portal_list.first_id(); - let scroll_from_first_id = portal_list.scroll_position(); - // Determine which item is currently visible the top of the screen (the first event) // so that we can jump back to that position instantly after applying this update. let current_first_event_id_opt = tl.items @@ -1018,7 +1019,26 @@ impl Widget for Timeline { .and_then(|ev| ev.event_id().map(|i| i.to_owned())) ); - log!("current_first_event_id_opt: {current_first_event_id_opt:?}, orig_first_id: {orig_first_id}"); + log!("current_first_event_id_opt: {current_first_event_id_opt:?}, orig_first_id: {orig_first_id}, old items: {}, new items: {}", + tl.items.len(), items.len(), + ); + + if items.is_empty() { + log!("Timeline::handle_event(): timeline was cleared for room {}", tl.room_id); + + // TODO: Save the current first event ID before it gets removed + // such that we can jump back to that position later after applying this update. + + // TODO: here we need to re-build the timeline via TimelineBuilder + // and set the TimelineFocus to one of the above-saved event IDs. + } + + // Maybe todo?: we can often avoid the following loops that iterate over the `items` list + // by only doing that if `clear_cache` is true, or if `changed_indices` range includes + // any index that comes before (is less than) the above `orig_first_id`. + + + if let Some(top_event_id) = current_first_event_id_opt.as_ref() { for (idx, item) in items.iter().enumerate() { let Some(item_event_id) = item.as_event().and_then(|ev| ev.event_id()) else {