5a84834 · helix · git.iko.soy

git.iko.soy/helix

fix: write-all crash (#4384)

When we do auto formatting, the code that takes the LSP's response and applies the changes to the document are just getting the currently focused view and giving that to the function, basically always assuming that the document that we're applying the change to is in focus, and not in a background view. This is usually fine for a single view, even if it's a buffer in the background, because it's still the same view and the selection will get updated accordingly for when you switch back to it. But it's obviously a problem for when there are multiple views, because if you don't have the target document in focus, it will ask the document to update the wrong view, hence the crash. The problem with this is picking which view to apply any selection change to. In the absence of any more data points on the views themselves, we simply pick the first view associated with the document we are saving.

commit5a848344a9b6f476922ea004e6a7f5b6d32ef768 authorSkyler Hawthorne <skyler@dead10ck.com> dateFri 21 Oct 2022, 03:43 (4 years ago) parentbad49ef

2 files changed+32−8

compare with master

unifiedsplit

helix-term/src/commands.rs+3−2 helix-term/src/commands/typed.rs+29−6

helix-term/src/commands.rs+3−2

@@ -2515,19 +2515,20 @@ fn insert_at_line_end(cx: &mut Context) {

25152515async fn make_format_callback(

25162516 doc_id: DocumentId,

25172517 doc_version: i32,

2518+ view_id: ViewId,

25182519 format: impl Future<Output = Result<Transaction, FormatterError>> + Send + 'static,

25192520 write: Option<(Option<PathBuf>, bool)>,

25202521) -> anyhow::Result<job::Callback> {

25212522 let format = format.await;

25222523

25232524 let call: job::Callback = Callback::Editor(Box::new(move |editor| {

2524− if !editor.documents.contains_key(&doc_id) {

2525+ if !editor.documents.contains_key(&doc_id) || !editor.tree.contains(view_id) {

25252526 return;

25262527 }

25272528

25282529 let scrolloff = editor.config().scrolloff;

25292530 let doc = doc_mut!(editor, &doc_id);

2530− let view = view_mut!(editor);

2531+ let view = view_mut!(editor, view_id);

25312532

25322533 if let Ok(format) = format {

25332534 if doc.version() == doc_version {

helix-term/src/commands/typed.rs+29−6

@@ -271,7 +271,7 @@ fn write_impl(

271271) -> anyhow::Result<()> {

272272 let editor_auto_fmt = cx.editor.config().auto_format;

273273 let jobs = &mut cx.jobs;

274− let doc = doc_mut!(cx.editor);

274+ let (view, doc) = current!(cx.editor);

275275 let path = path.map(AsRef::as_ref);

276276

277277 let fmt = if editor_auto_fmt {

@@ -279,6 +279,7 @@ fn write_impl(

279279 let callback = make_format_callback(

280280 doc.id(),

281281 doc.version(),

282+ view.id,

282283 fmt,

283284 Some((path.map(Into::into), force)),

284285 );

@@ -344,9 +345,9 @@ fn format(

344345 return Ok(());

345346 }

346347

347− let doc = doc!(cx.editor);

348+ let (view, doc) = current!(cx.editor);

348349 if let Some(format) = doc.format() {

349− let callback = make_format_callback(doc.id(), doc.version(), format, None);

350+ let callback = make_format_callback(doc.id(), doc.version(), view.id, format, None);

350351 cx.jobs.callback(callback);

351352 }

352353

@@ -568,12 +569,13 @@ pub fn write_all_impl(

568569 let mut errors: Vec<&'static str> = Vec::new();

569570 let auto_format = cx.editor.config().auto_format;

570571 let jobs = &mut cx.jobs;

572+ let current_view = view!(cx.editor);

571573

572574 // save all documents

573575 let saves: Vec<_> = cx

574576 .editor

575577 .documents

576− .values()

578+ .values_mut()

577579 .filter_map(|doc| {

578580 if !doc.is_modified() {

579581 return None;

@@ -585,10 +587,30 @@ pub fn write_all_impl(

585587 return None;

586588 }

587589

590+ // Look for a view to apply the formatting change to. If the document

591+ // is in the current view, just use that. Otherwise, since we don't

592+ // have any other metric available for better selection, just pick

593+ // the first view arbitrarily so that we still commit the document

594+ // state for undos. If somehow we have a document that has not been

595+ // initialized with any view, initialize it with the current view.

596+ let target_view = if doc.selections().contains_key(&current_view.id) {

597+ current_view.id

598+ } else if let Some(view) = doc.selections().keys().next() {

599+ *view

600+ } else {

601+ doc.ensure_view_init(current_view.id);

602+ current_view.id

603+ };

604+

588605 let fmt = if auto_format {

589606 doc.auto_format().map(|fmt| {

590− let callback =

591− make_format_callback(doc.id(), doc.version(), fmt, Some((None, force)));

607+ let callback = make_format_callback(

608+ doc.id(),

609+ doc.version(),

610+ target_view,

611+ fmt,

612+ Some((None, force)),

613+ );

592614 jobs.add(Job::with_callback(callback).wait_before_exiting());

593615 })

594616 } else {

@@ -598,6 +620,7 @@ pub fn write_all_impl(

598620 if fmt.is_none() {

599621 return Some(doc.id());

600622 }

623+

601624 None

602625 })

603626 .collect();