From 9d4c6c60fbc7970070237d709a7fd584a25451e8 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Tue, 26 Mar 2024 16:17:20 +0100 Subject: [PATCH] Do not format or fully save non-dirty buffers (#9813) Fixes https://github.com/zed-industries/zed/issues/9475 Release Notes: - Start skipping formatting and actual FS saving for non-dirty buffers ([9475](https://github.com/zed-industries/zed/issues/9475)) --- crates/editor/src/editor.rs | 7 +- crates/editor/src/editor_tests.rs | 257 +++++++++++++++++++++++++++++- crates/editor/src/items.rs | 58 +++---- 3 files changed, 282 insertions(+), 40 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 3308f5c9b569b23efbe564b644064656e1543d65..739f770a2795fada17b324538dee8faa452f1522 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -8264,14 +8264,17 @@ impl Editor { cx: &mut ViewContext, ) -> Task> { let buffer = self.buffer().clone(); - let buffers = buffer.read(cx).all_buffers(); + let mut buffers = buffer.read(cx).all_buffers(); + if trigger == FormatTrigger::Save { + buffers.retain(|buffer| buffer.read(cx).is_dirty()); + } let mut timeout = cx.background_executor().timer(FORMAT_TIMEOUT).fuse(); let format = project.update(cx, |project, cx| project.format(buffers, true, trigger, cx)); cx.spawn(|_, mut cx| async move { let transaction = futures::select_biased! { - _ = timeout => { + () = timeout => { log::warn!("timed out waiting for formatting"); None } diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 54dbee3e9bcd2a88f7ccad33c3189e3c58a32fb0..f3f476d25f8cd269e62b17d286e915d989f1cd77 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -5573,7 +5573,7 @@ async fn test_document_format_during_save(cx: &mut gpui::TestAppContext) { let buffer = cx.new_model(|cx| MultiBuffer::singleton(buffer, cx)); let (editor, cx) = cx.add_window_view(|cx| build_editor(buffer, cx)); - _ = editor.update(cx, |editor, cx| editor.set_text("one\ntwo\nthree\n", cx)); + editor.update(cx, |editor, cx| editor.set_text("one\ntwo\nthree\n", cx)); assert!(cx.read(|cx| editor.is_dirty(cx))); let save = editor @@ -5594,7 +5594,7 @@ async fn test_document_format_during_save(cx: &mut gpui::TestAppContext) { .next() .await; cx.executor().start_waiting(); - let _x = save.await; + save.await; assert_eq!( editor.update(cx, |editor, cx| editor.text(cx)), @@ -5602,7 +5602,7 @@ async fn test_document_format_during_save(cx: &mut gpui::TestAppContext) { ); assert!(!cx.read(|cx| editor.is_dirty(cx))); - _ = editor.update(cx, |editor, cx| editor.set_text("one\ntwo\nthree\n", cx)); + editor.update(cx, |editor, cx| editor.set_text("one\ntwo\nthree\n", cx)); assert!(cx.read(|cx| editor.is_dirty(cx))); // Ensure we can still save even if formatting hangs. @@ -5626,6 +5626,18 @@ async fn test_document_format_during_save(cx: &mut gpui::TestAppContext) { ); assert!(!cx.read(|cx| editor.is_dirty(cx))); + // For non-dirty buffer, no formatting request should be sent + let save = editor + .update(cx, |editor, cx| editor.save(true, project.clone(), cx)) + .unwrap(); + let _pending_format_request = fake_server + .handle_request::(move |_, _| async move { + panic!("Should not be invoked on non-dirty buffer"); + }) + .next(); + cx.executor().start_waiting(); + save.await; + // Set rust language override and assert overridden tabsize is sent to language server update_test_language_settings(cx, |settings| { settings.languages.insert( @@ -5637,6 +5649,8 @@ async fn test_document_format_during_save(cx: &mut gpui::TestAppContext) { ); }); + editor.update(cx, |editor, cx| editor.set_text("somehting_new\n", cx)); + assert!(cx.read(|cx| editor.is_dirty(cx))); let save = editor .update(cx, |editor, cx| editor.save(true, project.clone(), cx)) .unwrap(); @@ -5655,6 +5669,223 @@ async fn test_document_format_during_save(cx: &mut gpui::TestAppContext) { save.await; } +#[gpui::test] +async fn test_multibuffer_format_during_save(cx: &mut gpui::TestAppContext) { + init_test(cx, |_| {}); + + let cols = 4; + let rows = 10; + let sample_text_1 = sample_text(rows, cols, 'a'); + assert_eq!( + sample_text_1, + "aaaa\nbbbb\ncccc\ndddd\neeee\nffff\ngggg\nhhhh\niiii\njjjj" + ); + let sample_text_2 = sample_text(rows, cols, 'l'); + assert_eq!( + sample_text_2, + "llll\nmmmm\nnnnn\noooo\npppp\nqqqq\nrrrr\nssss\ntttt\nuuuu" + ); + let sample_text_3 = sample_text(rows, cols, 'v'); + assert_eq!( + sample_text_3, + "vvvv\nwwww\nxxxx\nyyyy\nzzzz\n{{{{\n||||\n}}}}\n~~~~\n\u{7f}\u{7f}\u{7f}\u{7f}" + ); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + "/a", + json!({ + "main.rs": sample_text_1, + "other.rs": sample_text_2, + "lib.rs": sample_text_3, + }), + ) + .await; + + let project = Project::test(fs, ["/a".as_ref()], cx).await; + let workspace = cx.add_window(|cx| Workspace::test_new(project.clone(), cx)); + let cx = &mut VisualTestContext::from_window(*workspace.deref(), cx); + + let language_registry = project.read_with(cx, |project, _| project.languages().clone()); + language_registry.add(rust_lang()); + let mut fake_servers = language_registry.register_fake_lsp_adapter( + "Rust", + FakeLspAdapter { + capabilities: lsp::ServerCapabilities { + document_formatting_provider: Some(lsp::OneOf::Left(true)), + ..Default::default() + }, + ..Default::default() + }, + ); + + let worktree = project.update(cx, |project, _| { + let mut worktrees = project.worktrees().collect::>(); + assert_eq!(worktrees.len(), 1); + worktrees.pop().unwrap() + }); + let worktree_id = worktree.update(cx, |worktree, _| worktree.id()); + + let buffer_1 = project + .update(cx, |project, cx| { + project.open_buffer((worktree_id, "main.rs"), cx) + }) + .await + .unwrap(); + let buffer_2 = project + .update(cx, |project, cx| { + project.open_buffer((worktree_id, "other.rs"), cx) + }) + .await + .unwrap(); + let buffer_3 = project + .update(cx, |project, cx| { + project.open_buffer((worktree_id, "lib.rs"), cx) + }) + .await + .unwrap(); + + let multi_buffer = cx.new_model(|cx| { + let mut multi_buffer = MultiBuffer::new(0, ReadWrite); + multi_buffer.push_excerpts( + buffer_1.clone(), + [ + ExcerptRange { + context: Point::new(0, 0)..Point::new(3, 0), + primary: None, + }, + ExcerptRange { + context: Point::new(5, 0)..Point::new(7, 0), + primary: None, + }, + ExcerptRange { + context: Point::new(9, 0)..Point::new(10, 4), + primary: None, + }, + ], + cx, + ); + multi_buffer.push_excerpts( + buffer_2.clone(), + [ + ExcerptRange { + context: Point::new(0, 0)..Point::new(3, 0), + primary: None, + }, + ExcerptRange { + context: Point::new(5, 0)..Point::new(7, 0), + primary: None, + }, + ExcerptRange { + context: Point::new(9, 0)..Point::new(10, 4), + primary: None, + }, + ], + cx, + ); + multi_buffer.push_excerpts( + buffer_3.clone(), + [ + ExcerptRange { + context: Point::new(0, 0)..Point::new(3, 0), + primary: None, + }, + ExcerptRange { + context: Point::new(5, 0)..Point::new(7, 0), + primary: None, + }, + ExcerptRange { + context: Point::new(9, 0)..Point::new(10, 4), + primary: None, + }, + ], + cx, + ); + multi_buffer + }); + let multi_buffer_editor = + cx.new_view(|cx| Editor::new(EditorMode::Full, multi_buffer, Some(project.clone()), cx)); + + multi_buffer_editor.update(cx, |editor, cx| { + editor.change_selections(Some(Autoscroll::Next), cx, |s| s.select_ranges(Some(1..2))); + editor.insert("|one|two|three|", cx); + }); + assert!(cx.read(|cx| multi_buffer_editor.is_dirty(cx))); + multi_buffer_editor.update(cx, |editor, cx| { + editor.change_selections(Some(Autoscroll::Next), cx, |s| { + s.select_ranges(Some(60..70)) + }); + editor.insert("|four|five|six|", cx); + }); + assert!(cx.read(|cx| multi_buffer_editor.is_dirty(cx))); + + // First two buffers should be edited, but not the third one. + assert_eq!( + multi_buffer_editor.update(cx, |editor, cx| editor.text(cx)), + "a|one|two|three|aa\nbbbb\ncccc\n\nffff\ngggg\n\njjjj\nllll\nmmmm\nnnnn|four|five|six|\nr\n\nuuuu\nvvvv\nwwww\nxxxx\n\n{{{{\n||||\n\n\u{7f}\u{7f}\u{7f}\u{7f}", + ); + buffer_1.update(cx, |buffer, _| { + assert!(buffer.is_dirty()); + assert_eq!( + buffer.text(), + "a|one|two|three|aa\nbbbb\ncccc\ndddd\neeee\nffff\ngggg\nhhhh\niiii\njjjj", + ) + }); + buffer_2.update(cx, |buffer, _| { + assert!(buffer.is_dirty()); + assert_eq!( + buffer.text(), + "llll\nmmmm\nnnnn|four|five|six|oooo\npppp\nr\nssss\ntttt\nuuuu", + ) + }); + buffer_3.update(cx, |buffer, _| { + assert!(!buffer.is_dirty()); + assert_eq!(buffer.text(), sample_text_3,) + }); + + cx.executor().start_waiting(); + let save = multi_buffer_editor + .update(cx, |editor, cx| editor.save(true, project.clone(), cx)) + .unwrap(); + + let fake_server = fake_servers.next().await.unwrap(); + fake_server + .server + .on_request::(move |params, _| async move { + Ok(Some(vec![lsp::TextEdit::new( + lsp::Range::new(lsp::Position::new(0, 3), lsp::Position::new(1, 0)), + format!("[{} formatted]", params.text_document.uri), + )])) + }) + .detach(); + save.await; + + // After multibuffer saving, only first two buffers should be reformatted, but not the third one (as it was not dirty). + assert!(cx.read(|cx| !multi_buffer_editor.is_dirty(cx))); + assert_eq!( + multi_buffer_editor.update(cx, |editor, cx| editor.text(cx)), + "a|o[file:///a/main.rs formatted]bbbb\ncccc\n\nffff\ngggg\n\njjjj\n\nlll[file:///a/other.rs formatted]mmmm\nnnnn|four|five|six|\nr\n\nuuuu\n\nvvvv\nwwww\nxxxx\n\n{{{{\n||||\n\n\u{7f}\u{7f}\u{7f}\u{7f}", + ); + buffer_1.update(cx, |buffer, _| { + assert!(!buffer.is_dirty()); + assert_eq!( + buffer.text(), + "a|o[file:///a/main.rs formatted]bbbb\ncccc\ndddd\neeee\nffff\ngggg\nhhhh\niiii\njjjj\n", + ) + }); + buffer_2.update(cx, |buffer, _| { + assert!(!buffer.is_dirty()); + assert_eq!( + buffer.text(), + "lll[file:///a/other.rs formatted]mmmm\nnnnn|four|five|six|oooo\npppp\nr\nssss\ntttt\nuuuu\n", + ) + }); + buffer_3.update(cx, |buffer, _| { + assert!(!buffer.is_dirty()); + assert_eq!(buffer.text(), sample_text_3,) + }); +} + #[gpui::test] async fn test_range_format_during_save(cx: &mut gpui::TestAppContext) { init_test(cx, |_| {}); @@ -5687,7 +5918,7 @@ async fn test_range_format_during_save(cx: &mut gpui::TestAppContext) { let buffer = cx.new_model(|cx| MultiBuffer::singleton(buffer, cx)); let (editor, cx) = cx.add_window_view(|cx| build_editor(buffer, cx)); - _ = editor.update(cx, |editor, cx| editor.set_text("one\ntwo\nthree\n", cx)); + editor.update(cx, |editor, cx| editor.set_text("one\ntwo\nthree\n", cx)); assert!(cx.read(|cx| editor.is_dirty(cx))); let save = editor @@ -5715,7 +5946,7 @@ async fn test_range_format_during_save(cx: &mut gpui::TestAppContext) { ); assert!(!cx.read(|cx| editor.is_dirty(cx))); - _ = editor.update(cx, |editor, cx| editor.set_text("one\ntwo\nthree\n", cx)); + editor.update(cx, |editor, cx| editor.set_text("one\ntwo\nthree\n", cx)); assert!(cx.read(|cx| editor.is_dirty(cx))); // Ensure we can still save even if formatting hangs. @@ -5741,7 +5972,19 @@ async fn test_range_format_during_save(cx: &mut gpui::TestAppContext) { ); assert!(!cx.read(|cx| editor.is_dirty(cx))); - // Set rust language override and assert overridden tabsize is sent to language server + // For non-dirty buffer, no formatting request should be sent + let save = editor + .update(cx, |editor, cx| editor.save(true, project.clone(), cx)) + .unwrap(); + let _pending_format_request = fake_server + .handle_request::(move |_, _| async move { + panic!("Should not be invoked on non-dirty buffer"); + }) + .next(); + cx.executor().start_waiting(); + save.await; + + // Set Rust language override and assert overridden tabsize is sent to language server update_test_language_settings(cx, |settings| { settings.languages.insert( "Rust".into(), @@ -5752,6 +5995,8 @@ async fn test_range_format_during_save(cx: &mut gpui::TestAppContext) { ); }); + editor.update(cx, |editor, cx| editor.set_text("somehting_new\n", cx)); + assert!(cx.read(|cx| editor.is_dirty(cx))); let save = editor .update(cx, |editor, cx| editor.save(true, project.clone(), cx)) .unwrap(); diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index 944f63133d3e5b41f55986578424df08f11c69f6..84baa95d7e9bb6b4d9af822aae0c42d4f68585ed 100644 --- a/crates/editor/src/items.rs +++ b/crates/editor/src/items.rs @@ -699,43 +699,37 @@ impl Item for Editor { let buffers = self.buffer().clone().read(cx).all_buffers(); cx.spawn(|this, mut cx| async move { if format { - this.update(&mut cx, |this, cx| { - this.perform_format(project.clone(), FormatTrigger::Save, cx) + this.update(&mut cx, |editor, cx| { + editor.perform_format(project.clone(), FormatTrigger::Save, cx) })? .await?; } - if buffers.len() == 1 { - project - .update(&mut cx, |project, cx| project.save_buffers(buffers, cx))? - .await?; - } else { - // For multi-buffers, only save those ones that contain changes. For clean buffers - // we simulate saving by calling `Buffer::did_save`, so that language servers or - // other downstream listeners of save events get notified. - let (dirty_buffers, clean_buffers) = buffers.into_iter().partition(|buffer| { - buffer - .update(&mut cx, |buffer, _| { - buffer.is_dirty() || buffer.has_conflict() - }) - .unwrap_or(false) - }); + // Only format and save the buffers with changes. For clean buffers, + // we simulate saving by calling `Buffer::did_save`, so that language servers or + // other downstream listeners of save events get notified. + let (dirty_buffers, clean_buffers) = buffers.into_iter().partition(|buffer| { + buffer + .update(&mut cx, |buffer, _| { + buffer.is_dirty() || buffer.has_conflict() + }) + .unwrap_or(false) + }); - project - .update(&mut cx, |project, cx| { - project.save_buffers(dirty_buffers, cx) - })? - .await?; - for buffer in clean_buffers { - buffer - .update(&mut cx, |buffer, cx| { - let version = buffer.saved_version().clone(); - let fingerprint = buffer.saved_version_fingerprint(); - let mtime = buffer.saved_mtime(); - buffer.did_save(version, fingerprint, mtime, cx); - }) - .ok(); - } + project + .update(&mut cx, |project, cx| { + project.save_buffers(dirty_buffers, cx) + })? + .await?; + for buffer in clean_buffers { + buffer + .update(&mut cx, |buffer, cx| { + let version = buffer.saved_version().clone(); + let fingerprint = buffer.saved_version_fingerprint(); + let mtime = buffer.saved_mtime(); + buffer.did_save(version, fingerprint, mtime, cx); + }) + .ok(); } Ok(())