Merge pull request #2016 from zed-industries/serialization-updates

Mikayla Maki created

Serialization touch ups

Change summary

crates/editor/src/editor.rs               |   2 
crates/editor/src/items.rs                |   8 +
crates/editor/src/persistence.rs          |  55 ++++++++++
crates/editor/src/scroll.rs               | 116 ++++++++++++++++++++----
crates/sqlez/src/bindable.rs              |  35 +++++++
crates/sqlez/src/connection.rs            | 101 ++++++++++++++++++---
crates/terminal_view/src/persistence.rs   |   6 
crates/terminal_view/src/terminal_view.rs |   3 
8 files changed, 277 insertions(+), 49 deletions(-)

Detailed changes

crates/editor/src/editor.rs 🔗

@@ -3629,9 +3629,7 @@ impl Editor {
     }
 
     pub fn undo(&mut self, _: &Undo, cx: &mut ViewContext<Self>) {
-        dbg!("undo");
         if let Some(tx_id) = self.buffer.update(cx, |buffer, cx| buffer.undo(cx)) {
-            dbg!(tx_id);
             if let Some((selections, _)) = self.selection_history.transaction(tx_id).cloned() {
                 self.change_selections(None, cx, |s| {
                     s.select_anchors(selections.to_vec());

crates/editor/src/items.rs 🔗

@@ -8,6 +8,7 @@ use anyhow::{anyhow, Context, Result};
 use collections::HashSet;
 use futures::future::try_join_all;
 use futures::FutureExt;
+
 use gpui::{
     elements::*, geometry::vector::vec2f, AppContext, Entity, ModelHandle, MutableAppContext,
     RenderContext, Subscription, Task, View, ViewContext, ViewHandle, WeakViewHandle,
@@ -765,6 +766,7 @@ impl Item for Editor {
     fn added_to_workspace(&mut self, workspace: &mut Workspace, cx: &mut ViewContext<Self>) {
         let workspace_id = workspace.database_id();
         let item_id = cx.view_id();
+        self.workspace_id = Some(workspace_id);
 
         fn serialize(
             buffer: ModelHandle<Buffer>,
@@ -836,7 +838,11 @@ impl Item for Editor {
                         .context("Project item at stored path was not a buffer")?;
 
                     Ok(cx.update(|cx| {
-                        cx.add_view(pane, |cx| Editor::for_buffer(buffer, Some(project), cx))
+                        cx.add_view(pane, |cx| {
+                            let mut editor = Editor::for_buffer(buffer, Some(project), cx);
+                            editor.read_scroll_position_from_db(item_id, workspace_id, cx);
+                            editor
+                        })
                     }))
                 })
             })

crates/editor/src/persistence.rs 🔗

@@ -2,9 +2,19 @@ use std::path::PathBuf;
 
 use db::sqlez_macros::sql;
 use db::{define_connection, query};
+
 use workspace::{ItemId, WorkspaceDb, WorkspaceId};
 
 define_connection!(
+    // Current table shape using pseudo-rust syntax:
+    // editors(
+    //   item_id: usize,
+    //   workspace_id: usize,
+    //   path: PathBuf,
+    //   scroll_top_row: usize,
+    //   scroll_vertical_offset: f32,
+    //   scroll_horizontal_offset: f32,
+    // )
     pub static ref DB: EditorDb<WorkspaceDb> =
         &[sql! (
             CREATE TABLE editors(
@@ -15,8 +25,13 @@ define_connection!(
                 FOREIGN KEY(workspace_id) REFERENCES workspaces(workspace_id)
                 ON DELETE CASCADE
                 ON UPDATE CASCADE
-        ) STRICT;
-    )];
+            ) STRICT;
+        ),
+        sql! (
+            ALTER TABLE editors ADD COLUMN scroll_top_row INTEGER NOT NULL DEFAULT 0;
+            ALTER TABLE editors ADD COLUMN scroll_horizontal_offset REAL NOT NULL DEFAULT 0;
+            ALTER TABLE editors ADD COLUMN scroll_vertical_offset REAL NOT NULL DEFAULT 0;
+        )];
 );
 
 impl EditorDb {
@@ -29,8 +44,40 @@ impl EditorDb {
 
     query! {
         pub async fn save_path(item_id: ItemId, workspace_id: WorkspaceId, path: PathBuf) -> Result<()> {
-            INSERT OR REPLACE INTO editors(item_id, workspace_id, path)
-            VALUES (?, ?, ?)
+            INSERT INTO editors
+                (item_id, workspace_id, path)
+            VALUES
+                (?1, ?2, ?3)
+            ON CONFLICT DO UPDATE SET
+                item_id = ?1,
+                workspace_id = ?2,
+                path = ?3
+        }
+    }
+
+    // Returns the scroll top row, and offset
+    query! {
+        pub fn get_scroll_position(item_id: ItemId, workspace_id: WorkspaceId) -> Result<Option<(u32, f32, f32)>> {
+            SELECT scroll_top_row, scroll_horizontal_offset, scroll_vertical_offset
+            FROM editors
+            WHERE item_id = ? AND workspace_id = ?
+        }
+    }
+
+    query! {
+        pub async fn save_scroll_position(
+            item_id: ItemId,
+            workspace_id: WorkspaceId,
+            top_row: u32,
+            vertical_offset: f32,
+            horizontal_offset: f32
+        ) -> Result<()> {
+            UPDATE OR IGNORE editors
+            SET
+                scroll_top_row = ?3,
+                scroll_horizontal_offset = ?4,
+                scroll_vertical_offset = ?5
+            WHERE item_id = ?1 AND workspace_id = ?2
         }
     }
 }

crates/editor/src/scroll.rs 🔗

@@ -11,11 +11,14 @@ use gpui::{
     geometry::vector::{vec2f, Vector2F},
     Axis, MutableAppContext, Task, ViewContext,
 };
-use language::Bias;
+use language::{Bias, Point};
+use util::ResultExt;
+use workspace::WorkspaceId;
 
 use crate::{
     display_map::{DisplaySnapshot, ToDisplayPoint},
     hover_popover::hide_hover,
+    persistence::DB,
     Anchor, DisplayPoint, Editor, EditorMode, Event, MultiBufferSnapshot, ToPoint,
 };
 
@@ -170,37 +173,68 @@ impl ScrollManager {
         scroll_position: Vector2F,
         map: &DisplaySnapshot,
         local: bool,
+        workspace_id: Option<i64>,
         cx: &mut ViewContext<Editor>,
     ) {
-        let new_anchor = if scroll_position.y() <= 0. {
-            ScrollAnchor {
-                top_anchor: Anchor::min(),
-                offset: scroll_position.max(vec2f(0., 0.)),
-            }
+        let (new_anchor, top_row) = if scroll_position.y() <= 0. {
+            (
+                ScrollAnchor {
+                    top_anchor: Anchor::min(),
+                    offset: scroll_position.max(vec2f(0., 0.)),
+                },
+                0,
+            )
         } else {
-            let scroll_top_buffer_offset =
-                DisplayPoint::new(scroll_position.y() as u32, 0).to_offset(&map, Bias::Right);
+            let scroll_top_buffer_point =
+                DisplayPoint::new(scroll_position.y() as u32, 0).to_point(&map);
             let top_anchor = map
                 .buffer_snapshot
-                .anchor_at(scroll_top_buffer_offset, Bias::Right);
-
-            ScrollAnchor {
-                top_anchor,
-                offset: vec2f(
-                    scroll_position.x(),
-                    scroll_position.y() - top_anchor.to_display_point(&map).row() as f32,
-                ),
-            }
+                .anchor_at(scroll_top_buffer_point, Bias::Right);
+
+            (
+                ScrollAnchor {
+                    top_anchor,
+                    offset: vec2f(
+                        scroll_position.x(),
+                        scroll_position.y() - top_anchor.to_display_point(&map).row() as f32,
+                    ),
+                },
+                scroll_top_buffer_point.row,
+            )
         };
 
-        self.set_anchor(new_anchor, local, cx);
+        self.set_anchor(new_anchor, top_row, local, workspace_id, cx);
     }
 
-    fn set_anchor(&mut self, anchor: ScrollAnchor, local: bool, cx: &mut ViewContext<Editor>) {
+    fn set_anchor(
+        &mut self,
+        anchor: ScrollAnchor,
+        top_row: u32,
+        local: bool,
+        workspace_id: Option<i64>,
+        cx: &mut ViewContext<Editor>,
+    ) {
         self.anchor = anchor;
         cx.emit(Event::ScrollPositionChanged { local });
         self.show_scrollbar(cx);
         self.autoscroll_request.take();
+        if let Some(workspace_id) = workspace_id {
+            let item_id = cx.view_id();
+
+            cx.background()
+                .spawn(async move {
+                    DB.save_scroll_position(
+                        item_id,
+                        workspace_id,
+                        top_row,
+                        anchor.offset.x(),
+                        anchor.offset.y(),
+                    )
+                    .await
+                    .log_err()
+                })
+                .detach()
+        }
         cx.notify();
     }
 
@@ -274,8 +308,13 @@ impl Editor {
         let map = self.display_map.update(cx, |map, cx| map.snapshot(cx));
 
         hide_hover(self, cx);
-        self.scroll_manager
-            .set_scroll_position(scroll_position, &map, local, cx);
+        self.scroll_manager.set_scroll_position(
+            scroll_position,
+            &map,
+            local,
+            self.workspace_id,
+            cx,
+        );
     }
 
     pub fn scroll_position(&self, cx: &mut ViewContext<Self>) -> Vector2F {
@@ -285,7 +324,12 @@ impl Editor {
 
     pub fn set_scroll_anchor(&mut self, scroll_anchor: ScrollAnchor, cx: &mut ViewContext<Self>) {
         hide_hover(self, cx);
-        self.scroll_manager.set_anchor(scroll_anchor, true, cx);
+        let top_row = scroll_anchor
+            .top_anchor
+            .to_point(&self.buffer().read(cx).snapshot(cx))
+            .row;
+        self.scroll_manager
+            .set_anchor(scroll_anchor, top_row, true, self.workspace_id, cx);
     }
 
     pub(crate) fn set_scroll_anchor_remote(
@@ -294,7 +338,12 @@ impl Editor {
         cx: &mut ViewContext<Self>,
     ) {
         hide_hover(self, cx);
-        self.scroll_manager.set_anchor(scroll_anchor, false, cx);
+        let top_row = scroll_anchor
+            .top_anchor
+            .to_point(&self.buffer().read(cx).snapshot(cx))
+            .row;
+        self.scroll_manager
+            .set_anchor(scroll_anchor, top_row, false, self.workspace_id, cx);
     }
 
     pub fn scroll_screen(&mut self, amount: &ScrollAmount, cx: &mut ViewContext<Self>) {
@@ -345,4 +394,25 @@ impl Editor {
 
         Ordering::Greater
     }
+
+    pub fn read_scroll_position_from_db(
+        &mut self,
+        item_id: usize,
+        workspace_id: WorkspaceId,
+        cx: &mut ViewContext<Editor>,
+    ) {
+        let scroll_position = DB.get_scroll_position(item_id, workspace_id);
+        if let Ok(Some((top_row, x, y))) = scroll_position {
+            let top_anchor = self
+                .buffer()
+                .read(cx)
+                .snapshot(cx)
+                .anchor_at(Point::new(top_row as u32, 0), Bias::Left);
+            let scroll_anchor = ScrollAnchor {
+                offset: Vector2F::new(x, y),
+                top_anchor,
+            };
+            self.set_scroll_anchor(scroll_anchor, cx);
+        }
+    }
 }

crates/sqlez/src/bindable.rs 🔗

@@ -89,6 +89,26 @@ impl Column for f64 {
     }
 }
 
+impl Bind for f32 {
+    fn bind(&self, statement: &Statement, start_index: i32) -> Result<i32> {
+        statement
+            .bind_double(start_index, *self as f64)
+            .with_context(|| format!("Failed to bind f64 at index {start_index}"))?;
+        Ok(start_index + 1)
+    }
+}
+
+impl Column for f32 {
+    fn column(statement: &mut Statement, start_index: i32) -> Result<(Self, i32)> {
+        let result = statement
+            .column_double(start_index)
+            .with_context(|| format!("Failed to parse f32 at index {start_index}"))?
+            as f32;
+
+        Ok((result, start_index + 1))
+    }
+}
+
 impl Bind for i32 {
     fn bind(&self, statement: &Statement, start_index: i32) -> Result<i32> {
         statement
@@ -122,6 +142,21 @@ impl Column for i64 {
     }
 }
 
+impl Bind for u32 {
+    fn bind(&self, statement: &Statement, start_index: i32) -> Result<i32> {
+        (*self as i64)
+            .bind(statement, start_index)
+            .with_context(|| format!("Failed to bind usize at index {start_index}"))
+    }
+}
+
+impl Column for u32 {
+    fn column(statement: &mut Statement, start_index: i32) -> Result<(Self, i32)> {
+        let result = statement.column_int64(start_index)?;
+        Ok((result as u32, start_index + 1))
+    }
+}
+
 impl Bind for usize {
     fn bind(&self, statement: &Statement, start_index: i32) -> Result<i32> {
         (*self as i64)

crates/sqlez/src/connection.rs 🔗

@@ -93,36 +93,77 @@ impl Connection {
         let sql_start = remaining_sql.as_ptr();
 
         unsafe {
+            let mut alter_table = None;
             while {
                 let remaining_sql_str = remaining_sql.to_str().unwrap().trim();
-                remaining_sql_str != ";" && !remaining_sql_str.is_empty()
+                let any_remaining_sql = remaining_sql_str != ";" && !remaining_sql_str.is_empty();
+                if any_remaining_sql {
+                    alter_table = parse_alter_table(remaining_sql_str);
+                }
+                any_remaining_sql
             } {
                 let mut raw_statement = ptr::null_mut::<sqlite3_stmt>();
                 let mut remaining_sql_ptr = ptr::null();
-                sqlite3_prepare_v2(
-                    self.sqlite3,
-                    remaining_sql.as_ptr(),
-                    -1,
-                    &mut raw_statement,
-                    &mut remaining_sql_ptr,
-                );
-
-                let res = sqlite3_errcode(self.sqlite3);
-                let offset = sqlite3_error_offset(self.sqlite3);
-                let message = sqlite3_errmsg(self.sqlite3);
+
+                let (res, offset, message, _conn) = if let Some(table_to_alter) = alter_table {
+                    // ALTER TABLE is a weird statement. When preparing the statement the table's
+                    // existence is checked *before* syntax checking any other part of the statement.
+                    // Therefore, we need to make sure that the table has been created before calling
+                    // prepare. As we don't want to trash whatever database this is connected to, we
+                    // create a new in-memory DB to test.
+
+                    let temp_connection = Connection::open_memory(None);
+                    //This should always succeed, if it doesn't then you really should know about it
+                    temp_connection
+                        .exec(&format!(
+                        "CREATE TABLE {table_to_alter}(__place_holder_column_for_syntax_checking)"
+                    ))
+                        .unwrap()()
+                    .unwrap();
+
+                    sqlite3_prepare_v2(
+                        temp_connection.sqlite3,
+                        remaining_sql.as_ptr(),
+                        -1,
+                        &mut raw_statement,
+                        &mut remaining_sql_ptr,
+                    );
+
+                    (
+                        sqlite3_errcode(temp_connection.sqlite3),
+                        sqlite3_error_offset(temp_connection.sqlite3),
+                        sqlite3_errmsg(temp_connection.sqlite3),
+                        Some(temp_connection),
+                    )
+                } else {
+                    sqlite3_prepare_v2(
+                        self.sqlite3,
+                        remaining_sql.as_ptr(),
+                        -1,
+                        &mut raw_statement,
+                        &mut remaining_sql_ptr,
+                    );
+                    (
+                        sqlite3_errcode(self.sqlite3),
+                        sqlite3_error_offset(self.sqlite3),
+                        sqlite3_errmsg(self.sqlite3),
+                        None,
+                    )
+                };
 
                 sqlite3_finalize(raw_statement);
 
                 if res == 1 && offset >= 0 {
+                    let sub_statement_correction =
+                        remaining_sql.as_ptr() as usize - sql_start as usize;
                     let err_msg =
                         String::from_utf8_lossy(CStr::from_ptr(message as *const _).to_bytes())
                             .into_owned();
-                    let sub_statement_correction =
-                        remaining_sql.as_ptr() as usize - sql_start as usize;
 
                     return Some((err_msg, offset as usize + sub_statement_correction));
                 }
                 remaining_sql = CStr::from_ptr(remaining_sql_ptr);
+                alter_table = None;
             }
         }
         None
@@ -162,6 +203,25 @@ impl Connection {
     }
 }
 
+fn parse_alter_table(remaining_sql_str: &str) -> Option<String> {
+    let remaining_sql_str = remaining_sql_str.to_lowercase();
+    if remaining_sql_str.starts_with("alter") {
+        if let Some(table_offset) = remaining_sql_str.find("table") {
+            let after_table_offset = table_offset + "table".len();
+            let table_to_alter = remaining_sql_str
+                .chars()
+                .skip(after_table_offset)
+                .skip_while(|c| c.is_whitespace())
+                .take_while(|c| !c.is_whitespace())
+                .collect::<String>();
+            if !table_to_alter.is_empty() {
+                return Some(table_to_alter);
+            }
+        }
+    }
+    None
+}
+
 impl Drop for Connection {
     fn drop(&mut self) {
         unsafe { sqlite3_close(self.sqlite3) };
@@ -331,4 +391,17 @@ mod test {
 
         assert_eq!(res, Some(first_stmt.len() + second_offset + 1));
     }
+
+    #[test]
+    fn test_alter_table_syntax() {
+        let connection = Connection::open_memory(Some("test_alter_table_syntax"));
+
+        assert!(connection
+            .sql_has_syntax_error("ALTER TABLE test ADD x TEXT")
+            .is_none());
+
+        assert!(connection
+            .sql_has_syntax_error("ALTER TABLE test AAD x TEXT")
+            .is_some());
+    }
 }

crates/terminal_view/src/persistence.rs 🔗

@@ -42,10 +42,10 @@ impl TerminalDb {
     }
 
     query! {
-        pub async fn take_working_directory(item_id: ItemId, workspace_id: WorkspaceId) -> Result<Option<PathBuf>> {
-            DELETE FROM terminals
+        pub fn get_working_directory(item_id: ItemId, workspace_id: WorkspaceId) -> Result<Option<PathBuf>> {
+            SELECT working_directory
+            FROM terminals
             WHERE item_id = ? AND workspace_id = ?
-            RETURNING working_directory
         }
     }
 }

crates/terminal_view/src/terminal_view.rs 🔗

@@ -706,8 +706,7 @@ impl Item for TerminalView {
         let window_id = cx.window_id();
         cx.spawn(|pane, mut cx| async move {
             let cwd = TERMINAL_DB
-                .take_working_directory(item_id, workspace_id)
-                .await
+                .get_working_directory(item_id, workspace_id)
                 .log_err()
                 .flatten();