Merge pull request #1647 from zed-industries/format-buffer-action

Add editor action to manually invoke buffer format

assets/keymaps/default.json

@@ -93,6 +93,7 @@
             "cmd-shift-down": "editor::SelectToEnd",
             "cmd-a": "editor::SelectAll",
             "cmd-l": "editor::SelectLine",
+            "cmd-shift-i": "editor::Format",
             "cmd-shift-left": [
                 "editor::SelectToBeginningOfLine",
                 {

assets/settings/default.json

@@ -42,21 +42,20 @@
     // 3. Position the dock full screen over the entire workspace"
     //     "default_dock_anchor": "expanded"
     "default_dock_anchor": "right",
-    // How to auto-format modified buffers when saving them. This
-    // setting can take three values:
+    // Whether or not to perform a buffer format before saving
+    "format_on_save": true,
+    // How to perform a buffer format. This setting can take two values:
     //
-    // 1. Don't format code
-    //     "format_on_save": "off"
-    // 2. Format code using the current language server:
+    // 1. Format code using the current language server:
     //     "format_on_save": "language_server"
-    // 3. Format code using an external command:
+    // 2. Format code using an external command:
     //     "format_on_save": {
     //       "external": {
     //         "command": "prettier",
     //         "arguments": ["--stdin-filepath", "{buffer_path}"]
     //       }
     //     }
-    "format_on_save": "language_server",
+    "formatter": "language_server",
     // How to soft-wrap long lines of text. This setting can take
     // three values:
     //

crates/collab/src/integration_tests.rs

@@ -36,7 +36,7 @@ use project::{
 use rand::prelude::*;
 use rpc::PeerId;
 use serde_json::json;
-use settings::{FormatOnSave, Settings};
+use settings::{Formatter, Settings};
 use sqlx::types::time::OffsetDateTime;
 use std::{
     cell::RefCell,
@@ -1990,6 +1990,8 @@ async fn test_reloading_buffer_manually(cx_a: &mut TestAppContext, cx_b: &mut Te
 
 #[gpui::test(iterations = 10)]
 async fn test_formatting_buffer(cx_a: &mut TestAppContext, cx_b: &mut TestAppContext) {
+    use project::FormatTrigger;
+
     let mut server = TestServer::start(cx_a.foreground(), cx_a.background()).await;
     let client_a = server.create_client(cx_a, "user_a").await;
     let client_b = server.create_client(cx_b, "user_b").await;
@@ -2042,7 +2044,12 @@ async fn test_formatting_buffer(cx_a: &mut TestAppContext, cx_b: &mut TestAppCon
 
     project_b
         .update(cx_b, |project, cx| {
-            project.format(HashSet::from_iter([buffer_b.clone()]), true, cx)
+            project.format(
+                HashSet::from_iter([buffer_b.clone()]),
+                true,
+                FormatTrigger::Save,
+                cx,
+            )
         })
         .await
         .unwrap();
@@ -2055,7 +2062,7 @@ async fn test_formatting_buffer(cx_a: &mut TestAppContext, cx_b: &mut TestAppCon
     // host's configuration is honored as opposed to using the guest's settings.
     cx_a.update(|cx| {
         cx.update_global(|settings: &mut Settings, _| {
-            settings.editor_defaults.format_on_save = Some(FormatOnSave::External {
+            settings.editor_defaults.formatter = Some(Formatter::External {
                 command: "awk".to_string(),
                 arguments: vec!["{sub(/two/,\"{buffer_path}\")}1".to_string()],
             });
@@ -2063,7 +2070,12 @@ async fn test_formatting_buffer(cx_a: &mut TestAppContext, cx_b: &mut TestAppCon
     });
     project_b
         .update(cx_b, |project, cx| {
-            project.format(HashSet::from_iter([buffer_b.clone()]), true, cx)
+            project.format(
+                HashSet::from_iter([buffer_b.clone()]),
+                true,
+                FormatTrigger::Save,
+                cx,
+            )
         })
         .await
         .unwrap();

crates/editor/src/editor.rs

@@ -19,6 +19,7 @@ use collections::{BTreeMap, Bound, HashMap, HashSet, VecDeque};
 pub use display_map::DisplayPoint;
 use display_map::*;
 pub use element::*;
+use futures::FutureExt;
 use fuzzy::{StringMatch, StringMatchCandidate};
 use gpui::{
     actions,
@@ -49,7 +50,7 @@ pub use multi_buffer::{
 };
 use multi_buffer::{MultiBufferChunks, ToOffsetUtf16};
 use ordered_float::OrderedFloat;
-use project::{LocationLink, Project, ProjectPath, ProjectTransaction};
+use project::{FormatTrigger, LocationLink, Project, ProjectPath, ProjectTransaction};
 use selections_collection::{resolve_multiple, MutableSelectionsCollection, SelectionsCollection};
 use serde::{Deserialize, Serialize};
 use settings::Settings;
@@ -76,6 +77,8 @@ const MAX_LINE_LEN: usize = 1024;
 const MIN_NAVIGATION_HISTORY_ROW_DELTA: i64 = 10;
 const MAX_SELECTION_HISTORY_LEN: usize = 1024;
 
+pub const FORMAT_TIMEOUT: Duration = Duration::from_secs(2);
+
 #[derive(Clone, Deserialize, PartialEq, Default)]
 pub struct SelectNext {
     #[serde(default)]
@@ -204,6 +207,7 @@ actions!(
         OpenExcerpts,
         RestartLanguageServer,
         Hover,
+        Format,
     ]
 );
 
@@ -310,6 +314,7 @@ pub fn init(cx: &mut MutableAppContext) {
     cx.add_action(Editor::toggle_code_actions);
     cx.add_action(Editor::open_excerpts);
     cx.add_action(Editor::jump);
+    cx.add_async_action(Editor::format);
     cx.add_action(Editor::restart_language_server);
     cx.add_action(Editor::show_character_palette);
     cx.add_async_action(Editor::confirm_completion);
@@ -5173,6 +5178,51 @@ impl Editor {
         self.pending_rename.as_ref()
     }
 
+    fn format(&mut self, _: &Format, cx: &mut ViewContext<'_, Self>) -> Option<Task<Result<()>>> {
+        let project = match &self.project {
+            Some(project) => project.clone(),
+            None => return None,
+        };
+
+        Some(self.perform_format(project, cx))
+    }
+
+    fn perform_format(
+        &mut self,
+        project: ModelHandle<Project>,
+        cx: &mut ViewContext<'_, Self>,
+    ) -> Task<Result<()>> {
+        let buffer = self.buffer().clone();
+        let buffers = buffer.read(cx).all_buffers();
+
+        let mut timeout = cx.background().timer(FORMAT_TIMEOUT).fuse();
+        let format = project.update(cx, |project, cx| {
+            project.format(buffers, true, FormatTrigger::Manual, cx)
+        });
+
+        cx.spawn(|_, mut cx| async move {
+            let transaction = futures::select_biased! {
+                _ = timeout => {
+                    log::warn!("timed out waiting for formatting");
+                    None
+                }
+                transaction = format.log_err().fuse() => transaction,
+            };
+
+            buffer.update(&mut cx, |buffer, cx| {
+                if let Some(transaction) = transaction {
+                    if !buffer.is_singleton() {
+                        buffer.push_transaction(&transaction.0);
+                    }
+                }
+
+                cx.notify();
+            });
+
+            Ok(())
+        })
+    }
+
     fn restart_language_server(&mut self, _: &RestartLanguageServer, cx: &mut ViewContext<Self>) {
         if let Some(project) = self.project.clone() {
             self.buffer.update(cx, |multi_buffer, cx| {
@@ -10087,7 +10137,7 @@ mod tests {
             unreachable!()
         });
         let save = cx.update(|cx| editor.save(project.clone(), cx));
-        cx.foreground().advance_clock(items::FORMAT_TIMEOUT);
+        cx.foreground().advance_clock(super::FORMAT_TIMEOUT);
         cx.foreground().start_waiting();
         save.await.unwrap();
         assert_eq!(
@@ -10203,7 +10253,7 @@ mod tests {
             },
         );
         let save = cx.update(|cx| editor.save(project.clone(), cx));
-        cx.foreground().advance_clock(items::FORMAT_TIMEOUT);
+        cx.foreground().advance_clock(super::FORMAT_TIMEOUT);
         cx.foreground().start_waiting();
         save.await.unwrap();
         assert_eq!(
@@ -10241,6 +10291,87 @@ mod tests {
         save.await.unwrap();
     }
 
+    #[gpui::test]
+    async fn test_document_format_manual_trigger(cx: &mut gpui::TestAppContext) {
+        cx.foreground().forbid_parking();
+
+        let mut language = Language::new(
+            LanguageConfig {
+                name: "Rust".into(),
+                path_suffixes: vec!["rs".to_string()],
+                ..Default::default()
+            },
+            Some(tree_sitter_rust::language()),
+        );
+        let mut fake_servers = language
+            .set_fake_lsp_adapter(Arc::new(FakeLspAdapter {
+                capabilities: lsp::ServerCapabilities {
+                    document_formatting_provider: Some(lsp::OneOf::Left(true)),
+                    ..Default::default()
+                },
+                ..Default::default()
+            }))
+            .await;
+
+        let fs = FakeFs::new(cx.background());
+        fs.insert_file("/file.rs", Default::default()).await;
+
+        let project = Project::test(fs, ["/file.rs".as_ref()], cx).await;
+        project.update(cx, |project, _| project.languages().add(Arc::new(language)));
+        let buffer = project
+            .update(cx, |project, cx| project.open_local_buffer("/file.rs", cx))
+            .await
+            .unwrap();
+
+        cx.foreground().start_waiting();
+        let fake_server = fake_servers.next().await.unwrap();
+
+        let buffer = cx.add_model(|cx| MultiBuffer::singleton(buffer, cx));
+        let (_, editor) = cx.add_window(|cx| build_editor(buffer, cx));
+        editor.update(cx, |editor, cx| editor.set_text("one\ntwo\nthree\n", cx));
+
+        let format = editor.update(cx, |editor, cx| editor.perform_format(project.clone(), cx));
+        fake_server
+            .handle_request::<lsp::request::Formatting, _, _>(move |params, _| async move {
+                assert_eq!(
+                    params.text_document.uri,
+                    lsp::Url::from_file_path("/file.rs").unwrap()
+                );
+                assert_eq!(params.options.tab_size, 4);
+                Ok(Some(vec![lsp::TextEdit::new(
+                    lsp::Range::new(lsp::Position::new(0, 3), lsp::Position::new(1, 0)),
+                    ", ".to_string(),
+                )]))
+            })
+            .next()
+            .await;
+        cx.foreground().start_waiting();
+        format.await.unwrap();
+        assert_eq!(
+            editor.read_with(cx, |editor, cx| editor.text(cx)),
+            "one, two\nthree\n"
+        );
+
+        editor.update(cx, |editor, cx| editor.set_text("one\ntwo\nthree\n", cx));
+        // Ensure we don't lock if formatting hangs.
+        fake_server.handle_request::<lsp::request::Formatting, _, _>(move |params, _| async move {
+            assert_eq!(
+                params.text_document.uri,
+                lsp::Url::from_file_path("/file.rs").unwrap()
+            );
+            futures::future::pending::<()>().await;
+            unreachable!()
+        });
+        let format = editor.update(cx, |editor, cx| editor.perform_format(project, cx));
+        cx.foreground().advance_clock(super::FORMAT_TIMEOUT);
+        cx.foreground().start_waiting();
+        format.await.unwrap();
+        assert_eq!(
+            editor.read_with(cx, |editor, cx| editor.text(cx)),
+            "one\ntwo\nthree\n"
+        );
+    }
+
     #[gpui::test]
     async fn test_completion(cx: &mut gpui::TestAppContext) {
         let mut cx = EditorLspTestContext::new_rust(

crates/editor/src/items.rs

@@ -1,7 +1,7 @@
 use crate::{
     display_map::ToDisplayPoint, link_go_to_definition::hide_link_definition,
     movement::surrounding_word, Anchor, Autoscroll, Editor, Event, ExcerptId, MultiBuffer,
-    MultiBufferSnapshot, NavigationData, ToPoint as _,
+    MultiBufferSnapshot, NavigationData, ToPoint as _, FORMAT_TIMEOUT,
 };
 use anyhow::{anyhow, Result};
 use futures::FutureExt;
@@ -10,7 +10,7 @@ use gpui::{
     RenderContext, Subscription, Task, View, ViewContext, ViewHandle,
 };
 use language::{Bias, Buffer, File as _, OffsetRangeExt, SelectionGoal};
-use project::{File, Project, ProjectEntryId, ProjectPath};
+use project::{File, FormatTrigger, Project, ProjectEntryId, ProjectPath};
 use rpc::proto::{self, update_view};
 use settings::Settings;
 use smallvec::SmallVec;
@@ -20,7 +20,6 @@ use std::{
     fmt::Write,
     ops::Range,
     path::{Path, PathBuf},
-    time::Duration,
 };
 use text::{Point, Selection};
 use util::TryFutureExt;
@@ -30,7 +29,6 @@ use workspace::{
     ToolbarItemLocation,
 };
 
-pub const FORMAT_TIMEOUT: Duration = Duration::from_secs(2);
 pub const MAX_TAB_TITLE_LEN: usize = 24;
 
 impl FollowableItem for Editor {
@@ -409,7 +407,9 @@ impl Item for Editor {
         let buffer = self.buffer().clone();
         let buffers = buffer.read(cx).all_buffers();
         let mut timeout = cx.background().timer(FORMAT_TIMEOUT).fuse();
-        let format = project.update(cx, |project, cx| project.format(buffers, true, cx));
+        let format = project.update(cx, |project, cx| {
+            project.format(buffers, true, FormatTrigger::Save, cx)
+        });
         cx.spawn(|_, mut cx| async move {
             let transaction = futures::select_biased! {
                 _ = timeout => {

crates/project/src/project.rs

@@ -364,6 +364,22 @@ impl ProjectEntryId {
     }
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum FormatTrigger {
+    Save,
+    Manual,
+}
+
+impl FormatTrigger {
+    fn from_proto(value: i32) -> FormatTrigger {
+        match value {
+            0 => FormatTrigger::Save,
+            1 => FormatTrigger::Manual,
+            _ => FormatTrigger::Save,
+        }
+    }
+}
+
 impl Project {
     pub fn init(client: &Arc<Client>) {
         client.add_model_message_handler(Self::handle_request_join_project);
@@ -3047,6 +3063,7 @@ impl Project {
         &self,
         buffers: HashSet<ModelHandle<Buffer>>,
         push_to_history: bool,
+        trigger: FormatTrigger,
         cx: &mut ModelContext<Project>,
     ) -> Task<Result<ProjectTransaction>> {
         let mut local_buffers = Vec::new();
@@ -3076,6 +3093,7 @@ impl Project {
                 let response = client
                     .request(proto::FormatBuffers {
                         project_id,
+                        trigger: trigger as i32,
                         buffer_ids: remote_buffers
                             .iter()
                             .map(|buffer| buffer.read_with(&cx, |buffer, _| buffer.remote_id()))
@@ -3092,18 +3110,22 @@ impl Project {
             }
 
             for (buffer, buffer_abs_path, language_server) in local_buffers {
-                let (format_on_save, tab_size) = buffer.read_with(&cx, |buffer, cx| {
+                let (format_on_save, formatter, tab_size) = buffer.read_with(&cx, |buffer, cx| {
                     let settings = cx.global::<Settings>();
                     let language_name = buffer.language().map(|language| language.name());
                     (
                         settings.format_on_save(language_name.as_deref()),
+                        settings.formatter(language_name.as_deref()),
                         settings.tab_size(language_name.as_deref()),
                     )
                 });
 
-                let transaction = match format_on_save {
-                    settings::FormatOnSave::Off => continue,
-                    settings::FormatOnSave::LanguageServer => Self::format_via_lsp(
+                if trigger == FormatTrigger::Save && !format_on_save {
+                    continue;
+                }
+
+                let transaction = match formatter {
+                    settings::Formatter::LanguageServer => Self::format_via_lsp(
                         &this,
                         &buffer,
                         &buffer_abs_path,
@@ -3113,7 +3135,8 @@ impl Project {
                     )
                     .await
                     .context("failed to format via language server")?,
-                    settings::FormatOnSave::External { command, arguments } => {
+
+                    settings::Formatter::External { command, arguments } => {
                         Self::format_via_external_command(
                             &buffer,
                             &buffer_abs_path,
@@ -5296,7 +5319,8 @@ impl Project {
                         .ok_or_else(|| anyhow!("unknown buffer id {}", buffer_id))?,
                 );
             }
-            Ok::<_, anyhow::Error>(this.format(buffers, false, cx))
+            let trigger = FormatTrigger::from_proto(envelope.payload.trigger);
+            Ok::<_, anyhow::Error>(this.format(buffers, false, trigger, cx))
         })?;
 
         let project_transaction = format.await?;

crates/rpc/proto/zed.proto

@@ -420,9 +420,15 @@ message ReloadBuffersResponse {
     ProjectTransaction transaction = 1;
 }
 
+enum FormatTrigger {
+    Save = 0;
+    Manual = 1;
+}
+
 message FormatBuffers {
     uint64 project_id = 1;
-    repeated uint64 buffer_ids = 2;
+    FormatTrigger trigger = 2;
+    repeated uint64 buffer_ids = 3;
 }
 
 message FormatBuffersResponse {

crates/rpc/src/rpc.rs

@@ -6,4 +6,4 @@ pub use conn::Connection;
 pub use peer::*;
 mod macros;
 
-pub const PROTOCOL_VERSION: u32 = 31;
+pub const PROTOCOL_VERSION: u32 = 32;

crates/settings/src/settings.rs

@@ -58,7 +58,8 @@ pub struct EditorSettings {
     pub hard_tabs: Option<bool>,
     pub soft_wrap: Option<SoftWrap>,
     pub preferred_line_length: Option<u32>,
-    pub format_on_save: Option<FormatOnSave>,
+    pub format_on_save: Option<bool>,
+    pub formatter: Option<Formatter>,
     pub enable_language_server: Option<bool>,
 }
 
@@ -72,8 +73,7 @@ pub enum SoftWrap {
 
 #[derive(Clone, Debug, Deserialize, PartialEq, Eq, JsonSchema)]
 #[serde(rename_all = "snake_case")]
-pub enum FormatOnSave {
-    Off,
+pub enum Formatter {
     LanguageServer,
     External {
         command: String,
@@ -207,6 +207,7 @@ impl Settings {
         font_cache: &FontCache,
         themes: &ThemeRegistry,
     ) -> Self {
+        #[track_caller]
         fn required<T>(value: Option<T>) -> Option<T> {
             assert!(value.is_some(), "missing default setting value");
             value
@@ -236,6 +237,7 @@ impl Settings {
                 soft_wrap: required(defaults.editor.soft_wrap),
                 preferred_line_length: required(defaults.editor.preferred_line_length),
                 format_on_save: required(defaults.editor.format_on_save),
+                formatter: required(defaults.editor.formatter),
                 enable_language_server: required(defaults.editor.enable_language_server),
             },
             editor_overrides: Default::default(),
@@ -322,8 +324,12 @@ impl Settings {
         self.language_setting(language, |settings| settings.preferred_line_length)
     }
 
-    pub fn format_on_save(&self, language: Option<&str>) -> FormatOnSave {
-        self.language_setting(language, |settings| settings.format_on_save.clone())
+    pub fn format_on_save(&self, language: Option<&str>) -> bool {
+        self.language_setting(language, |settings| settings.format_on_save)
+    }
+
+    pub fn formatter(&self, language: Option<&str>) -> Formatter {
+        self.language_setting(language, |settings| settings.formatter.clone())
     }
 
     pub fn enable_language_server(&self, language: Option<&str>) -> bool {
@@ -358,7 +364,8 @@ impl Settings {
                 hard_tabs: Some(false),
                 soft_wrap: Some(SoftWrap::None),
                 preferred_line_length: Some(80),
-                format_on_save: Some(FormatOnSave::LanguageServer),
+                format_on_save: Some(true),
+                formatter: Some(Formatter::LanguageServer),
                 enable_language_server: Some(true),
             },
             editor_overrides: Default::default(),