From 69a13fbba2f4caac6dc0d981b0cd077e4fa4b489 Mon Sep 17 00:00:00 2001
From: Ingrid <git@ingrids.email>
Date: Fri, 16 Feb 2024 21:08:06 +0100
Subject: [PATCH] fix quirky file persistence behaviour

---
 helix-term/src/application.rs    |  4 +-
 helix-term/src/commands/typed.rs |  3 +-
 helix-view/src/editor.rs         | 76 ++++++++++++++++++++++----------
 3 files changed, 57 insertions(+), 26 deletions(-)

diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs
index 5f6e79809..57a8d0c52 100644
--- a/helix-term/src/application.rs
+++ b/helix-term/src/application.rs
@@ -247,8 +247,8 @@ impl Application {
                     ));
                     // 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);
+                    // 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 495e73459..ad0d6eff3 100644
--- a/helix-term/src/commands/typed.rs
+++ b/helix-term/src/commands/typed.rs
@@ -134,7 +134,8 @@ fn open(cx: &mut compositor::Context, args: &[Cow<str>], event: PromptEvent) ->
                 doc.set_selection(view.id, pos);
             }
             // does not affect opening a buffer without pos
-            align_view(doc, view, Align::Center);
+            // TODO: ensure removing this will not cause problems
+            // align_view(doc, view, Align::Center);
         }
     }
     Ok(())
diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs
index f513a0bcc..04dd49fe8 100644
--- a/helix-view/src/editor.rs
+++ b/helix-view/src/editor.rs
@@ -11,6 +11,7 @@ use crate::{
     register::Registers,
     theme::{self, Theme},
     tree::{self, Tree},
+    view::ViewPosition,
     Document, DocumentId, View, ViewId,
 };
 use dap::StackFrame;
@@ -1540,6 +1541,7 @@ impl Editor {
     fn replace_document_in_view(&mut self, current_view: ViewId, doc_id: DocumentId) {
         let scrolloff = self.config().scrolloff;
         let view = self.tree.get_mut(current_view);
+        view.doc = doc_id;
 
         view.doc = doc_id;
         let doc = doc_mut!(self, &doc_id);
@@ -1641,16 +1643,6 @@ impl Editor {
                 // initialize selection for view
                 let doc = doc_mut!(self, &id);
 
-                if let Some((view_position, selection)) = self
-                    .old_file_locs
-                    .get(doc.path().unwrap())
-                    .map(|x| x.to_owned())
-                {
-                    let view = self.tree.get_mut(view_id);
-                    view.offset = view_position;
-                    doc.set_selection(view_id, selection);
-                }
-
                 doc.ensure_view_init(view_id);
                 doc.mark_as_focused();
             }
@@ -1711,9 +1703,14 @@ 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
         } else {
+            new_doc = true;
+
             let mut doc = Document::open(
                 &path,
                 None,
@@ -1737,28 +1734,44 @@ impl Editor {
         };
 
         self.switch(id, action);
+
+        if new_doc {
+            if let Some((view_position, selection)) =
+                self.old_file_locs.get(&path).map(|x| x.to_owned())
+            {
+                let (view, doc) = current!(self);
+                view.offset = view_position;
+                doc.set_selection(view.id, selection);
+            }
+        }
+
         Ok(id)
     }
 
     pub fn close(&mut self, id: ViewId) {
-        let view = self.tree.get(id);
-        // TODO: do something about this unwrap
-        let doc = self.document(view.doc).unwrap();
-        if let Some(path) = doc.path() {
-            // TODO: can the arg here be a reference? would save cloning
-            persistence::push_file_history(FileHistoryEntry::new(
-                path.clone(),
-                view.offset,
-                doc.selection(id).clone(),
-            ));
-            self.old_file_locs
-                .insert(path.to_owned(), (view.offset, doc.selection(id).clone()));
-        };
+        let offset = self.tree.get(id).offset.clone();
+
+        let mut file_locs = Vec::new();
 
         // Remove selections for the closed view on all documents.
         for doc in self.documents_mut() {
+            if let Some(path) = doc.path() {
+                file_locs.push((path.clone(), offset, doc.selection(id).clone()));
+            };
+
             doc.remove_view(id);
         }
+
+        for loc in file_locs {
+            // TODO: can the arg here be a reference? would save cloning
+            persistence::push_file_history(FileHistoryEntry::new(
+                loc.0.clone(),
+                loc.1,
+                loc.2.clone(),
+            ));
+            self.old_file_locs.insert(loc.0, (loc.1, loc.2));
+        }
+
         self.tree.remove(id);
         self._refresh();
     }
@@ -1780,11 +1793,14 @@ impl Editor {
             tokio::spawn(language_server.text_document_did_close(doc.identifier()));
         }
 
+        #[derive(Debug)]
         enum Action {
             Close(ViewId),
             ReplaceDoc(ViewId, DocumentId),
         }
 
+        let mut file_locs = Vec::new();
+
         let actions: Vec<Action> = self
             .tree
             .views_mut()
@@ -1792,6 +1808,10 @@ impl Editor {
                 view.remove_document(&doc_id);
 
                 if view.doc == doc_id {
+                    if let Some(path) = doc.path() {
+                        file_locs.push((path.clone(), view.offset, doc.selection(view.id).clone()));
+                    };
+
                     // something was previously open in the view, switch to previous doc
                     if let Some(prev_doc) = view.docs_access_history.pop() {
                         Some(Action::ReplaceDoc(view.id, prev_doc))
@@ -1805,6 +1825,16 @@ impl Editor {
             })
             .collect();
 
+        for loc in file_locs {
+            // TODO: can the arg here be a reference? would save cloning
+            persistence::push_file_history(FileHistoryEntry::new(
+                loc.0.clone(),
+                loc.1,
+                loc.2.clone(),
+            ));
+            self.old_file_locs.insert(loc.0, (loc.1, loc.2));
+        }
+
         for action in actions {
             match action {
                 Action::Close(view_id) => {