Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write path fixes #2267

Merged
merged 37 commits into from
Oct 20, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
d706194
chore(write): serialize write operations within a Document
dead10ck Apr 10, 2022
a5a9318
fix: buffer-close ensuring writes
dead10ck Apr 23, 2022
83b6042
fix(write): do not set new path on document until write succeeds
dead10ck Apr 24, 2022
e1f7bdb
fix buffer-close
dead10ck May 10, 2022
69c9e44
update write-quit to wait for saves
dead10ck May 11, 2022
b8a07f7
add conditional noop render back
dead10ck Jun 24, 2022
cb23399
improve reliability of shutdown
dead10ck Jul 10, 2022
c941858
fix modified status with auto format
dead10ck Jul 5, 2022
aaa1450
fix write-quit with auto format
dead10ck Jul 12, 2022
8c667ef
factor editor event handling into function
dead10ck Jul 22, 2022
faa00d4
increase LSP shutdown timeout
dead10ck Aug 7, 2022
e5fd5e2
fix panic when view of pending write is closed
dead10ck Aug 9, 2022
d544376
reset idle timer for all events
dead10ck Aug 10, 2022
7b11e9a
fix erroneous write sender close
dead10ck Aug 23, 2022
57de4e6
various fixes in write-all path
dead10ck Aug 24, 2022
6cffc7f
Add note about log level for integration tests
dead10ck Aug 29, 2022
f82a551
Rename doc save event names to past tense
dead10ck Aug 31, 2022
18c3211
Save text in document saved events, use in status message
dead10ck Aug 31, 2022
af03df3
fix write scratch buffer to file
dead10ck Aug 31, 2022
b3fc31a
move language server refresh to document saved event handler
dead10ck Sep 2, 2022
b530a86
remove Callback::Compositor variant
dead10ck Sep 3, 2022
3f07885
document should save even if formatter fails
dead10ck Sep 17, 2022
9e64974
remove Document::format_and_save
dead10ck Sep 17, 2022
31d1bbf
review comments
dead10ck Sep 21, 2022
bf378e7
fix tests
dead10ck Oct 4, 2022
beb3427
improve app close failure display
dead10ck Oct 4, 2022
30c9399
Use a single save_queue on the editor
archseer Oct 14, 2022
b0212b3
Deduplicate flush_writes
archseer Oct 16, 2022
b155e86
Use a write_count to determine how many writes left to flush
archseer Oct 16, 2022
55b50d9
Seems like this flush is unnecessary
archseer Oct 16, 2022
1b6f731
Wire up save_queue as a part of new_document rather than open
archseer Oct 16, 2022
2a43ee0
doc.close() now unused
archseer Oct 16, 2022
52ba550
Use flush_writes in application.close()
archseer Oct 16, 2022
e645804
Editor::flush_writes returns an error
dead10ck Oct 17, 2022
759d55c
fail if doc save sender is closed
dead10ck Oct 17, 2022
9a406b5
reduce LSP timeout to 3s
dead10ck Oct 19, 2022
756253b
fix tree_sitter_scopes
dead10ck Oct 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ to `cargo install` anything either).
Integration tests for helix-term can be run with `cargo integration-test`. Code
contributors are strongly encouraged to write integration tests for their code.
Existing tests can be used as examples. Helpers can be found in
[helpers.rs][helpers.rs]
[helpers.rs][helpers.rs]. The log level can be set with the `HELIX_LOG_LEVEL`
environment variable, e.g. `HELIX_LOG_LEVEL=debug cargo integration-test`.

## Minimum Stable Rust Version (MSRV) Policy

Expand Down
1 change: 0 additions & 1 deletion helix-core/src/auto_pairs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use std::collections::HashMap;
use smallvec::SmallVec;

// Heavily based on https://github.com/codemirror/closebrackets/

