Nearly done, not scheduling our own re-render yet

Mikayla Maki created

Change summary

crates/terminal/src/connected_el.rs   | 127 +++++++++++++++-------------
crates/terminal/src/connected_view.rs |   5 
crates/terminal/src/mappings/keys.rs  |   4 
crates/terminal/src/terminal.rs       |  73 +++++++---------
crates/terminal/src/terminal_view.rs  |   5 
5 files changed, 105 insertions(+), 109 deletions(-)

Detailed changes

crates/terminal/src/connected_el.rs 🔗

@@ -46,6 +46,7 @@ pub struct LayoutState {
     background_color: Color,
     selection_color: Color,
     size: TermDimensions,
+    display_offset: usize,
 }
 
 ///Helper struct for converting data between alacritty's cursor points, and displayed cursor points
@@ -355,6 +356,7 @@ impl TerminalEl {
         view_id: usize,
         visible_bounds: RectF,
         cur_size: TermDimensions,
+        display_offset: usize,
         cx: &mut PaintContext,
     ) {
         let mouse_down_connection = self.terminal.clone();
@@ -371,7 +373,7 @@ impl TerminalEl {
                                     position,
                                     origin,
                                     cur_size,
-                                    terminal.get_display_offset(),
+                                    display_offset,
                                 );
 
                                 terminal.mouse_down(point, side);
@@ -396,7 +398,7 @@ impl TerminalEl {
                                     position,
                                     origin,
                                     cur_size,
-                                    terminal.get_display_offset(),
+                                    display_offset,
                                 );
 
                                 terminal.click(point, side, click_count);
@@ -415,7 +417,7 @@ impl TerminalEl {
                                     position,
                                     origin,
                                     cur_size,
-                                    terminal.get_display_offset(),
+                                    display_offset,
                                 );
 
                                 terminal.drag(point, side);
@@ -514,10 +516,6 @@ impl Element for TerminalEl {
         let settings = cx.global::<Settings>();
         let font_cache = cx.font_cache();
 
-        //First step, make all methods take mut and update internal event queue to take new actions
-        //Update process terminal event to handle all actions correctly
-        //And it's done.
-
         //Setup layout information
         let terminal_theme = settings.theme.terminal.clone(); //TODO: Try to minimize this clone.
         let text_style = TerminalEl::make_text_style(font_cache, &settings);
@@ -534,58 +532,59 @@ impl Element for TerminalEl {
             terminal_theme.colors.background.clone()
         };
 
-        let (cursor, cells, rects, highlights) =
-            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,
-                        );
-
-                        //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()),
-                                    )
-                                },
+        let (cursor, cells, rects, highlights, display_offset) = self
+            .terminal
+            .upgrade(cx)
+            .unwrap()
+            .update(cx.app, |terminal, mcx| {
+                terminal.set_size(dimensions);
+                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,
+                    );
+
+                    //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(),
+                                    },
+                                )],
                             )
                         };
 
-                        (cursor, cells, rects, highlights)
-                    })
-                });
+                        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, content.display_offset)
+                })
+            });
 
         //Done!
         (
@@ -598,6 +597,7 @@ impl Element for TerminalEl {
                 size: dimensions,
                 rects,
                 highlights,
+                display_offset,
             },
         )
     }
