From: Simon Tatham Date: Sat, 27 Jan 2024 09:12:14 +0000 (+0000) Subject: Implement a proper TUI Error Log. X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ian/git?a=commitdiff_plain;h=29d51d5a5cab41e4428068291f56e73d6b4df2bc;p=mastodonochrome.git Implement a proper TUI Error Log. At last! Now, most errors received from the server will go into a log that doesn't interrupt and terminate the TUI, and you can at least try to recover from them and do something else instead. There's undoubtedly still work to be done in making the errors more useful, but this is a start. I've left some thoughts in TODO.md. --- diff --git a/TODO.md b/TODO.md index eac263d..6929698 100644 --- a/TODO.md +++ b/TODO.md @@ -17,31 +17,39 @@ Again, this will need some persistent inter-activity state in a `RefCell` in the `TuiLogicalState`. Also some UI thought about what history should be shared with what. -## Error handling in the UI - -Real Monochrome has an 'Error Log' at ESC Y V, containing records of -errors that happened in relation to your particular connection, such -as when you tried to read a file but a backend error happened on the -server. We should have one of those, and use it to handle failure to -retrieve things you were trying to read. - -This is already marked in the code with multiple FIXMEs: - -* the `result.expect` at the end of `new_activity_state` -* the handler for the existing `LogicalAction::Error`, which currently - just beeps -* the handler for the return value of `Client::process_stream_update` -* the error in `PostMenu::post`, although if the error is recoverable - (e.g. by editing the text and retrying) then the Error Log is - overkill and we should redisplay the `PostMenu` with the message -* errors in `get_user_to_examine` and `get_post_id_to_read`, although - again if it's just 'no such thing' then the Error Log is overkill. - Should only be for 'help, the server is totally confused'. -* errors in `save_ldb` - -Each of those should instead catch the error, make an Error Log -record, and transfer you to the error-log viewer with a beep, just the -way Mono did it. +## Error handling improvements + +Error Log entries could usefully include various extra detail: +* `ClientError` should distinguish HTTP methods, so that the + inappropriate text 'fetching URL' isn't used in the case where you + were trying to _do_ something, like post a toot. +* probably the method of `Client` that generated an error ought to + annotate it with a description of the client operation being + performed, so that it's not just 'while fetching this URL' but + 'while trying to retrieve a status / account / etc', and maybe even + which one. +* the caller of that method in turn might also want to add further + context, like 'I was trying to fetch this status because this other + one was replying to it'. + +Some error situations could be handled in a less generic way: + +* in `PostMenu::post`, if the server refuses the post, we should + display the error in the posting menu and let the user modify and + retry, similar to the way the login/registration flow does it. But + any error weirder than that should still go to the Error Log. +* similarly, in the `get_user_to_examine` and `get_post_id_to_read` + overlay activities, we should respond to 'no such thing' by letting + the user re-edit and try again, in case it was just a typo + +We don't have good handling for I/O errors while saving the user's +LDB. That's not a _client_ error, but we could make an extra enum +branch in `ClientError` anyway, so that the TuiLogicalState could put +it in the Error Log to notify the user of a problem. Then perhaps we +could set a flag to suppress further LDB-saving errors (don't want to +keep whinging every 2 minutes or on every file change!), and clear the +flag again if a later LDB save is successful (e.g. the user has freed +up some disk space). ## Reading diff --git a/src/activity_stack.rs b/src/activity_stack.rs index eeac1e6..ea9dadc 100644 --- a/src/activity_stack.rs +++ b/src/activity_stack.rs @@ -37,6 +37,7 @@ pub enum UtilityActivity { ListStatusBoosters(String), UserOptions(String), InstanceRules, + ErrorLog, } #[derive(PartialEq, Eq, Debug, Clone, Hash)] diff --git a/src/client.rs b/src/client.rs index 98e1acf..558e919 100644 --- a/src/client.rs +++ b/src/client.rs @@ -33,6 +33,7 @@ pub enum FeedId { Boosters(String), Followers(String), Followees(String), + Errors, } #[derive(Debug, PartialEq, Eq, Clone)] @@ -538,7 +539,7 @@ pub enum AppTokenType<'a> { impl Client { pub fn new(auth: AuthConfig) -> Result { - Ok(Client { + let mut client = Client { auth, client: reqwest_client()?, accounts: HashMap::new(), @@ -550,7 +551,19 @@ impl Client { permit_write: false, logfile: None, error_log: ErrorLog::new(), - }) + }; + + client.feeds.insert( + FeedId::Errors, + Feed { + ids: VecDeque::new(), + origin: 0, + extend_past: None, + extend_future: None, + }, + ); + + Ok(client) } pub fn set_writable(&mut self, permit: bool) { @@ -968,6 +981,7 @@ impl Client { FeedId::Followees(id) => { Req::get(&format!("api/v1/accounts/{id}/following")) } + FeedId::Errors => return Ok(false), }; let req = match ext { @@ -1094,6 +1108,9 @@ impl Client { } acs.iter().rev().map(|ac| ac.id.clone()).collect() } + FeedId::Errors => { + panic!("we should have returned early in this case") + } }; let any_new = !ids.is_empty(); @@ -1773,6 +1790,23 @@ impl Client { } pub fn add_to_error_log(&mut self, err: ClientError) { + let new_id = + self.error_log.origin + (self.error_log.items.len() as isize); self.error_log.push_now(err); + self.feeds + .get_mut(&FeedId::Errors) + .expect("the Error feed should always be present") + .ids + .push_back(new_id.to_string()); + } + + pub fn get_error_log_entry( + &self, + id: &str, + ) -> (ClientError, DateTime) { + let index = isize::from_str_radix(id, 10).expect( + "We generated these ids ourselves and they should all be valid", + ); + self.error_log.items[(index - self.error_log.origin) as usize].clone() } } diff --git a/src/file.rs b/src/file.rs index f29425b..887a66b 100644 --- a/src/file.rs +++ b/src/file.rs @@ -114,11 +114,11 @@ impl FileDataSource for FeedSource { } fn extendable(&self) -> bool { - true + self.id != FeedId::Errors } fn want_to_jump_to_new_content(&self) -> bool { - self.id == FeedId::Mentions + self.id == FeedId::Mentions || self.id == FeedId::Errors } } @@ -2204,3 +2204,41 @@ pub fn view_thread( )?; Ok(Box::new(file)) } + +struct ErrorLogFileType; +impl FileType for ErrorLogFileType { + type Item = ErrorLogEntry; + + fn get_item( + &self, + id: &str, + client: &mut Client, + ) -> Result { + let (err, time) = client.get_error_log_entry(id); + Ok(ErrorLogEntry::new(err, time)) + } +} + +pub fn error_log( + file_positions: &HashMap, + client: &mut Client, + is_interrupt: bool, +) -> Box { + let feed = FeedId::Errors; + let pos = file_positions.get(&feed); + let title = ColouredString::general( + "Error Log [ESC][Y][V]", + "HHHHHHHHHHHHHKKKHHKHHKH", + ); + + let file = File::new_infallible( + client, + FeedSource::new(feed), + title, + ErrorLogFileType, + pos, + None, + is_interrupt, + ); + Box::new(file) +} diff --git a/src/tui.rs b/src/tui.rs index 1d1c36b..e300394 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -532,7 +532,7 @@ impl Tui { Err(err) => { self.client.add_to_error_log(err); - // FIXME: throw user into the Error Log + self.state.throw_into_error_log(&mut self.client); Vec::new() } } @@ -812,6 +812,8 @@ impl TuiLogicalState { } LogicalAction::Error(err) => { client.add_to_error_log(err); + self.activity_stack.goto(UtilityActivity::ErrorLog.into()); + self.changed_activity(client, None, true); break PhysicalAction::Beep; } LogicalAction::PostComposed(post) => { @@ -1023,6 +1025,9 @@ impl TuiLogicalState { client, is_interrupt, ), + Activity::Util(UtilityActivity::ErrorLog) => { + Ok(error_log(&self.file_positions, client, is_interrupt)) + } Activity::Util(UtilityActivity::EgoLog) => { ego_log(&self.file_positions, client) } @@ -1108,7 +1113,7 @@ impl TuiLogicalState { Ok(state) => state, Err(err) => { client.add_to_error_log(err); - panic!("FIXME: need to implement the Error Log here"); + error_log(&self.file_positions, client, true) } } } @@ -1227,4 +1232,9 @@ impl TuiLogicalState { None => None, } } + + fn throw_into_error_log(&mut self, client: &mut Client) { + self.activity_stack.goto(UtilityActivity::ErrorLog.into()); + self.changed_activity(client, None, true); + } }