From 0f3c10a021bbe79e20bde1f55b87465edeec476d Mon Sep 17 00:00:00 2001 From: Skyler Hawthorne Date: Wed, 16 Mar 2022 23:34:21 -0400 Subject: [PATCH] Fix initial selection of Document in new view When a new View of a Document is created, a default cursor of 0, 0 is created, and it does not get normalized to a single width cursor until at least one movement of the cursor happens. This appears to have no practical negative effect that I could find, but it makes tests difficult to work with, since the initial selection is not what you expect it to be. This changes the initial selection of a new View to be the width of the first grapheme in the text. --- .gitignore | 1 - helix-core/src/auto_pairs.rs | 14 +--- helix-core/src/selection.rs | 10 ++- helix-loader/Cargo.toml | 1 - helix-loader/src/lib.rs | 4 +- helix-term/src/application.rs | 21 ++++++ helix-term/src/commands.rs | 17 +++-- helix-term/tests/integration.rs | 112 ++++++++++++++++++++++++-------- helix-view/src/document.rs | 34 +++++++++- helix-view/src/editor.rs | 24 ++----- 10 files changed, 173 insertions(+), 65 deletions(-) diff --git a/.gitignore b/.gitignore index 346d0946a..6a6fc782a 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,5 @@ target .direnv helix-term/rustfmt.toml -helix-syntax/languages/ result runtime/grammars diff --git a/helix-core/src/auto_pairs.rs b/helix-core/src/auto_pairs.rs index bcd47356f..1131178e3 100644 --- a/helix-core/src/auto_pairs.rs +++ b/helix-core/src/auto_pairs.rs @@ -1,9 +1,7 @@ //! When typing the opening character of one of the possible pairs defined below, //! this module provides the functionality to insert the paired closing character. -use crate::{ - graphemes, movement::Direction, Range, Rope, RopeGraphemes, Selection, Tendril, Transaction, -}; +use crate::{graphemes, movement::Direction, Range, Rope, Selection, Tendril, Transaction}; use std::collections::HashMap; use log::debug; @@ -149,14 +147,6 @@ fn prev_char(doc: &Rope, pos: usize) -> Option { doc.get_char(pos - 1) } -fn is_single_grapheme(doc: &Rope, range: &Range) -> bool { - let mut graphemes = RopeGraphemes::new(doc.slice(range.from()..range.to())); - let first = graphemes.next(); - let second = graphemes.next(); - debug!("first: {:#?}, second: {:#?}", first, second); - first.is_some() && second.is_none() -} - /// calculate what the resulting range should be for an auto pair insertion fn get_next_range( doc: &Rope, @@ -189,8 +179,8 @@ fn get_next_range( ); } - let single_grapheme = is_single_grapheme(doc, start_range); let doc_slice = doc.slice(..); + let single_grapheme = start_range.is_single_grapheme(doc_slice); // just skip over graphemes if len_inserted == 0 { diff --git a/helix-core/src/selection.rs b/helix-core/src/selection.rs index 1b2416f5b..83bab5e30 100644 --- a/helix-core/src/selection.rs +++ b/helix-core/src/selection.rs @@ -8,7 +8,7 @@ use crate::{ prev_grapheme_boundary, }, movement::Direction, - Assoc, ChangeSet, RopeSlice, + Assoc, ChangeSet, RopeGraphemes, RopeSlice, }; use smallvec::{smallvec, SmallVec}; use std::borrow::Cow; @@ -339,6 +339,14 @@ impl Range { pub fn cursor_line(&self, text: RopeSlice) -> usize { text.char_to_line(self.cursor(text)) } + + /// Returns true if this Range covers a single grapheme in the given text + pub fn is_single_grapheme(&self, doc: RopeSlice) -> bool { + let mut graphemes = RopeGraphemes::new(doc.slice(self.from()..self.to())); + let first = graphemes.next(); + let second = graphemes.next(); + first.is_some() && second.is_none() + } } impl From<(usize, usize)> for Range { diff --git a/helix-loader/Cargo.toml b/helix-loader/Cargo.toml index 203844727..3d8a697cc 100644 --- a/helix-loader/Cargo.toml +++ b/helix-loader/Cargo.toml @@ -20,7 +20,6 @@ toml = "0.5" etcetera = "0.4" tree-sitter = "0.20" once_cell = "1.12" - log = "0.4" # TODO: these two should be on !wasm32 only diff --git a/helix-loader/src/lib.rs b/helix-loader/src/lib.rs index 595ac7aa5..ff4414b26 100644 --- a/helix-loader/src/lib.rs +++ b/helix-loader/src/lib.rs @@ -13,7 +13,9 @@ pub fn runtime_dir() -> std::path::PathBuf { if let Ok(dir) = std::env::var("CARGO_MANIFEST_DIR") { // this is the directory of the crate being run by cargo, we need the workspace path so we take the parent - return std::path::PathBuf::from(dir).parent().unwrap().join(RT_DIR); + let path = std::path::PathBuf::from(dir).parent().unwrap().join(RT_DIR); + log::debug!("runtime dir: {}", path.to_string_lossy()); + return path; } const RT_DIR: &str = "runtime"; diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index 21595eae7..146194bff 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -55,8 +55,29 @@ pub struct Application { lsp_progress: LspProgressMap, } +#[cfg(feature = "integration")] +fn setup_integration_logging() { + // Separate file config so we can include year, month and day in file logs + let _ = fern::Dispatch::new() + .format(|out, message, record| { + out.finish(format_args!( + "{} {} [{}] {}", + chrono::Local::now().format("%Y-%m-%dT%H:%M:%S%.3f"), + record.target(), + record.level(), + message + )) + }) + .level(log::LevelFilter::Debug) + .chain(std::io::stdout()) + .apply(); +} + impl Application { pub fn new(args: Args, config: Config) -> Result { + #[cfg(feature = "integration")] + setup_integration_logging(); + use helix_view::editor::Action; let config_dir = helix_loader::config_dir(); diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index c9c8e6a98..6b01cbe31 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -2094,10 +2094,17 @@ fn insert_mode(cx: &mut Context) { let (view, doc) = current!(cx.editor); enter_insert_mode(doc); - let selection = doc - .selection(view.id) - .clone() - .transform(|range| Range::new(range.to(), range.from())); + log::trace!( + "entering insert mode with sel: {:?}, text: {:?}", + doc.selection(view.id), + doc.text().to_string() + ); + + let selection = doc.selection(view.id).clone().transform(|range| { + let new_range = Range::new(range.to(), range.from()); + new_range + }); + doc.set_selection(view.id, selection); } @@ -2444,8 +2451,8 @@ fn normal_mode(cx: &mut Context) { graphemes::prev_grapheme_boundary(text, range.to()), ) }); - doc.set_selection(view.id, selection); + doc.set_selection(view.id, selection); doc.restore_cursor = false; } } diff --git a/helix-term/tests/integration.rs b/helix-term/tests/integration.rs index 31a0d218e..58883d40e 100644 --- a/helix-term/tests/integration.rs +++ b/helix-term/tests/integration.rs @@ -2,9 +2,9 @@ mod integration { use std::path::PathBuf; - use helix_core::{syntax::AutoPairConfig, Position, Selection, Tendril, Transaction}; + use helix_core::{syntax::AutoPairConfig, Position, Selection, Transaction}; use helix_term::{application::Application, args::Args, config::Config}; - use helix_view::{current, doc, input::parse_macro}; + use helix_view::{doc, input::parse_macro}; use crossterm::event::{Event, KeyEvent}; use indoc::indoc; @@ -25,14 +25,14 @@ mod integration { let mut app = app.unwrap_or_else(|| Application::new(Args::default(), Config::default()).unwrap()); - let (view, doc) = current!(app.editor); + let (view, doc) = helix_view::current!(app.editor); + let sel = doc.selection(view.id).clone(); + // replace the initial text with the input text doc.apply( - &Transaction::insert( - doc.text(), - &Selection::single(1, 0), - Tendril::from(&test_case.in_text), - ) + &Transaction::change_by_selection(&doc.text(), &sel, |_| { + (0, doc.text().len_chars(), Some((&test_case.in_text).into())) + }) .with_selection(test_case.in_selection.clone()), view.id, ); @@ -80,12 +80,12 @@ mod integration { Args::default(), Config::default(), TestCase { - in_text: String::new(), + in_text: "\n".into(), in_selection: Selection::single(0, 1), // TODO: fix incorrect selection on new doc - in_keys: String::from("ihello worldhl"), - out_text: String::from("hello world\n"), - out_selection: Selection::single(11, 12), + in_keys: "ihello world".into(), + out_text: "hello world\n".into(), + out_selection: Selection::single(12, 11), }, )?; @@ -93,16 +93,74 @@ mod integration { } #[tokio::test] - async fn auto_pairs_basic() -> anyhow::Result<()> { + async fn insert_mode_cursor_position() -> anyhow::Result<()> { test_key_sequence_text_result( Args::default(), Config::default(), TestCase { in_text: String::new(), + in_selection: Selection::single(0, 0), + in_keys: "i".into(), + out_text: String::new(), + out_selection: Selection::single(0, 0), + }, + )?; + + test_key_sequence_text_result( + Args::default(), + Config::default(), + TestCase { + in_text: "\n".into(), + in_selection: Selection::single(0, 1), + in_keys: "i".into(), + out_text: "\n".into(), + out_selection: Selection::single(1, 0), + }, + )?; + + test_key_sequence_text_result( + Args::default(), + Config::default(), + TestCase { + in_text: "\n".into(), in_selection: Selection::single(0, 1), - in_keys: String::from("i(hl"), - out_text: String::from("()\n"), - out_selection: Selection::single(1, 2), + in_keys: "ii".into(), + out_text: "\n".into(), + out_selection: Selection::single(1, 0), + }, + )?; + + Ok(()) + } + + #[tokio::test] + async fn insert_to_normal_mode_cursor_position() -> anyhow::Result<()> { + test_key_sequence_text_result( + Args::default(), + Config::default(), + TestCase { + in_text: "\n".into(), + in_selection: Selection::single(0, 1), + in_keys: "i".into(), + out_text: "\n".into(), + out_selection: Selection::single(1, 0), + }, + )?; + + Ok(()) + } + + #[tokio::test] + async fn auto_pairs_basic() -> anyhow::Result<()> { + test_key_sequence_text_result( + Args::default(), + Config::default(), + TestCase { + in_text: "\n".into(), + in_selection: Selection::single(0, 1), + in_keys: "i(".into(), + out_text: "()\n".into(), + out_selection: Selection::single(2, 1), }, )?; @@ -116,11 +174,11 @@ mod integration { ..Default::default() }, TestCase { - in_text: String::new(), + in_text: "\n".into(), in_selection: Selection::single(0, 1), - in_keys: String::from("i(hl"), - out_text: String::from("(\n"), - out_selection: Selection::single(1, 2), + in_keys: "i(".into(), + out_text: "(\n".into(), + out_selection: Selection::single(2, 1), }, )?; @@ -136,15 +194,17 @@ mod integration { }, Config::default(), TestCase { - in_text: String::from("void foo() {}"), - in_selection: Selection::single(12, 13), - in_keys: String::from("i"), - out_text: String::from(indoc! {r#" + in_text: "void foo() {}\n".into(), + in_selection: Selection::single(13, 12), + in_keys: "i".into(), + out_text: indoc! {r#" void foo() { } - "#}), - out_selection: Selection::single(15, 16), + "#} + .trim_start() + .into(), + out_selection: Selection::single(16, 15), }, )?; diff --git a/helix-view/src/document.rs b/helix-view/src/document.rs index a2d2af77c..00adaa1a7 100644 --- a/helix-view/src/document.rs +++ b/helix-view/src/document.rs @@ -1,5 +1,6 @@ use anyhow::{anyhow, bail, Context, Error}; use helix_core::auto_pairs::AutoPairs; +use helix_core::Range; use serde::de::{self, Deserialize, Deserializer}; use serde::Serialize; use std::cell::Cell; @@ -83,7 +84,7 @@ impl Serialize for Mode { pub struct Document { pub(crate) id: DocumentId, text: Rope, - pub(crate) selections: HashMap, + selections: HashMap, path: Option, encoding: &'static encoding::Encoding, @@ -637,6 +638,37 @@ impl Document { .insert(view_id, selection.ensure_invariants(self.text().slice(..))); } + /// Find the origin selection of the text in a document, i.e. where + /// a single cursor would go if it were on the first grapheme. If + /// the text is empty, returns (0, 0). + pub fn origin(&self) -> Range { + if self.text().len_chars() == 0 { + return Range::new(0, 0); + } + + Range::new(0, 1).grapheme_aligned(self.text().slice(..)) + } + + /// Reset the view's selection on this document to the + /// [origin](Document::origin) cursor. + pub fn reset_selection(&mut self, view_id: ViewId) { + let origin = self.origin(); + self.set_selection(view_id, Selection::single(origin.anchor, origin.head)); + } + + /// Initializes a new selection for the given view if it does not + /// already have one. + pub fn ensure_view_init(&mut self, view_id: ViewId) { + if self.selections.get(&view_id).is_none() { + self.reset_selection(view_id); + } + } + + /// Remove a view's selection from this document. + pub fn remove_view(&mut self, view_id: ViewId) { + self.selections.remove(&view_id); + } + /// Apply a [`Transaction`] to the [`Document`] to change its text. fn apply_impl(&mut self, transaction: &Transaction, view_id: ViewId) -> bool { let old_doc = self.text().clone(); diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index 76ac0b51d..8607c65a2 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -32,12 +32,12 @@ use anyhow::{bail, Error}; pub use helix_core::diagnostic::Severity; pub use helix_core::register::Registers; +use helix_core::Position; use helix_core::{ auto_pairs::AutoPairs, syntax::{self, AutoPairConfig}, Change, }; -use helix_core::{Position, Selection}; use helix_dap as dap; use serde::{ser::SerializeMap, Deserialize, Deserializer, Serialize, Serializer}; @@ -645,11 +645,8 @@ impl Editor { view.offset = Position::default(); let doc = self.documents.get_mut(&doc_id).unwrap(); + doc.ensure_view_init(view.id); - // initialize selection for view - doc.selections - .entry(view.id) - .or_insert_with(|| Selection::point(0)); // TODO: reuse align_view let pos = doc .selection(view.id) @@ -719,9 +716,7 @@ impl Editor { Action::Load => { let view_id = view!(self).id; let doc = self.documents.get_mut(&id).unwrap(); - if doc.selections().is_empty() { - doc.set_selection(view_id, Selection::point(0)); - } + doc.ensure_view_init(view_id); return; } Action::HorizontalSplit | Action::VerticalSplit => { @@ -736,7 +731,7 @@ impl Editor { ); // initialize selection for view let doc = self.documents.get_mut(&id).unwrap(); - doc.set_selection(view_id, Selection::point(0)); + doc.ensure_view_init(view_id); } } @@ -769,7 +764,7 @@ impl Editor { Ok(self.new_file_from_document(action, Document::from(rope, Some(encoding)))) } - // ??? + // ??? possible use for integration tests pub fn open(&mut self, path: PathBuf, action: Action) -> Result { let path = helix_core::path::get_canonicalized_path(&path)?; let id = self.document_by_path(&path).map(|doc| doc.id); @@ -791,12 +786,7 @@ impl Editor { pub fn close(&mut self, id: ViewId) { let view = self.tree.get(self.tree.focus); // remove selection - self.documents - .get_mut(&view.doc) - .unwrap() - .selections - .remove(&id); - + self.documents.get_mut(&view.doc).unwrap().remove_view(id); self.tree.remove(id); self._refresh(); } @@ -871,7 +861,7 @@ impl Editor { let view = View::new(doc_id, self.config().gutters.clone()); let view_id = self.tree.insert(view); let doc = self.documents.get_mut(&doc_id).unwrap(); - doc.set_selection(view_id, Selection::point(0)); + doc.ensure_view_init(view_id); } self._refresh();