@@ -616,7 +616,14 @@ impl Element for TerminalEl {
             let origin = bounds.origin() + vec2f(layout.size.cell_width, 0.);
 
             //Elements are ephemeral, only at paint time do we know what could be clicked by a mouse
-            self.attach_mouse_handlers(origin, self.view.id(), visible_bounds, layout.size, cx);
+            self.attach_mouse_handlers(
+                origin,
+                self.view.id(),
+                visible_bounds,
+                layout.size,
+                layout.display_offset,
+                cx,
+            );
 
             cx.paint_layer(clip_bounds, |cx| {
                 //Start with a background color
@@ -694,9 +701,9 @@ impl Element for TerminalEl {
                         (delta.y() / layout.size.line_height) * ALACRITTY_SCROLL_MULTIPLIER;
 
                     self.terminal.upgrade(cx.app).map(|terminal| {
-                        terminal
-                            .read(cx.app)
-                            .scroll(Scroll::Delta(vertical_scroll.round() as i32));
+                        terminal.update(cx.app, |term, _| {
+                            term.scroll(Scroll::Delta(vertical_scroll.round() as i32))
+                        });
                     });
                 })
                 .is_some(),

crates/terminal/src/connected_view.rs 🔗

@@ -43,11 +43,12 @@ impl ConnectedView {
         modal: bool,
         cx: &mut ViewContext<Self>,
     ) -> Self {
-        // cx.observe(&terminal, |_, _, cx| cx.notify()).detach(); //Terminal notifies for us
+        cx.observe(&terminal, |_, _, cx| cx.notify()).detach();
         cx.subscribe(&terminal, |this, _, event, cx| match event {
             Event::Wakeup => {
                 if !cx.is_self_focused() {
                     this.has_new_content = true;
+                    cx.notify();
                     cx.emit(Event::Wakeup);
                 }
             }
@@ -91,7 +92,7 @@ impl ConnectedView {
 
     ///Attempt to paste the clipboard into the terminal
     fn copy(&mut self, _: &Copy, cx: &mut ViewContext<Self>) {
-        self.terminal.read(cx).copy()
+        self.terminal.update(cx, |term, _| term.copy())
     }
 
     ///Attempt to paste the clipboard into the terminal

crates/terminal/src/mappings/keys.rs 🔗

@@ -40,8 +40,8 @@ impl Modifiers {
 ///This is unavoidable for our use case. GPUI cannot wait until we acquire the terminal
 ///lock to determine whether we could actually send the keystroke with the current settings. Therefore,
 ///This conservative guess is used instead. Note that in practice the case where this method
-///Returns false when the actual terminal would consume the keystroke never actually happens. All
-///keystrokes that depend on terminal modes also have a mapping that doesn't depend on the terminal mode.
+///Returns false when the actual terminal would consume the keystroke never happens. All keystrokes
+///that depend on terminal modes also have a mapping that doesn't depend on the terminal mode.
 ///This is fragile, but as these mappings are locked up in legacy compatibility, it's probably good enough
 pub fn might_convert(keystroke: &Keystroke) -> bool {
     to_esc_str(keystroke, &TermMode::NONE).is_some()

crates/terminal/src/terminal.rs 🔗

@@ -24,13 +24,10 @@ use alacritty_terminal::{
 };
 use anyhow::{bail, Result};
 use futures::channel::mpsc::{unbounded, UnboundedReceiver, UnboundedSender};
-use itertools::Itertools;
 use mappings::keys::might_convert;
 use modal::deploy_modal;
 use settings::{Settings, Shell};
-use std::{
-    cmp::Ordering, collections::HashMap, fmt::Display, path::PathBuf, sync::Arc, time::Duration,
-};
+use std::{collections::HashMap, fmt::Display, path::PathBuf, sync::Arc, time::Duration};
 use terminal_view::TerminalView;
 use thiserror::Error;
 
@@ -53,7 +50,7 @@ pub fn init(cx: &mut MutableAppContext) {
     connected_view::init(cx);
 }
 
-const DEBUG_TERMINAL_WIDTH: f32 = 100.;
+const DEBUG_TERMINAL_WIDTH: f32 = 500.;
 const DEBUG_TERMINAL_HEIGHT: f32 = 30.; //This needs to be wide enough that the CI & a local dev's prompt can fill the whole space.
 const DEBUG_CELL_WIDTH: f32 = 5.;
 const DEBUG_LINE_HEIGHT: f32 = 5.;
@@ -77,7 +74,7 @@ enum InternalEvent {
     Paste(String),
     Scroll(Scroll),
     SetSelection(Option<Selection>),
-    UpdateSelection(Point),
+    UpdateSelection((Point, Direction)),
     Copy,
 }
 
@@ -337,7 +334,7 @@ impl TerminalBuilder {
             pty_tx: Notifier(pty_tx),
             term,
 
-            event_stack: vec![],
+            events: vec![],
             title: shell_txt.clone(),
             default_title: shell_txt,
         };
@@ -366,12 +363,10 @@ impl TerminalBuilder {
                         Err(_) => break,
                     }
                 }
-
                 match this.upgrade(&cx) {
                     Some(this) => {
-                        this.update(&mut cx, |this, cx| {
+                        this.update(&mut cx, |this, _cx| {
                             this.push_events(events);
-                            cx.notify();
                         });
                     }
                     None => break 'outer,
@@ -389,14 +384,14 @@ impl TerminalBuilder {
 pub struct Terminal {
     pty_tx: Notifier,
     term: Arc<FairMutex<Term<ZedListener>>>,
-    event_stack: Vec<InternalEvent>,
+    events: Vec<InternalEvent>,
     default_title: String,
     title: String,
 }
 
 impl Terminal {
     fn push_events(&mut self, events: Vec<AlacTermEvent>) {
-        self.event_stack
+        self.events
             .extend(events.into_iter().map(|e| InternalEvent::TermEvent(e)))
     }
 
@@ -407,6 +402,7 @@ impl Terminal {
         term: &mut Term<ZedListener>,
         cx: &mut ModelContext<Self>,
     ) {
+        dbg!(event);
         // TODO: Handle is_self_focused in subscription on terminal view
         match event {
             InternalEvent::TermEvent(term_event) => match term_event {
@@ -467,6 +463,7 @@ impl Terminal {
                 term.clear_screen(ClearMode::Saved);
             }
             InternalEvent::Keystroke(keystroke) => {
+                println!("Trying keystroke: {}", keystroke);
                 let esc = to_esc_str(keystroke, term.mode());
                 if let Some(esc) = esc {
                     self.notify_pty(esc);
@@ -482,10 +479,10 @@ impl Terminal {
                 }
             }
             InternalEvent::Scroll(scroll) => term.scroll_display(*scroll),
-            InternalEvent::SetSelection(sel) => term.selection = sel,
-            InternalEvent::UpdateSelection(point) => {
+            InternalEvent::SetSelection(sel) => term.selection = sel.clone(),
+            InternalEvent::UpdateSelection((point, side)) => {
                 if let Some(mut selection) = term.selection.take() {
-                    selection.update(*point, side);
+                    selection.update(*point, *side);
                     term.selection = Some(selection);
                 }
             }
@@ -502,30 +499,24 @@ impl Terminal {
         self.pty_tx.notify(txt.into_bytes());
     }
 
-    //TODO:
-    // - Continue refactor into event system
-    // - Fix PTYWrite call to not be circular and messy
-    // - Change title to be emitted and maintained externally
-
     ///Write the Input payload to the tty.
     pub fn write_to_pty(&mut self, input: String) {
-        self.event_stack
+        self.events
             .push(InternalEvent::TermEvent(AlacTermEvent::PtyWrite(input)))
     }
 
     ///Resize the terminal and the PTY.
     pub fn set_size(&mut self, new_size: TermDimensions) {
-        self.event_stack
-            .push(InternalEvent::Resize(new_size.into()))
+        self.events.push(InternalEvent::Resize(new_size.into()))
     }
 
     pub fn clear(&mut self) {
-        self.event_stack.push(InternalEvent::Clear)
+        self.events.push(InternalEvent::Clear)
     }
 
     pub fn try_keystroke(&mut self, keystroke: &Keystroke) -> bool {
         if might_convert(keystroke) {
-            self.event_stack
+            self.events
                 .push(InternalEvent::Keystroke(keystroke.clone()));
             true
         } else {
@@ -535,27 +526,25 @@ impl Terminal {
 
     ///Paste text into the terminal
     pub fn paste(&mut self, text: &str) {
-        self.event_stack
-            .push(InternalEvent::Paste(text.to_string()));
+        self.events.push(InternalEvent::Paste(text.to_string()));
     }
 
-    pub fn copy(&self) {
-        self.event_stack.push(InternalEvent::Copy);
+    pub fn copy(&mut self) {
+        self.events.push(InternalEvent::Copy);
     }
 
     pub fn render_lock<F, T>(&mut self, cx: &mut ModelContext<Self>, f: F) -> T
     where
         F: FnOnce(RenderableContent, char) -> T,
     {
+        println!("RENDER LOCK!");
         let m = self.term.clone(); //Arc clone
         let mut term = m.lock();
 
-        for event in self.event_stack.clone().into_iter().sorted() {
-            self.process_terminal_event(&event, &mut term, cx)
+        while let Some(e) = self.events.pop() {
+            self.process_terminal_event(&e, &mut term, cx)
         }
 
-        self.event_stack.clear();
-
         let content = term.renderable_content();
         let cursor_text = term.grid()[content.cursor.point].c;
 
@@ -563,11 +552,11 @@ impl Terminal {
     }
 
     ///Scroll the terminal
-    pub fn scroll(&self, _scroll: Scroll) {
-        self.event_stack.push(InternalEvent::Scroll(scroll));
+    pub fn scroll(&mut self, scroll: Scroll) {
+        self.events.push(InternalEvent::Scroll(scroll));
     }
 
-    pub fn click(&self, point: Point, side: Direction, clicks: usize) {
+    pub fn click(&mut self, point: Point, side: Direction, clicks: usize) {
         let selection_type = match clicks {
             0 => return, //This is a release
             1 => Some(SelectionType::Simple),
@@ -579,17 +568,17 @@ impl Terminal {
         let selection =
             selection_type.map(|selection_type| Selection::new(selection_type, point, side));
 
-        self.event_stack
-            .push(InternalEvent::SetSelection(selection));
+        self.events.push(InternalEvent::SetSelection(selection));
     }
 
-    pub fn drag(&self, point: Point, side: Direction) {
-        self.event_stack.push(InternalEvent::UpdateSelection(point));
+    pub fn drag(&mut self, point: Point, side: Direction) {
+        self.events
+            .push(InternalEvent::UpdateSelection((point, side)));
     }
 
     ///TODO: Check if the mouse_down-then-click assumption holds, so this code works as expected
-    pub fn mouse_down(&self, point: Point, side: Direction) {
-        self.event_stack
+    pub fn mouse_down(&mut self, point: Point, side: Direction) {
+        self.events
             .push(InternalEvent::SetSelection(Some(Selection::new(
                 SelectionType::Simple,
                 point,

crates/terminal/src/terminal_view.rs 🔗

@@ -65,14 +65,13 @@ impl TerminalView {
         workspace.add_item(Box::new(view), cx);
     }
 
-    ///Create a new Terminal view. This spawns a task, a thread, and opens the TTY devices
-    ///To get the right working directory from a workspace, use: `get_wd_for_workspace()`
+    ///Create a new Terminal view. This spawns a task, a thread, and opens the TTY devices    
     pub fn new(
         working_directory: Option<PathBuf>,
         modal: bool,
         cx: &mut ViewContext<Self>,
     ) -> Self {
-        //The details here don't matter, the terminal will be resized on the first layout
+        //The exact size here doesn't matter, the terminal will be resized on the first layout
         let size_info = TermDimensions::default();
 
         let settings = cx.global::<Settings>();