Finished refactor for mutable terminal and long-single-lock style. Currently terminal is deadlocking instantly, need to just do the full refactor

Mikayla Maki created

Change summary

crates/terminal/src/connected_el.rs                | 113 ++++++++-------
crates/terminal/src/connected_view.rs              |  34 ++--
crates/terminal/src/terminal.rs                    |  59 +++++--
crates/terminal/src/tests/terminal_test_context.rs |  21 +-
4 files changed, 125 insertions(+), 102 deletions(-)

Detailed changes

crates/terminal/src/connected_el.rs 🔗

@@ -512,10 +512,10 @@ impl Element for TerminalEl {
         cx: &mut gpui::LayoutContext,
     ) -> (gpui::geometry::vector::Vector2F, Self::LayoutState) {
         let settings = cx.global::<Settings>();
-        let font_cache = &cx.font_cache();
+        let font_cache = cx.font_cache();
 
         //Setup layout information
-        let terminal_theme = &settings.theme.terminal;
+        let terminal_theme = settings.theme.terminal.clone(); //-_-
         let text_style = TerminalEl::make_text_style(font_cache, &settings);
         let selection_color = settings.theme.editor.selection.selection;
         let dimensions = {
@@ -524,62 +524,64 @@ impl Element for TerminalEl {
             TermDimensions::new(line_height, cell_width, constraint.max)
         };
 
-        let terminal = self.terminal.upgrade(cx).unwrap().read(cx);
+        let background_color = if self.modal {
+            terminal_theme.colors.modal_background.clone()
+        } else {
+            terminal_theme.colors.background.clone()
+        };
 
         let (cursor, cells, rects, highlights) =
-            terminal.render_lock(Some(dimensions.clone()), |content, cursor_text| {
-                let (cells, rects, highlights) = TerminalEl::layout_grid(
-                    content.display_iter,
-                    &text_style,
-                    terminal_theme,
-                    cx.text_layout_cache,
-                    self.modal,
-                    content.selection,
-                );
-
-                //Layout cursor
-                let cursor = {
-                    let cursor_point =
-                        DisplayCursor::from(content.cursor.point, content.display_offset);
-                    let cursor_text = {
-                        let str_trxt = cursor_text.to_string();
-                        cx.text_layout_cache.layout_str(
-                            &str_trxt,
-                            text_style.font_size,
-                            &[(
-                                str_trxt.len(),
-                                RunStyle {
-                                    font_id: text_style.font_id,
-                                    color: terminal_theme.colors.background,
-                                    underline: Default::default(),
-                                },
-                            )],
-                        )
-                    };
+            self.terminal
+                .upgrade(cx)
+                .unwrap()
+                .update(cx.app, |terminal, mcx| {
+                    terminal.render_lock(mcx, |content, cursor_text| {
+                        let (cells, rects, highlights) = TerminalEl::layout_grid(
+                            content.display_iter,
+                            &text_style,
+                            &terminal_theme,
+                            cx.text_layout_cache,
+                            self.modal,
+                            content.selection,
+                        );
 
-                    TerminalEl::shape_cursor(cursor_point, dimensions, &cursor_text).map(
-                        move |(cursor_position, block_width)| {
-                            Cursor::new(
-                                cursor_position,
-                                block_width,
-                                dimensions.line_height,
-                                terminal_theme.colors.cursor,
-                                CursorShape::Block,
-                                Some(cursor_text.clone()),
+                        //Layout cursor
+                        let cursor = {
+                            let cursor_point =
+                                DisplayCursor::from(content.cursor.point, content.display_offset);
+                            let cursor_text = {
+                                let str_trxt = cursor_text.to_string();
+                                cx.text_layout_cache.layout_str(
+                                    &str_trxt,
+                                    text_style.font_size,
+                                    &[(
+                                        str_trxt.len(),
+                                        RunStyle {
+                                            font_id: text_style.font_id,
+                                            color: terminal_theme.colors.background,
+                                            underline: Default::default(),
+                                        },
+                                    )],
+                                )
+                            };
+
+                            TerminalEl::shape_cursor(cursor_point, dimensions, &cursor_text).map(
+                                move |(cursor_position, block_width)| {
+                                    Cursor::new(
+                                        cursor_position,
+                                        block_width,
+                                        dimensions.line_height,
+                                        terminal_theme.colors.cursor,
+                                        CursorShape::Block,
+                                        Some(cursor_text.clone()),
+                                    )
+                                },
                             )
-                        },
-                    )
-                };
-
-                (cursor, cells, rects, highlights)
-            });
+                        };
 
-        //Select background color
-        let background_color = if self.modal {
-            terminal_theme.colors.modal_background
-        } else {
-            terminal_theme.colors.background
-        };
+                        (cursor, cells, rects, highlights)
+                    })
+                });
 
         //Done!
         (
@@ -706,8 +708,9 @@ impl Element for TerminalEl {
 
                 self.terminal
                     .upgrade(cx.app)
-                    .map(|model_handle| model_handle.read(cx.app))
-                    .map(|term| term.try_keystroke(keystroke))
+                    .map(|model_handle| {
+                        model_handle.update(cx.app, |term, _| term.try_keystroke(keystroke))
+                    })
                     .unwrap_or(false)
             }
             _ => false,

crates/terminal/src/connected_view.rs 🔗

@@ -87,7 +87,7 @@ impl ConnectedView {
     }
 
     fn clear(&mut self, _: &Clear, cx: &mut ViewContext<Self>) {
-        self.terminal.read(cx).clear();
+        self.terminal.update(cx, |term, _| term.clear());
     }
 
     ///Attempt to paste the clipboard into the terminal
@@ -101,43 +101,43 @@ impl ConnectedView {
     ///Attempt to paste the clipboard into the terminal
     fn paste(&mut self, _: &Paste, cx: &mut ViewContext<Self>) {
         cx.read_from_clipboard().map(|item| {
-            self.terminal.read(cx).paste(item.text());
+            self.terminal.update(cx, |term, _| term.paste(item.text()));
         });
     }
 
     ///Synthesize the keyboard event corresponding to 'up'
     fn up(&mut self, _: &Up, cx: &mut ViewContext<Self>) {
-        self.terminal
-            .read(cx)
-            .try_keystroke(&Keystroke::parse("up").unwrap());
+        self.terminal.update(cx, |term, _| {
+            term.try_keystroke(&Keystroke::parse("up").unwrap());
+        });
     }
 
     ///Synthesize the keyboard event corresponding to 'down'
     fn down(&mut self, _: &Down, cx: &mut ViewContext<Self>) {
-        self.terminal
-            .read(cx)
-            .try_keystroke(&Keystroke::parse("down").unwrap());
+        self.terminal.update(cx, |term, _| {
+            term.try_keystroke(&Keystroke::parse("down").unwrap());
+        });
     }
 
     ///Synthesize the keyboard event corresponding to 'ctrl-c'
     fn ctrl_c(&mut self, _: &CtrlC, cx: &mut ViewContext<Self>) {
-        self.terminal
-            .read(cx)
-            .try_keystroke(&Keystroke::parse("ctrl-c").unwrap());
+        self.terminal.update(cx, |term, _| {
+            term.try_keystroke(&Keystroke::parse("ctrl-c").unwrap());
+        });
     }
 
     ///Synthesize the keyboard event corresponding to 'escape'
     fn escape(&mut self, _: &Escape, cx: &mut ViewContext<Self>) {
-        self.terminal
-            .read(cx)
-            .try_keystroke(&Keystroke::parse("escape").unwrap());
+        self.terminal.update(cx, |term, _| {
+            term.try_keystroke(&Keystroke::parse("escape").unwrap());
+        });
     }
 
     ///Synthesize the keyboard event corresponding to 'enter'
     fn enter(&mut self, _: &Enter, cx: &mut ViewContext<Self>) {
-        self.terminal
-            .read(cx)
-            .try_keystroke(&Keystroke::parse("enter").unwrap());
+        self.terminal.update(cx, |term, _| {
+            term.try_keystroke(&Keystroke::parse("enter").unwrap());
+        });
     }
 }
 

crates/terminal/src/terminal.rs 🔗

@@ -4,6 +4,11 @@ pub mod mappings;
 pub mod modal;
 pub mod terminal_view;
 
+#[cfg(test)]
+use alacritty_terminal::term::cell::Cell;
+#[cfg(test)]
+use alacritty_terminal::Grid;
+
 use alacritty_terminal::{
     ansi::{ClearMode, Handler},
     config::{Config, Program, PtyConfig},
@@ -383,13 +388,11 @@ impl Terminal {
         &mut self,
         event: alacritty_terminal::event::Event,
         term: &mut Term<ZedListener>,
-        cx: &mut ModelContext<Terminal>,
+        cx: &mut ModelContext<Self>,
     ) {
         match event {
             // TODO: Handle is_self_focused in subscription on terminal view
-            AlacTermEvent::Wakeup => {
-                cx.emit(Event::Wakeup);
-            }
+            AlacTermEvent::Wakeup => { /* Irrelevant, as we always notify on any event */ }
             AlacTermEvent::PtyWrite(out) => {
                 term.scroll_display(Scroll::Bottom);
                 self.pty_tx.notify(out.into_bytes())
@@ -411,17 +414,20 @@ impl Terminal {
             AlacTermEvent::ClipboardStore(_, data) => {
                 cx.write_to_clipboard(ClipboardItem::new(data))
             }
-            AlacTermEvent::ClipboardLoad(_, format) => self.write_to_pty(format(
-                &cx.read_from_clipboard()
-                    .map(|ci| ci.text().to_string())
-                    .unwrap_or("".to_string()),
-            )),
+            AlacTermEvent::ClipboardLoad(_, format) => self.pty_tx.notify(
+                format(
+                    &cx.read_from_clipboard()
+                        .map(|ci| ci.text().to_string())
+                        .unwrap_or("".to_string()),
+                )
+                .into_bytes(),
+            ),
             AlacTermEvent::ColorRequest(index, format) => {
                 let color = term.colors()[index].unwrap_or_else(|| {
                     let term_style = &cx.global::<Settings>().theme.terminal;
                     to_alac_rgb(get_color_at_index(&index, &term_style.colors))
                 });
-                self.write_to_pty(format(color))
+                self.pty_tx.notify(format(color).into_bytes())
             }
             AlacTermEvent::CursorBlinkingChange => {
                 //TODO: Set a timer to blink the cursor on and off
@@ -435,7 +441,7 @@ impl Terminal {
     }
 
     ///Write the Input payload to the tty. This locks the terminal so we can scroll it.
-    pub fn write_to_pty(&self, input: String) {
+    pub fn write_to_pty(&mut self, input: String) {
         self.event_stack.push(AlacTermEvent::PtyWrite(input))
     }
 
@@ -447,12 +453,12 @@ impl Terminal {
         self.term.lock().resize(term_size);
     }
 
-    pub fn clear(&self) {
+    pub fn clear(&mut self) {
         self.write_to_pty("\x0c".into());
         self.term.lock().clear_screen(ClearMode::Saved);
     }
 
-    pub fn try_keystroke(&self, keystroke: &Keystroke) -> bool {
+    pub fn try_keystroke(&mut self, keystroke: &Keystroke) -> bool {
         let guard = self.term.lock();
         let mode = guard.mode();
         let esc = to_esc_str(keystroke, mode);
@@ -466,7 +472,7 @@ impl Terminal {
     }
 
     ///Paste text into the terminal
-    pub fn paste(&self, text: &str) {
+    pub fn paste(&mut self, text: &str) {
         if self.term.lock().mode().contains(TermMode::BRACKETED_PASTE) {
             self.write_to_pty("\x1b[200~".to_string());
             self.write_to_pty(text.replace('\x1b', "").to_string());
@@ -490,11 +496,12 @@ impl Terminal {
         self.term.lock().selection = sel;
     }
 
-    pub fn render_lock<F, T>(&self, cx: &mut ModelContext<Self>, f: F) -> T
+    pub fn render_lock<F, T>(&mut self, cx: &mut ModelContext<Self>, f: F) -> T
     where
         F: FnOnce(RenderableContent, char) -> T,
     {
-        let mut term = self.term.lock(); //Lock
+        let m = self.term.clone(); //TODO avoid clone?
+        let mut term = m.lock(); //Lock
 
         //TODO, handle resizes
         // if let Some(new_size) = new_size {
@@ -505,7 +512,13 @@ impl Terminal {
         //     term.resize(new_size); //Reflow
         // }
 
-        for event in self.event_stack.drain(..) {
+        for event in self
+            .event_stack
+            .iter()
+            .map(|event| event.clone())
+            .collect::<Vec<AlacTermEvent>>() //TODO avoid copy
+            .drain(..)
+        {
             self.process_terminal_event(event, &mut term, cx)
         }
 
@@ -516,12 +529,13 @@ impl Terminal {
     }
 
     pub fn get_display_offset(&self) -> usize {
-        self.term.lock().renderable_content().display_offset
+        10
+        // self.term.lock().renderable_content().display_offset
     }
 
     ///Scroll the terminal
-    pub fn scroll(&self, scroll: Scroll) {
-        self.term.lock().scroll_display(scroll)
+    pub fn scroll(&self, _scroll: Scroll) {
+        // self.term.lock().scroll_display(scroll)
     }
 
     pub fn click(&self, point: Point, side: Direction, clicks: usize) {
@@ -549,6 +563,11 @@ impl Terminal {
     pub fn mouse_down(&self, point: Point, side: Direction) {
         self.set_selection(Some(Selection::new(SelectionType::Simple, point, side)));
     }
+
+    #[cfg(test)]
+    fn grid(&self) -> Grid<Cell> {
+        self.term.lock().grid().clone()
+    }
 }
 
 impl Drop for Terminal {

crates/terminal/src/tests/terminal_test_context.rs 🔗

@@ -43,8 +43,9 @@ impl<'a> TerminalTestContext<'a> {
         });
 
         connection
-            .condition(self.cx, |conn, cx| {
-                let content = Self::grid_as_str(conn);
+            .condition(self.cx, |term, cx| {
+                let content = Self::grid_as_str(term);
+
                 f(content, cx)
             })
             .await;
@@ -132,14 +133,14 @@ impl<'a> TerminalTestContext<'a> {
     }
 
     fn grid_as_str(connection: &Terminal) -> String {
-        connection.render_lock(None, |content, _| {
-            let lines = content.display_iter.group_by(|i| i.point.line.0);
-            lines
-                .into_iter()
-                .map(|(_, line)| line.map(|i| i.c).collect::<String>())
-                .collect::<Vec<String>>()
-                .join("\n")
-        })
+        let content = connection.grid();
+
+        let lines = content.display_iter().group_by(|i| i.point.line.0);
+        lines
+            .into_iter()
+            .map(|(_, line)| line.map(|i| i.c).collect::<String>())
+            .collect::<Vec<String>>()
+            .join("\n")
     }
 }