From 36a8a614a95c4f294e272b8b0466e5a14e6fbe34 Mon Sep 17 00:00:00 2001 From: Ingrid Date: Sun, 22 Sep 2024 16:41:46 +0200 Subject: [PATCH] address hanging TODOs --- helix-loader/src/persistence.rs | 26 ++++++++++++++++---------- helix-term/src/application.rs | 19 ++++++------------- helix-term/src/commands/typed.rs | 4 +--- helix-term/tests/test/persistence.rs | 16 ++++++---------- helix-view/src/editor.rs | 17 +++++++---------- helix-view/src/persistence.rs | 6 +++--- 6 files changed, 39 insertions(+), 49 deletions(-) diff --git a/helix-loader/src/persistence.rs b/helix-loader/src/persistence.rs index c9ec96871..019977794 100644 --- a/helix-loader/src/persistence.rs +++ b/helix-loader/src/persistence.rs @@ -12,11 +12,9 @@ pub fn write_history(filepath: PathBuf, entries: &Vec) { .create(true) .truncate(true) .open(filepath) - // TODO: do something about this unwrap .unwrap(); for entry in entries { - // TODO: do something about this unwrap serialize_into(&file, &entry).unwrap(); } } @@ -26,19 +24,23 @@ pub fn push_history(filepath: PathBuf, entry: &T) { .append(true) .create(true) .open(filepath) - // TODO: do something about this unwrap .unwrap(); - // TODO: do something about this unwrap serialize_into(file, entry).unwrap(); } -pub fn read_history Deserialize<'a>>(filepath: PathBuf) -> Vec { +pub fn read_history Deserialize<'a>>(filepath: &PathBuf) -> Vec { match File::open(filepath) { Ok(file) => { let mut read = BufReader::new(file); let mut entries = Vec::new(); - // TODO: more sophisticated error handling + // FIXME: Can we do better error handling here? It's unfortunate that bincode doesn't + // distinguish an empty reader from an actual error. + // + // Perhaps we could use the underlying bufreader to check for emptiness in the while + // condition, then we could know any errors from bincode should be surfaced or logged. + // BufRead has a method `has_data_left` that would work for this, but at the time of + // writing it is nightly-only and experimental :( while let Ok(entry) = deserialize_from(&mut read) { entries.push(entry); } @@ -46,8 +48,13 @@ pub fn read_history Deserialize<'a>>(filepath: PathBuf) -> Vec { } Err(e) => match e.kind() { io::ErrorKind::NotFound => Vec::new(), - // TODO: do something about this panic - _ => panic!(), + // Going through the potential errors listed from the docs: + // - `InvalidInput` can't happen since we aren't setting options + // - `AlreadyExists` can't happen since we aren't setting `create_new` + // - `PermissionDenied` could happen if someone really borked their file permissions + // in `~/.local`, but helix already panics in that case, and I think a panic is + // acceptable. + _ => unreachable!(), }, } } @@ -56,8 +63,7 @@ pub fn trim_history Deserialize<'a>>( filepath: PathBuf, limit: usize, ) { - // TODO: can we remove this clone? - let history: Vec = read_history(filepath.clone()); + let history: Vec = read_history(&filepath); if history.len() > limit { let trim_start = history.len() - limit; let trimmed_history = history[trim_start..].to_vec(); diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index be2e35cb4..ab7ce1237 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -164,26 +164,23 @@ impl Application { old_file_locs, ); - // TODO: do most of this in the background? + // Should we be doing these in background tasks? if persistence_config.commands { editor .registers .write(':', persistence::read_command_history()) - // TODO: do something about this unwrap .unwrap(); } if persistence_config.search { editor .registers .write('/', persistence::read_search_history()) - // TODO: do something about this unwrap .unwrap(); } if persistence_config.clipboard { editor .registers .write('"', persistence::read_clipboard_file()) - // TODO: do something about this unwrap .unwrap(); } @@ -228,25 +225,25 @@ impl Application { Some(Layout::Horizontal) => Action::HorizontalSplit, None => Action::Load, }; - let doc_id = match editor.open(&file, action) { + match editor.open(&file, action) { // Ignore irregular files during application init. Err(DocumentOpenError::IrregularFile) => { nr_of_files -= 1; continue; } Err(err) => return Err(anyhow::anyhow!(err)), - Ok(doc_id) => doc_id, + Ok(_) => (), }; // with Action::Load all documents have the same view // NOTE: this isn't necessarily true anymore. If // `--vsplit` or `--hsplit` are used, the file which is // opened last is focused on. if let Some(pos) = pos { - let view_id = editor.tree.focus; - let doc = doc_mut!(editor, &doc_id); + let (view, doc) = current!(editor); let pos = Selection::point(pos_at_coords(doc.text().slice(..), pos, true)); - doc.set_selection(view_id, pos); + doc.set_selection(view.id, pos); + align_view(doc, view, Align::Center); } } } @@ -260,10 +257,6 @@ impl Application { nr_of_files, if nr_of_files == 1 { "" } else { "s" } // avoid "Loaded 1 files." grammo )); - // align the view to center after all files are loaded, - // does not affect views without pos since it is at the top - // let (view, doc) = current!(editor); - // align_view(doc, view, Align::Center); } } else { editor.new_file(Action::VerticalSplit); diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs index 237f29730..69f5b5051 100644 --- a/helix-term/src/commands/typed.rs +++ b/helix-term/src/commands/typed.rs @@ -132,10 +132,8 @@ fn open(cx: &mut compositor::Context, args: &[Cow], event: PromptEvent) -> if let Some(pos) = pos { let pos = Selection::point(pos_at_coords(doc.text().slice(..), pos, true)); doc.set_selection(view.id, pos); + align_view(doc, view, Align::Center); } - // does not affect opening a buffer without pos - // TODO: ensure removing this will not cause problems - // align_view(doc, view, Align::Center); } } Ok(()) diff --git a/helix-term/tests/test/persistence.rs b/helix-term/tests/test/persistence.rs index 34bacb426..ac244ecda 100644 --- a/helix-term/tests/test/persistence.rs +++ b/helix-term/tests/test/persistence.rs @@ -53,11 +53,7 @@ async fn test_persistence() -> anyhow::Result<()> { .with_config(config_with_persistence()) .with_file(file.path(), None) .build()?, - // TODO: remove the h with a bugfix? - Some("oah:wq"), - // Some(&|app| { - // assert!(!app.editor.is_err(), "error: {:?}", app.editor.get_status()); - // }), + Some("oa:wq"), None, true, ) @@ -69,10 +65,10 @@ async fn test_persistence() -> anyhow::Result<()> { // Session 2: // open same file, // add newline, then b, - // copy the line + // copy the line ("b\n") // search for "a" // go back down to b - // use last command + // use last command (write-quit) test_key_sequence( &mut helpers::AppBuilder::new() .with_config(config_with_persistence()) @@ -91,10 +87,10 @@ async fn test_persistence() -> anyhow::Result<()> { // Session 3: // open same file, // paste - // use last search + // use last search ("/a") // append a // search for "1", "2", and "3" in sequence. - // use last command + // use last command (write-quit) test_key_sequence( &mut helpers::AppBuilder::new() .with_config(config_with_persistence()) @@ -112,7 +108,7 @@ async fn test_persistence() -> anyhow::Result<()> { // Session 4: // open same file - // use last command + // use last command (write-quit) test_key_sequence( &mut helpers::AppBuilder::new() .with_config(config_with_persistence()) diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index c7add0994..1b25b1e0f 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -1740,14 +1740,9 @@ impl Editor { let path = helix_stdx::path::canonicalize(path); let id = self.document_by_path(&path).map(|doc| doc.id); - // TODO: surely there's a neater way to do this? - let mut new_doc = false; - - let id = if let Some(id) = id { - id + let (id, new_doc) = if let Some(id) = id { + (id, false) } else { - new_doc = true; - let mut doc = Document::open( &path, None, @@ -1767,11 +1762,13 @@ impl Editor { let id = self.new_document(doc); self.launch_language_servers(id); - id + (id, true) }; self.switch(id, action); + // Restore file position + // This needs to happen after the call to switch, since switch messes with view offsets if new_doc && !self .config() @@ -1786,8 +1783,8 @@ impl Editor { let (view, doc) = current!(self); let doc_len = doc.text().len_chars(); - // don't restore the view and selection if the selection goes beyond the file's end - if !selection.ranges().iter().any(|range| range.to() >= doc_len) { + // Don't restore the view and selection if the selection goes beyond the file's end + if !selection.ranges().iter().any(|range| range.to() > doc_len) { doc.set_view_offset(view.id, view_position); doc.set_selection(view.id, selection); } diff --git a/helix-view/src/persistence.rs b/helix-view/src/persistence.rs index 332fc0392..754b08aa4 100644 --- a/helix-view/src/persistence.rs +++ b/helix-view/src/persistence.rs @@ -31,7 +31,7 @@ pub fn push_file_history(entry: &FileHistoryEntry) { } pub fn read_file_history() -> Vec { - read_history(file_histfile()) + read_history(&file_histfile()) } pub fn trim_file_history(limit: usize) { @@ -49,7 +49,7 @@ pub fn push_reg_history(register: char, line: &String) { } fn read_reg_history(filepath: PathBuf) -> Vec { - read_history(filepath) + read_history(&filepath) } pub fn read_command_history() -> Vec { @@ -77,5 +77,5 @@ pub fn write_clipboard_file(values: &Vec) { } pub fn read_clipboard_file() -> Vec { - read_history(clipboard_file()) + read_history(&clipboard_file()) }