address hanging TODOs

helix-loader/src/persistence.rs

@@ -12,11 +12,9 @@ pub fn write_history<T: Serialize>(filepath: PathBuf, entries: &Vec<T>) {
         .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<T: Serialize>(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<T: for<'a> Deserialize<'a>>(filepath: PathBuf) -> Vec<T> {
+pub fn read_history<T: for<'a> Deserialize<'a>>(filepath: &PathBuf) -> Vec<T> {
     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<T: for<'a> Deserialize<'a>>(filepath: PathBuf) -> Vec<T> {
         }
         Err(e) => match e.kind() {
             io::ErrorKind::NotFound => Vec::new(),
-            // TODO: do something about this panic
+            // Going through the potential errors listed from the docs:
-            _ => panic!(),
+            // - `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<T: Clone + Serialize + for<'a> Deserialize<'a>>(
     filepath: PathBuf,
     limit: usize,
 ) {
-    // TODO: can we remove this clone?
+    let history: Vec<T> = read_history(&filepath);
-    let history: Vec<T> = read_history(filepath.clone());
     if history.len() > limit {
         let trim_start = history.len() - limit;
         let trimmed_history = history[trim_start..].to_vec();

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 (view, doc) = current!(editor);
-                            let doc = doc_mut!(editor, &doc_id);
                             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);

helix-term/src/commands/typed.rs

@@ -132,10 +132,8 @@ fn open(cx: &mut compositor::Context, args: &[Cow<str>], 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(())

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("oa<esc>:wq<ret>"),
-        Some("oa<esc>h:wq<ret>"),
-        // Some(&|app| {
-        //     assert!(!app.editor.is_err(), "error: {:?}", app.editor.get_status());
-        // }),
         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())

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 (id, new_doc) = if let Some(id) = id {
-        let mut new_doc = false;
+            (id, false)
-
-        let id = if let Some(id) = id {
-            id
         } 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
+                // 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) {
+                if !selection.ranges().iter().any(|range| range.to() > doc_len) {
                     doc.set_view_offset(view.id, view_position);
                     doc.set_selection(view.id, selection);
                 }

helix-view/src/persistence.rs

@@ -31,7 +31,7 @@ pub fn push_file_history(entry: &FileHistoryEntry) {
 }
 
 pub fn read_file_history() -> Vec<FileHistoryEntry> {
-    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<String> {
-    read_history(filepath)
+    read_history(&filepath)
 }
 
 pub fn read_command_history() -> Vec<String> {
@@ -77,5 +77,5 @@ pub fn write_clipboard_file(values: &Vec<String>) {
 }
 
 pub fn read_clipboard_file() -> Vec<String> {
-    read_history(clipboard_file())
+    read_history(&clipboard_file())
 }