Merge pull request #1349 from zed-industries/connection-refactor

Mikayla Maki created

Terminal Connection touch up

Change summary

assets/keymaps/default.json             |  4 +
crates/terminal/src/connection.rs       | 37 ++++++++----
crates/terminal/src/modal.rs            |  7 ++
crates/terminal/src/terminal.rs         | 77 ++++++++++++++------------
crates/terminal/src/terminal_element.rs | 57 +++++++++++++++----
styles/package-lock.json                |  1 
6 files changed, 116 insertions(+), 67 deletions(-)

Detailed changes

assets/keymaps/default.json 🔗

@@ -409,6 +409,7 @@
         "bindings": {
             "ctrl-c": "terminal::Sigint",
             "escape": "terminal::Escape",
+            "shift-escape": "terminal::DeployModal",
             "ctrl-d": "terminal::Quit",
             "backspace": "terminal::Del",
             "enter": "terminal::Return",
@@ -418,7 +419,8 @@
             "down": "terminal::Down",
             "tab": "terminal::Tab",
             "cmd-v": "terminal::Paste",
-            "cmd-c": "terminal::Copy"
+            "cmd-c": "terminal::Copy",
+            "ctrl-l": "terminal::Clear"
         }
     }
 ]

crates/terminal/src/connection.rs 🔗

@@ -1,4 +1,5 @@
 use alacritty_terminal::{
+    ansi::{ClearMode, Handler},
     config::{Config, PtyConfig},
     event::{Event as AlacTermEvent, Notify},
     event_loop::{EventLoop, Msg, Notifier},
@@ -120,10 +121,10 @@ impl TerminalConnection {
             AlacTermEvent::Wakeup => {
                 cx.emit(Event::Wakeup);
             }
-            AlacTermEvent::PtyWrite(out) => self.write_to_pty(out, cx),
+            AlacTermEvent::PtyWrite(out) => self.write_to_pty(out),
             AlacTermEvent::MouseCursorDirty => {
                 //Calculate new cursor style.
-                //TODO
+                //TODO: alacritty/src/input.rs:L922-L939
                 //Check on correctly handling mouse events for terminals
                 cx.platform().set_cursor_style(CursorStyle::Arrow); //???
             }
@@ -138,20 +139,17 @@ impl TerminalConnection {
             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()),
-                ),
-                cx,
-            ),
+            AlacTermEvent::ClipboardLoad(_, format) => self.write_to_pty(format(
+                &cx.read_from_clipboard()
+                    .map(|ci| ci.text().to_string())
+                    .unwrap_or("".to_string()),
+            )),
             AlacTermEvent::ColorRequest(index, format) => {
                 let color = self.term.lock().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), cx)
