From fdb243f83e47d50af2db6688fec272dd8f8a2aaf Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sun, 21 Jan 2024 07:02:59 +0000 Subject: [PATCH] Keep persistent ActivityStates for everything on the stack. This fixes the cases where temporarily going to another full-screen activity failed to restore the in-progress state of the previous one. In particular, nipping out to the utilities menu from composing a post, and reading the server rules during account registration. It's a shame; I'd have preferred to be deleting and recreating state objects all the time, to ensure they had a well-defined amount of internal state. But this needed less organisation. Perhaps I can come back and do it better another time. --- TODO.md | 12 ---- src/activity_stack.rs | 18 ++++-- src/file.rs | 2 +- src/tui.rs | 147 ++++++++++++++++++++++++++++++------------ 4 files changed, 121 insertions(+), 58 deletions(-) diff --git a/TODO.md b/TODO.md index ed3e261..eb1393f 100644 --- a/TODO.md +++ b/TODO.md @@ -1,17 +1,5 @@ # Bugs -Some state fails to be retained if you temporarily leave an activity -via [ESC] and then return to it (say via [RET]). In the post-compose -menu, the client panics because `TuiLogicalState::new_activity_state` -was trying to go into the post-compose menu without the previous -activity having passed it a `Post`, because the activity in question -was the utilities menu which hasn't got one. And in login / account -registration, we go right back to the beginning. _Some_ new technique -is needed for persisting the state of activities on the current stack; -perhaps we need to start keeping a `Vec>`? Or -perhaps just have each activity save its absolutely necessary stuff? -Not sure which is better. - Server error reporting: HTTP error codes from Mastodon API requests are generally accompanied by a JSON body containing a more useful message. For example, if you prematurely try to start the full client diff --git a/src/activity_stack.rs b/src/activity_stack.rs index 2eaa5a0..552ed9e 100644 --- a/src/activity_stack.rs +++ b/src/activity_stack.rs @@ -1,7 +1,7 @@ use super::client::{Boosts, Replies}; use super::file::SearchDirection; -#[derive(PartialEq, Eq, Debug, Clone)] +#[derive(PartialEq, Eq, Debug, Clone, Hash)] pub enum NonUtilityActivity { MainMenu, HomeTimelineFile, @@ -14,7 +14,7 @@ pub enum NonUtilityActivity { LoginMenu, } -#[derive(PartialEq, Eq, Debug, Clone)] +#[derive(PartialEq, Eq, Debug, Clone, Hash)] pub enum UtilityActivity { UtilsMenu, ReadMentions, @@ -35,7 +35,7 @@ pub enum UtilityActivity { InstanceRules, } -#[derive(PartialEq, Eq, Debug, Clone)] +#[derive(PartialEq, Eq, Debug, Clone, Hash)] pub enum OverlayActivity { GetUserToExamine, GetPostIdToRead, @@ -43,7 +43,7 @@ pub enum OverlayActivity { GetSearchExpression(SearchDirection), } -#[derive(PartialEq, Eq, Debug, Clone)] +#[derive(PartialEq, Eq, Debug, Clone, Hash)] pub enum Activity { NonUtil(NonUtilityActivity), Util(UtilityActivity), @@ -181,6 +181,16 @@ impl ActivityStack { self.pop(); self.goto(act); } + + pub fn iter(&self) -> impl Iterator + '_ { + self.nonutil + .iter() + .cloned() + .map(Activity::NonUtil) + .chain(self.util.iter().cloned().map(Activity::Util)) + .chain(self.initial_util.iter().cloned().map(Activity::Util)) + .chain(self.overlay.iter().cloned().map(Activity::Overlay)) + } } #[test] diff --git a/src/file.rs b/src/file.rs index 1e0994f..f294fd7 100644 --- a/src/file.rs +++ b/src/file.rs @@ -373,7 +373,7 @@ enum UIMode { Select(HighlightType, SelectionPurpose), } -#[derive(Debug, PartialEq, Eq, Clone, Copy)] +#[derive(Debug, PartialEq, Eq, Clone, Copy, Hash)] pub enum SearchDirection { Up, Down, diff --git a/src/tui.rs b/src/tui.rs index 6d621e2..39e75c7 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -303,7 +303,7 @@ impl Tui { finish_account_setup(&mut client, cfgloc)?; } - let mut state = TuiLogicalState::new(&client, cfgloc.clone()); + let mut state = TuiLogicalState::new(&mut client, cfgloc.clone()); state.load_ldb()?; stdout().execute(EnterAlternateScreen)?; @@ -623,8 +623,7 @@ pub trait ActivityState { struct TuiLogicalState { activity_stack: ActivityStack, - activity_state: Box, - overlay_activity_state: Option>, + activity_states: HashMap>, last_area: Option, file_positions: HashMap, unfolded_posts: Rc>>, @@ -632,23 +631,24 @@ struct TuiLogicalState { } impl TuiLogicalState { - fn new(client: &Client, cfgloc: ConfigLocation) -> Self { - let (activity, activity_state) = if client.auth.is_logged_in() { - (NonUtilityActivity::MainMenu, main_menu(client)) + fn new(client: &mut Client, cfgloc: ConfigLocation) -> Self { + let activity = if client.auth.is_logged_in() { + NonUtilityActivity::MainMenu } else { - (NonUtilityActivity::LoginMenu, login_menu(cfgloc.clone())) + NonUtilityActivity::LoginMenu }; - let activity_stack = ActivityStack::new(activity); - TuiLogicalState { - activity_stack, - activity_state, - overlay_activity_state: None, + let mut st = TuiLogicalState { + activity_stack: ActivityStack::new(activity), + activity_states: HashMap::new(), last_area: None, file_positions: HashMap::new(), cfgloc, unfolded_posts: Rc::new(RefCell::new(HashSet::new())), - } + }; + st.changed_activity(client, None, false); + + st } fn draw_frame( @@ -660,13 +660,13 @@ impl TuiLogicalState { if self.last_area != Some(area) { self.last_area = Some(area); - self.activity_state.resize(w, h); - if let Some(ref mut state) = self.overlay_activity_state { + self.activity_state_mut().resize(w, h); + if let Some(ref mut state) = self.overlay_activity_state_mut() { state.resize(w, h); } } - let (lines, cursorpos) = self.activity_state.draw(w, h); + let (lines, cursorpos) = self.activity_state().draw(w, h); let mut cursorpos = cursorpos; buf.reset(); let mut last_x = 0; @@ -686,7 +686,7 @@ impl TuiLogicalState { CursorPosition::End => Some((last_x, last_y)), }; - if let Some(state) = &self.overlay_activity_state { + if let Some(state) = &self.overlay_activity_state() { let (lines, overlay_cursorpos) = state.draw(w, h); cursorpos = overlay_cursorpos; let ytop = h - min(h, lines.len()); @@ -734,10 +734,10 @@ impl TuiLogicalState { OurKey::Ctrl('L') => return PhysicalAction::Refresh, _ => { - if let Some(ref mut state) = self.overlay_activity_state { + if let Some(state) = self.overlay_activity_state_mut() { state.handle_keypress(key, client) } else { - self.activity_state.handle_keypress(key, client) + self.activity_state_mut().handle_keypress(key, client) } } }; @@ -773,7 +773,7 @@ impl TuiLogicalState { } LogicalAction::GotSearchExpression(dir, regex) => { self.pop_overlay_activity(); - self.activity_state.got_search_expression(dir, regex) + self.activity_state_mut().got_search_expression(dir, regex) } LogicalAction::Error(_) => break PhysicalAction::Beep, // FIXME: Error Log LogicalAction::PostComposed(post) => { @@ -813,7 +813,7 @@ impl TuiLogicalState { feeds_updated: HashSet, client: &mut Client, ) -> PhysicalAction { - self.activity_state + self.activity_state_mut() .handle_feed_updates(&feeds_updated, client); if feeds_updated.contains(&FeedId::Mentions) { @@ -862,18 +862,36 @@ impl TuiLogicalState { } fn checkpoint_ldb(&mut self) { - if let Some((feed_id, saved_pos)) = - self.activity_state.save_file_position() - { - let changed = - self.file_positions.get(&feed_id) != Some(&saved_pos); - self.file_positions.insert(feed_id, saved_pos); - if changed { - // FIXME: maybe suddenly change our mind and go to the - // Error Log - self.save_ldb().unwrap(); + let mut changed = false; + for state in self.activity_states.values_mut() { + if let Some((feed_id, saved_pos)) = state.save_file_position() { + if self.file_positions.get(&feed_id) != Some(&saved_pos) { + self.file_positions.insert(feed_id, saved_pos); + changed = true; + } } } + + if changed { + // FIXME: maybe suddenly change our mind and go to the + // Error Log + self.save_ldb().unwrap(); + } + } + + fn ensure_activity_state( + &mut self, + act: Activity, + client: &mut Client, + post: Option, + is_interrupt: bool, + ) { + if !self.activity_states.contains_key(&act) { + self.activity_states.insert( + act.clone(), + self.new_activity_state(act, client, post, is_interrupt), + ); + } } fn changed_activity( @@ -883,30 +901,36 @@ impl TuiLogicalState { is_interrupt: bool, ) { self.checkpoint_ldb(); - self.activity_state = self.new_activity_state( + + self.ensure_activity_state( self.activity_stack.top(), client, post, is_interrupt, ); - self.overlay_activity_state = match self.activity_stack.overlay() { - Some(activity) => { - Some(self.new_activity_state(activity, client, None, false)) - } - None => None, - }; + if let Some(act) = self.activity_stack.overlay() { + self.ensure_activity_state(act, client, None, false); + } + + self.gc_activity_states(); + if let Some(area) = self.last_area { let (w, h) = (area.width as usize, area.height as usize); - self.activity_state.resize(w, h); - if let Some(ref mut state) = self.overlay_activity_state { + self.activity_state_mut().resize(w, h); + if let Some(state) = self.overlay_activity_state_mut() { state.resize(w, h); } } } + fn gc_activity_states(&mut self) { + let states: HashSet<_> = self.activity_stack.iter().collect(); + self.activity_states.retain(|k, _| states.contains(k)); + } + fn pop_overlay_activity(&mut self) { self.activity_stack.pop_overlay(); - self.overlay_activity_state = None; + self.gc_activity_states(); } fn new_activity_state( @@ -1115,4 +1139,45 @@ impl TuiLogicalState { } } } + + fn activity_state(&self) -> &dyn ActivityState { + self.activity_states + .get(&self.activity_stack.top()) + .expect("every current activity ought to have an entry here") + .as_ref() + } + fn activity_state_mut(&mut self) -> &mut dyn ActivityState { + self.activity_states + .get_mut(&self.activity_stack.top()) + .expect("every current activity ought to have an entry here") + .as_mut() + } + fn overlay_activity_state(&self) -> Option<&dyn ActivityState> { + match self.activity_stack.overlay() { + Some(activity) => Some( + self.activity_states + .get(&activity) + .expect( + "every current activity ought to have an entry here", + ) + .as_ref(), + ), + None => None, + } + } + fn overlay_activity_state_mut( + &mut self, + ) -> Option<&mut (dyn ActivityState + '_)> { + match self.activity_stack.overlay() { + Some(activity) => Some( + self.activity_states + .get_mut(&activity) + .expect( + "every current activity ought to have an entry here", + ) + .as_mut(), + ), + None => None, + } + } } -- 2.30.2