pub const DEFAULT_PAIRS: &[(char, char)] = &[
('(', ')'),
('{', '}'),
Expand Down
6 changes: 6 additions & 0 deletions helix-core/src/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ pub struct Configuration {
pub language: Vec<LanguageConfiguration>,
}

impl Default for Configuration {
fn default() -> Self {
crate::config::default_syntax_loader()
}
}

// largely based on tree-sitter/cli/src/loader.rs
#[derive(Debug, Serialize, Deserialize)]
#[serde(rename_all = "kebab-case", deny_unknown_fields)]
Expand Down
208 changes: 156 additions & 52 deletions helix-term/src/application.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
use arc_swap::{access::Map, ArcSwap};
use futures_util::Stream;
use helix_core::{
config::{default_syntax_loader, user_syntax_loader},
diagnostic::{DiagnosticTag, NumberOrString},
path::get_relative_path,
pos_at_coords, syntax, Selection,
};
use helix_lsp::{lsp, util::lsp_pos_to_pos, LspProgressMap};
use helix_view::{align_view, editor::ConfigEvent, theme, tree::Layout, Align, Editor};
use helix_view::{
align_view,
document::DocumentSavedEventResult,
editor::{ConfigEvent, EditorEvent},
theme,
tree::Layout,
Align, Editor,
};
use serde_json::json;

use crate::{
Expand All @@ -19,7 +26,7 @@ use crate::{
ui::{self, overlay::overlayed},
};

use log::{error, warn};
use log::{debug, error, warn};
use std::{
io::{stdin, stdout, Write},
sync::Arc,
Expand Down Expand Up @@ -102,7 +109,11 @@ fn restore_term() -> Result<(), Error> {
}

impl Application {
pub fn new(args: Args, config: Config) -> Result<Self, Error> {
pub fn new(
args: Args,
config: Config,
syn_loader_conf: syntax::Configuration,
) -> Result<Self, Error> {
#[cfg(feature = "integration")]
setup_integration_logging();

Expand All @@ -129,14 +140,6 @@ impl Application {
})
.unwrap_or_else(|| theme_loader.default_theme(true_color));

let syn_loader_conf = user_syntax_loader().unwrap_or_else(|err| {
eprintln!("Bad language config: {}", err);
eprintln!("Press <ENTER> to continue with default language config");
use std::io::Read;
// This waits for an enter press.
let _ = std::io::stdin().read(&mut []);
default_syntax_loader()
});
let syn_loader = std::sync::Arc::new(syntax::Loader::new(syn_loader_conf));

let mut compositor = Compositor::new().context("build compositor")?;
Expand Down Expand Up @@ -245,6 +248,10 @@ impl Application {
Ok(app)
}

#[cfg(feature = "integration")]
fn render(&mut self) {}

#[cfg(not(feature = "integration"))]
fn render(&mut self) {
let compositor = &mut self.compositor;

Expand Down Expand Up @@ -275,9 +282,6 @@ impl Application {
where
S: Stream<Item = crossterm::Result<crossterm::event::Event>> + Unpin,
{
#[cfg(feature = "integration")]
let mut idle_handled = false;

loop {
if self.editor.should_close() {
return false;
Expand All @@ -294,26 +298,6 @@ impl Application {
Some(signal) = self.signals.next() => {
self.handle_signals(signal).await;
}
Some((id, call)) = self.editor.language_servers.incoming.next() => {
self.handle_language_server_message(call, id).await;
// limit render calls for fast language server messages
let last = self.editor.language_servers.incoming.is_empty();

if last || self.last_render.elapsed() > LSP_DEADLINE {
self.render();
self.last_render = Instant::now();
}
}
Some(payload) = self.editor.debugger_events.next() => {
let needs_render = self.editor.handle_debugger_message(payload).await;
if needs_render {
self.render();
}
}
Some(config_event) = self.editor.config_events.1.recv() => {
self.handle_config_events(config_event);
self.render();
}
Some(callback) = self.jobs.futures.next() => {
self.jobs.handle_callback(&mut self.editor, &mut self.compositor, callback);
self.render();
Expand All @@ -322,26 +306,22 @@ impl Application {
self.jobs.handle_callback(&mut self.editor, &mut self.compositor, callback);
self.render();
}
_ = &mut self.editor.idle_timer => {
// idle timeout
self.editor.clear_idle_timer();
self.handle_idle_timeout();
event = self.editor.wait_event() => {
pickfire marked this conversation as resolved.
Show resolved Hide resolved
let _idle_handled = self.handle_editor_event(event).await;

#[cfg(feature = "integration")]
{
idle_handled = true;
if _idle_handled {
return true;
}
}
}
}

// for integration tests only, reset the idle timer after every
// event to make a signal when test events are done processing
// event to signal when test events are done processing
#[cfg(feature = "integration")]
{
if idle_handled {
return true;
}

self.editor.reset_idle_timer();
}
}
Expand Down Expand Up @@ -446,6 +426,111 @@ impl Application {
}
}

pub fn handle_document_write(&mut self, doc_save_event: DocumentSavedEventResult) {
let doc_save_event = match doc_save_event {
Ok(event) => event,
Err(err) => {
self.editor.set_error(err.to_string());
return;
}
};

let doc = match self.editor.document_mut(doc_save_event.doc_id) {
None => {
warn!(
"received document saved event for non-existent doc id: {}",
doc_save_event.doc_id
);

return;
}
Some(doc) => doc,
};

debug!(
"document {:?} saved with revision {}",
doc.path(),
doc_save_event.revision
);
Comment on lines +450 to +454
Copy link
Contributor

@pickfire pickfire May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder why editors don't show user the revision, if it did then users are able to use time travel easily. Like :ear 1f, but yeah the issue could be that even if we show it, user don't know the range they could use for time travel, like 1 or 0 is the min but don't know the max if they travel back.


doc.set_last_saved_revision(doc_save_event.revision);

let lines = doc_save_event.text.len_lines();
let bytes = doc_save_event.text.len_bytes();

if doc.path() != Some(&doc_save_event.path) {
if let Err(err) = doc.set_path(Some(&doc_save_event.path)) {
log::error!(
"error setting path for doc '{:?}': {}",
doc.path(),
err.to_string(),
);

self.editor.set_error(err.to_string());
dead10ck marked this conversation as resolved.
Show resolved Hide resolved
return;
}

let loader = self.editor.syn_loader.clone();

// borrowing the same doc again to get around the borrow checker
let doc = doc_mut!(self.editor, &doc_save_event.doc_id);
let id = doc.id();
doc.detect_language(loader);
let _ = self.editor.refresh_language_server(id);
}

// TODO: fix being overwritten by lsp
self.editor.set_status(format!(
"'{}' written, {}L {}B",
get_relative_path(&doc_save_event.path).to_string_lossy(),
lines,
bytes
));
}

#[inline(always)]
pub async fn handle_editor_event(&mut self, event: EditorEvent) -> bool {
log::debug!("received editor event: {:?}", event);

match event {
EditorEvent::DocumentSaved(event) => {
self.handle_document_write(event);
self.render();
}
EditorEvent::ConfigEvent(event) => {
self.handle_config_events(event);
self.render();
}
EditorEvent::LanguageServerMessage((id, call)) => {
self.handle_language_server_message(call, id).await;
// limit render calls for fast language server messages
let last = self.editor.language_servers.incoming.is_empty();

if last || self.last_render.elapsed() > LSP_DEADLINE {
self.render();
self.last_render = Instant::now();
}
}
EditorEvent::DebuggerEvent(payload) => {
let needs_render = self.editor.handle_debugger_message(payload).await;
if needs_render {
self.render();
}
}
EditorEvent::IdleTimer => {
self.editor.clear_idle_timer();
self.handle_idle_timeout();

#[cfg(feature = "integration")]
{
return true;
}
}
}

false
}

pub fn handle_terminal_events(&mut self, event: Result<CrosstermEvent, crossterm::ErrorKind>) {
let mut cx = crate::compositor::Context {
editor: &mut self.editor,
Expand Down Expand Up @@ -866,25 +951,44 @@ impl Application {

self.event_loop(input_stream).await;

let err = self.close().await.err();

let close_errs = self.close().await;
restore_term()?;

if let Some(err) = err {
for err in close_errs {
self.editor.exit_code = 1;
eprintln!("Error: {}", err);
}

Ok(self.editor.exit_code)
}

pub async fn close(&mut self) -> anyhow::Result<()> {
self.jobs.finish().await?;
pub async fn close(&mut self) -> Vec<anyhow::Error> {
// [NOTE] we intentionally do not return early for errors because we
// want to try to run as much cleanup as we can, regardless of
// errors along the way
let mut errs = Vec::new();

if let Err(err) = self
.jobs
.finish(&mut self.editor, Some(&mut self.compositor))
.await
{
log::error!("Error executing job: {}", err);
errs.push(err);
};

if let Err(err) = self.editor.flush_writes().await {
log::error!("Error writing: {}", err);
errs.push(err);
}

if self.editor.close_language_servers(None).await.is_err() {
log::error!("Timed out waiting for language servers to shutdown");
};
errs.push(anyhow::format_err!(
"Timed out waiting for language servers to shutdown"
));
}

Ok(())
errs
}
}
Loading