+                self.write_to_pty(format(color))
             }
             AlacTermEvent::CursorBlinkingChange => {
                 //TODO: Set a timer to blink the cursor on and off
@@ -164,15 +162,26 @@ impl TerminalConnection {
     }
 
     ///Write the Input payload to the tty. This locks the terminal so we can scroll it.
-    pub fn write_to_pty(&mut self, input: String, cx: &mut ModelContext<Self>) {
-        self.write_bytes_to_pty(input.into_bytes(), cx);
+    pub fn write_to_pty(&mut self, input: String) {
+        self.write_bytes_to_pty(input.into_bytes());
     }
 
     ///Write the Input payload to the tty. This locks the terminal so we can scroll it.
-    fn write_bytes_to_pty(&mut self, input: Vec<u8>, _: &mut ModelContext<Self>) {
+    fn write_bytes_to_pty(&mut self, input: Vec<u8>) {
         self.term.lock().scroll_display(Scroll::Bottom);
         self.pty_tx.notify(input);
     }
+
+    ///Resize the terminal and the PTY. This locks the terminal.
+    pub fn set_size(&mut self, new_size: SizeInfo) {
+        self.pty_tx.0.send(Msg::Resize(new_size)).ok();
+        self.term.lock().resize(new_size);
+    }
+
+    pub fn clear(&mut self) {
+        self.write_to_pty("\x0c".into());
+        self.term.lock().clear_screen(ClearMode::Saved);
+    }
 }
 
 impl Drop for TerminalConnection {

crates/terminal/src/modal.rs 🔗

@@ -3,6 +3,7 @@ use workspace::Workspace;
 
 use crate::{get_wd_for_workspace, DeployModal, Event, Terminal, TerminalConnection};
 
+#[derive(Debug)]
 struct StoredConnection(ModelHandle<TerminalConnection>);
 
 pub fn deploy_modal(workspace: &mut Workspace, _: &DeployModal, cx: &mut ViewContext<Workspace>) {
@@ -24,12 +25,18 @@ pub fn deploy_modal(workspace: &mut Workspace, _: &DeployModal, cx: &mut ViewCon
             let this = cx.add_view(|cx| Terminal::new(wd, true, cx));
             let connection_handle = this.read(cx).connection.clone();
             cx.subscribe(&connection_handle, on_event).detach();
+            //Set the global immediately, in case the user opens the command palette
+            cx.set_global::<Option<StoredConnection>>(Some(StoredConnection(
+                connection_handle.clone(),
+            )));
             this
         }) {
             let connection = closed_terminal_handle.read(cx).connection.clone();
             cx.set_global(Some(StoredConnection(connection)));
         }
     }
+
+    //The problem is that the terminal modal is never re-stored.
 }
 
 pub fn on_event(

crates/terminal/src/terminal.rs 🔗

@@ -1,11 +1,10 @@
-pub mod color_translation;
+mod color_translation;
 pub mod connection;
 mod modal;
 pub mod terminal_element;
 
 use alacritty_terminal::{
     event::{Event as AlacTermEvent, EventListener},
-    event_loop::Msg,
     grid::Scroll,
     term::SizeInfo,
 };
@@ -89,6 +88,7 @@ pub fn init(cx: &mut MutableAppContext) {
     cx.add_action(Terminal::paste);
     cx.add_action(Terminal::scroll_terminal);
     cx.add_action(Terminal::input);
+    cx.add_action(Terminal::clear);
     cx.add_action(deploy_modal);
 }
 
@@ -168,14 +168,6 @@ impl Terminal {
         }
     }
 
-    ///Resize the terminal and the PTY. This locks the terminal.
-    fn set_size(&self, new_size: SizeInfo, cx: &mut MutableAppContext) {
-        self.connection.update(cx, |connection, _| {
-            connection.pty_tx.0.send(Msg::Resize(new_size)).ok();
-            connection.term.lock().resize(new_size);
-        })
-    }
-
     ///Scroll the terminal. This locks the terminal
     fn scroll_terminal(&mut self, scroll: &ScrollTerminal, cx: &mut ViewContext<Self>) {
         self.connection
@@ -186,8 +178,9 @@ impl Terminal {
     }
 
     fn input(&mut self, Input(text): &Input, cx: &mut ViewContext<Self>) {
-        self.connection.update(cx, |connection, cx| {
-            connection.write_to_pty(text.clone(), cx);
+        self.connection.update(cx, |connection, _| {
+            //TODO: This is probably not encoding UTF8 correctly (see alacritty/src/input.rs:L825-837)
+            connection.write_to_pty(text.clone());
         });
 
         if self.has_bell {
@@ -196,6 +189,11 @@ impl Terminal {
         }
     }
 
+    fn clear(&mut self, _: &Clear, cx: &mut ViewContext<Self>) {
+        self.connection
+            .update(cx, |connection, _| connection.clear());
+    }
+
     ///Create a new Terminal in the current working directory or the user's home directory
     fn deploy(workspace: &mut Workspace, _: &Deploy, cx: &mut ViewContext<Workspace>) {
         let wd = get_wd_for_workspace(workspace, cx);
@@ -220,72 +218,72 @@ impl Terminal {
     ///Attempt to paste the clipboard into the terminal
     fn paste(&mut self, _: &Paste, cx: &mut ViewContext<Self>) {
         if let Some(item) = cx.read_from_clipboard() {
-            self.connection.update(cx, |connection, cx| {
-                connection.write_to_pty(item.text().to_owned(), cx);
+            self.connection.update(cx, |connection, _| {
+                connection.write_to_pty(item.text().to_owned());
             })
         }
     }
 
     ///Send the `up` key
     fn up(&mut self, _: &Up, cx: &mut ViewContext<Self>) {
-        self.connection.update(cx, |connection, cx| {
-            connection.write_to_pty(UP_SEQ.to_string(), cx);
+        self.connection.update(cx, |connection, _| {
+            connection.write_to_pty(UP_SEQ.to_string());
         });
     }
 
     ///Send the `down` key
     fn down(&mut self, _: &Down, cx: &mut ViewContext<Self>) {
-        self.connection.update(cx, |connection, cx| {
-            connection.write_to_pty(DOWN_SEQ.to_string(), cx);
+        self.connection.update(cx, |connection, _| {
+            connection.write_to_pty(DOWN_SEQ.to_string());
         });
     }
 
     ///Send the `tab` key
     fn tab(&mut self, _: &Tab, cx: &mut ViewContext<Self>) {
-        self.connection.update(cx, |connection, cx| {
-            connection.write_to_pty(TAB_CHAR.to_string(), cx);
+        self.connection.update(cx, |connection, _| {
+            connection.write_to_pty(TAB_CHAR.to_string());
         });
     }
 
     ///Send `SIGINT` (`ctrl-c`)
     fn send_sigint(&mut self, _: &Sigint, cx: &mut ViewContext<Self>) {
-        self.connection.update(cx, |connection, cx| {
-            connection.write_to_pty(ETX_CHAR.to_string(), cx);
+        self.connection.update(cx, |connection, _| {
+            connection.write_to_pty(ETX_CHAR.to_string());
         });
     }
 
     ///Send the `escape` key
     fn escape(&mut self, _: &Escape, cx: &mut ViewContext<Self>) {
-        self.connection.update(cx, |connection, cx| {
-            connection.write_to_pty(ESC_CHAR.to_string(), cx);
+        self.connection.update(cx, |connection, _| {
+            connection.write_to_pty(ESC_CHAR.to_string());
         });
     }
 
     ///Send the `delete` key. TODO: Difference between this and backspace?
     fn del(&mut self, _: &Del, cx: &mut ViewContext<Self>) {
-        self.connection.update(cx, |connection, cx| {
-            connection.write_to_pty(DEL_CHAR.to_string(), cx);
+        self.connection.update(cx, |connection, _| {
+            connection.write_to_pty(DEL_CHAR.to_string());
         });
     }
 
     ///Send a carriage return. TODO: May need to check the terminal mode.
     fn carriage_return(&mut self, _: &Return, cx: &mut ViewContext<Self>) {
-        self.connection.update(cx, |connection, cx| {
-            connection.write_to_pty(CARRIAGE_RETURN_CHAR.to_string(), cx);
+        self.connection.update(cx, |connection, _| {
+            connection.write_to_pty(CARRIAGE_RETURN_CHAR.to_string());
         });
     }
 
     //Send the `left` key
     fn left(&mut self, _: &Left, cx: &mut ViewContext<Self>) {
-        self.connection.update(cx, |connection, cx| {
-            connection.write_to_pty(LEFT_SEQ.to_string(), cx);
+        self.connection.update(cx, |connection, _| {
+            connection.write_to_pty(LEFT_SEQ.to_string());
         });
     }
 
     //Send the `right` key
     fn right(&mut self, _: &Right, cx: &mut ViewContext<Self>) {
-        self.connection.update(cx, |connection, cx| {
-            connection.write_to_pty(RIGHT_SEQ.to_string(), cx);
+        self.connection.update(cx, |connection, _| {
+            connection.write_to_pty(RIGHT_SEQ.to_string());
         });
     }
 }
@@ -296,7 +294,12 @@ impl View for Terminal {
     }
 
     fn render(&mut self, cx: &mut gpui::RenderContext<'_, Self>) -> ElementBox {
-        let element = TerminalEl::new(cx.handle()).contained();
+        let element = {
+            let connection_handle = self.connection.clone().downgrade();
+            let view_id = cx.view_id();
+            TerminalEl::new(view_id, connection_handle, self.modal).contained()
+        };
+
         if self.modal {
             let settings = cx.global::<Settings>();
             let container_style = settings.theme.terminal.modal_container;
@@ -470,8 +473,8 @@ mod tests {
         let terminal = cx.add_view(Default::default(), |cx| Terminal::new(None, false, cx));
 
         terminal.update(cx, |terminal, cx| {
-            terminal.connection.update(cx, |connection, cx| {
-                connection.write_to_pty("expr 3 + 4".to_string(), cx);
+            terminal.connection.update(cx, |connection, _| {
+                connection.write_to_pty("expr 3 + 4".to_string());
             });
             terminal.carriage_return(&Return, cx);
         });
@@ -495,8 +498,8 @@ mod tests {
         cx.set_condition_duration(Some(Duration::from_secs(2)));
 
         terminal.update(cx, |terminal, cx| {
-            terminal.connection.update(cx, |connection, cx| {
-                connection.write_to_pty("expr 3 + 4".to_string(), cx);
+            terminal.connection.update(cx, |connection, _| {
+                connection.write_to_pty("expr 3 + 4".to_string());
             });
             terminal.carriage_return(&Return, cx);
         });

crates/terminal/src/terminal_element.rs 🔗

@@ -21,7 +21,7 @@ use gpui::{
     json::json,
     text_layout::{Line, RunStyle},
     Event, FontCache, KeyDownEvent, MouseRegion, PaintContext, Quad, ScrollWheelEvent,
-    SizeConstraint, TextLayoutCache, WeakViewHandle,
+    SizeConstraint, TextLayoutCache, WeakModelHandle,
 };
 use itertools::Itertools;
 use ordered_float::OrderedFloat;
@@ -31,7 +31,9 @@ use theme::TerminalStyle;
 use std::{cmp::min, ops::Range, rc::Rc, sync::Arc};
 use std::{fmt::Debug, ops::Sub};
 
-use crate::{color_translation::convert_color, ScrollTerminal, Terminal, ZedListener};
+use crate::{
+    color_translation::convert_color, connection::TerminalConnection, ScrollTerminal, ZedListener,
+};
 
 ///Scrolling is unbearably sluggish by default. Alacritty supports a configurable
 ///Scroll multiplier that is set to 3 by default. This will be removed when I
@@ -44,8 +46,11 @@ const ALACRITTY_SCROLL_MULTIPLIER: f32 = 3.;
 const DEBUG_GRID: bool = false;
 
 ///The GPUI element that paints the terminal.
+///We need to keep a reference to the view for mouse events, do we need it for any other terminal stuff, or can we move that to connection?
 pub struct TerminalEl {
-    view: WeakViewHandle<Terminal>,
+    connection: WeakModelHandle<TerminalConnection>,
+    view_id: usize,
+    modal: bool,
 }
 
 ///New type pattern so I don't mix these two up
@@ -95,8 +100,16 @@ pub struct LayoutState {
 }
 
 impl TerminalEl {
-    pub fn new(view: WeakViewHandle<Terminal>) -> TerminalEl {
-        TerminalEl { view }
+    pub fn new(
+        view_id: usize,
+        connection: WeakModelHandle<TerminalConnection>,
+        modal: bool,
+    ) -> TerminalEl {
+        TerminalEl {
+            view_id,
+            connection,
+            modal,
+        }
     }
 }
 
@@ -118,22 +131,19 @@ impl Element for TerminalEl {
             cx.font_cache()
                 .em_advance(text_style.font_id, text_style.font_size),
         );
-        let view_handle = self.view.upgrade(cx).unwrap();
+        let connection_handle = self.connection.upgrade(cx).unwrap();
 
         //Tell the view our new size. Requires a mutable borrow of cx and the view
         let cur_size = make_new_size(constraint, &cell_width, &line_height);
         //Note that set_size locks and mutates the terminal.
-        view_handle.update(cx.app, |view, cx| view.set_size(cur_size, cx));
-
-        //Now that we're done with the mutable portion, grab the immutable settings and view again
-        let view = view_handle.read(cx);
+        connection_handle.update(cx.app, |connection, _| connection.set_size(cur_size));
 
         let (selection_color, terminal_theme) = {
             let theme = &(cx.global::<Settings>()).theme;
             (theme.editor.selection.selection, &theme.terminal)
         };
 
-        let terminal_mutex = view_handle.read(cx).connection.read(cx).term.clone();
+        let terminal_mutex = connection_handle.read(cx).term.clone();
         let term = terminal_mutex.lock();
         let grid = term.grid();
         let cursor_point = grid.cursor.point;
@@ -146,7 +156,7 @@ impl Element for TerminalEl {
             &text_style,
             terminal_theme,
             cx.text_layout_cache,
-            view.modal,
+            self.modal,
             content.selection,
         );
 
@@ -190,7 +200,7 @@ impl Element for TerminalEl {
         });
         drop(term);
 
-        let background_color = if view.modal {
+        let background_color = if self.modal {
             terminal_theme.colors.modal_background
         } else {
             terminal_theme.colors.background
@@ -229,7 +239,7 @@ impl Element for TerminalEl {
             attach_mouse_handlers(
                 origin,
                 cur_size,
-                self.view.id(),
+                self.view_id,
                 &layout.terminal,
                 visible_bounds,
                 cx,
@@ -349,6 +359,25 @@ impl Element for TerminalEl {
         _paint: &mut Self::PaintState,
         cx: &mut gpui::EventContext,
     ) -> bool {
+        //The problem:
+        //Depending on the terminal mode, we either send an escape sequence
+        //OR update our own data structures.
+        //e.g. scrolling. If we do smooth scrolling, then we need to check if
+        //we own scrolling and then if so, do our scrolling thing.
+        //Ok, so the terminal connection should have APIs for querying it semantically
+        //something like `should_handle_scroll()`. This means we need a handle to the connection.
+        //Actually, this is the only time that this app needs to talk to the outer world.
+        //TODO for scrolling rework: need a way of intercepting Home/End/PageUp etc.
+        //Sometimes going to scroll our own internal buffer, sometimes going to send ESC
+        //
+        //Same goes for key events
+        //Actually, we don't use the terminal at all in dispatch_event code, the view
+        //Handles it all. Check how the editor implements scrolling, is it view-level
+        //or element level?
+
+        //Question: Can we continue dispatching to the view, so it can talk to the connection
+        //Or should we instead add a connection into here?
+
         match event {
             Event::ScrollWheel(ScrollWheelEvent {
                 delta, position, ..

styles/package-lock.json 🔗

@@ -5,7 +5,6 @@
     "requires": true,
     "packages": {
         "": {
-            "name": "styles",
             "version": "1.0.0",
             "license": "ISC",
             "dependencies": {