Control channel: a command that discards unsaved work is refused, not carried out (#3284)

Lands #3284.

Co-Authored-By: matstrange <54028700+matstrange@users.noreply.github.com>

crates/engine/src/session/documents.rs

@@ -405,6 +405,12 @@ impl Session {
         id == self.document_id || self.others.iter().any(|d| d.document_id == id)
     }
 
+    /// Whether the open document `id` has changes that aren't written to a file yet: the
+    /// active document's own flag, or a background document's.
+    pub fn document_dirty(&self, id: u64) -> bool {
+        if id == self.document_id { self.dirty } else { self.others.iter().any(|d| d.document_id == id && d.dirty) }
+    }
+
     /// Every open document in the order they were opened, the active one included.
     pub fn documents(&self) -> Vec<DocInfo> {
         self.order

crates/ui-egui/src/app.rs

@@ -19,7 +19,7 @@ mod input;
 mod pictures;
 mod prefs;
 
-pub(crate) use commands::{discards_document, works_without_document};
+pub(crate) use commands::works_without_document;
 pub(crate) use input::control_click_as_secondary;
 
 impl WordApp {

crates/ui-egui/src/app/commands.rs

@@ -44,10 +44,9 @@ impl WordApp {
             self.dialog = dialogs::Dialog::open(name, self);
             return Ok(json!({"pending": name}));
         }
-        if (self.session.dirty || self.save_unconfirmed() || refused.is_some())
-            && discards_document(id, &params)
-            && !self.closes_only_its_window(id, &params)
-        {
+        // The same unsaved work the control channel's refusal covers, so the prompt and the
+        // refusal can't drift apart (#3274).
+        if (self.document_unsaved() || refused.is_some()) && discards_document(id, &params) && !self.closes_only_its_window(id, &params) {
             let name = self.prompt_name();
             self.dialog = Some(dialogs::Dialog::SaveChanges {
                 name,
@@ -119,6 +118,13 @@ impl WordApp {
         if id == "file.close" {
             return self.close_document(&params);
         }
+        // `file.closeDocument` closes the document its `id` names (the active one by default):
+        // the same unsaved work would go with it, so it needs the same explicit `force` (#3274).
+        if id == "file.closeDocument"
+            && let Some(e) = crate::control::close_refused(self, id, &params)
+        {
+            return Err(e.into());
+        }
         if id == "file.autosave" {
             return self.set_autosave(&params);
         }

crates/ui-egui/src/app/files.rs

@@ -81,6 +81,15 @@ impl WordApp {
         self.saves_in_flight.iter().any(|s| s.marked_saved && s.document == document)
     }
 
+    /// Whether the shown document has work that isn't saved yet: changes that aren't written
+    /// to a file, or a save that hasn't come back. The Save Changes prompt appears for a
+    /// command that would replace the document; a programmatic call (control channel, MCP
+    /// bridge), which never prompts, refuses the command instead — so both paths guard the
+    /// same work (#1268, #3274).
+    pub(crate) fn document_unsaved(&self) -> bool {
+        self.document_unsaved_by_id(self.session.document_id())
+    }
+
     /// The document's name as the Save Changes prompt shows it: its file's name, else its title.
     pub(super) fn prompt_name(&self) -> String {
         self.session.path.as_ref().and_then(|p| p.file_name()).map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| self.title_stem())

crates/ui-egui/src/canvas/tests.rs

@@ -2121,6 +2121,11 @@ fn an_agent_cannot_close_the_document_over_a_pending_comment_draft() {
     assert!(crate::control::close_refused(h.state(), "file.close", &json!({})).is_some());
     assert!(crate::control::close_refused(h.state(), "file.close", &json!({"force": true})).is_none());
     assert!(crate::control::close_refused(h.state(), "text.insert", &json!({"text": "x"})).is_none());
+    // The same for the command that names the document it closes (#3274).
+    let doc = h.state().session.document_id();
+    assert!(crate::control::close_refused(h.state(), "file.closeDocument", &json!({})).is_some());
+    assert!(crate::control::close_refused(h.state(), "file.closeDocument", &json!({"id": doc})).is_some());
+    assert!(crate::control::close_refused(h.state(), "file.closeDocument", &json!({"force": true})).is_none());
 }
 
 /// #1348: a comment edit typed, then refused (the document became read-only).

crates/ui-egui/src/control/handler.rs

@@ -81,19 +81,57 @@ fn click_events(app: &mut WordApp, pos: egui::Pos2, button: egui::PointerButton,
 /// Programmatic calls never open the Save prompt, so closing unsaved work must be explicit:
 /// save first, or pass `"force": true` (only a real boolean true counts) to discard it (#1268).
 pub(crate) fn close_refused(app: &WordApp, id: &str, params: &Value) -> Option<&'static str> {
+    // `file.close` closes only the drawn window when another window shows the same document:
+    // the document and its work stay open, so nothing is lost and nothing is refused (the
+    // interactive path asks nothing either) (#3274).
+    if app.closes_only_its_window(id, params) {
+        return None;
+    }
+    // The document the command closes: `file.close` the shown one, `file.closeDocument` the one
+    // its `id` names (the active one by default) (#3274).
+    let target = match id {
+        "file.close" => app.session.document_id(),
+        "file.closeDocument" => match params.get("id") {
+            None | Some(Value::Null) => app.session.document_id(),
+            // An `id` that isn't a document is the engine's error to give.
+            Some(v) => v.as_u64()?,
+        },
+        _ => return None,
+    };
+    close_refused_for(app, target, params)
+}
+
+/// The refusal for closing the document `target` with unsaved work in it: the same work the
+/// Save Changes prompt asks about, so the prompt and the refusal cover the same ground
+/// (#1268, #3274).
+fn close_refused_for(app: &WordApp, target: u64, params: &Value) -> Option<&'static str> {
     let force = params.get("force").and_then(Value::as_bool) == Some(true);
     if force {
         return None;
     }
-    if id == "file.close" && app.session.dirty {
+    if app.document_unsaved_by_id(target) {
         return Some(UNSAVED);
     }
-    // Comment text still being typed is unsaved work too (#1348): replacing the document would drop it.
-    (crate::discards_document(id, params) && crate::panes::has_comment_drafts(app))
-        .then_some("a comment is still being edited: finish it first, or pass {\"force\": true} to discard it")
+    // Comment text still being typed is unsaved work too (#1348): closing the document would
+    // drop it.
+    app.document_has_comment_drafts(target).then_some(COMMENT_UNSAVED)
 }
 
 const UNSAVED: &str = "the document has unsaved changes: save it first, or pass {\"force\": true} to discard them and close";
+const COMMENT_UNSAVED: &str = "a comment is still being edited: finish it first, or pass {\"force\": true} to discard it";
+
+/// Quitting closes every open document, so unsaved work needs the same explicit `force` as
+/// `file.close` rather than going silently (#1268, #3274). The interactive path asks about
+/// each document in turn; a programmatic call never prompts, so it refuses.
+fn quit_refused(app: &WordApp, params: &Value) -> Option<&'static str> {
+    let force = params.get("force").and_then(Value::as_bool) == Some(true);
+    if force {
+        return None;
+    }
+    app.has_unsaved_documents().then_some(UNSAVED_QUIT)
+}
+
+const UNSAVED_QUIT: &str = "a document has unsaved changes: save it first, or pass {\"force\": true} to quit without saving";
 
 /// The reply to a request that needs a document while none is open (#1129).
 const NO_DOCUMENT: &str = "no document is open; create one (file.new) or open one (file.open) first";
@@ -150,15 +188,28 @@ fn handle_in(app: &mut WordApp, doc: u64, method: &str, p: &Value) -> Outcome {
             }
             // Close, with the same guard as the window's: unsaved changes need `force`.
             if id == "file.close" {
-                let force = params.get("force").and_then(Value::as_bool) == Some(true);
-                if !force && app.session.documents().iter().any(|d| d.id == doc && d.dirty) {
-                    return err(UNSAVED);
+                if let Some(e) = close_refused_for(app, doc, &params) {
+                    return err(e);
                 }
                 return match app.session.close_document(doc) {
                     Ok(()) => ok(json!({"closed": true, "id": doc, "documents": app.session.documents()})),
                     Err(e) => err(e),
                 };
             }
+            if id == "file.closeDocument" {
+                // The document it names, or the one the request addresses.
+                let target = match params.get("id") {
+                    None | Some(Value::Null) => doc,
+                    // An `id` that isn't a document is the engine's error to give.
+                    Some(v) => match v.as_u64() {
+                        Some(id) => id,
+                        None => return err("`id` must be an open document's id"),
+                    },
+                };
+                if let Some(e) = close_refused_for(app, target, &params) {
+                    return err(e);
+                }
+            }
             let r = app.session.with_document(doc, |s| s.run(id, &params));
             // UI requests the command made carry `document`, so the window can tell they aren't
             // about the document it shows.
@@ -342,6 +393,11 @@ pub fn handle(app: &mut WordApp, ctx: &egui::Context, req: &ControlRequest) -> O
             ok(json!({}))
         }
         "app.quit" => {
+            // Quitting closes every open document, so unsaved work needs the same explicit
+            // `force` as `file.close` rather than going silently (#3274).
+            if let Some(e) = quit_refused(app, p) {
+                return err(e);
+            }
             app.quit_requested = true;
             ok(json!({}))
         }

crates/ui-egui/src/documents.rs

@@ -137,6 +137,24 @@ impl WordApp {
         Ok(())
     }
 
+    /// Whether the open document `id` has changes that aren't written to a file yet, or a save
+    /// the browser hasn't confirmed. The Save Changes prompt asks about the same work, so the
+    /// prompt and a programmatic refusal cannot drift apart (#1268, #3274).
+    pub(crate) fn document_unsaved_by_id(&self, id: u64) -> bool {
+        let dirty = self.session.document_dirty(id);
+        let unconfirmed = self.saves_in_flight.iter().any(|s| s.marked_saved && s.document == id);
+        dirty || unconfirmed
+    }
+
+    /// Whether the open document `id` has comment or reply text typed but not yet written to
+    /// it (#1348): closing the document would drop that text.
+    pub(crate) fn document_has_comment_drafts(&self, id: u64) -> bool {
+        if id == self.session.document_id() {
+            return panes::has_comment_drafts(self);
+        }
+        self.parked.get(&id).is_some_and(|p| !p.comment_drafts.is_empty())
+    }
+
     /// The open documents with work that would be lost on quitting: the shown one first, then
     /// the others in the order they were opened. Unsaved changes, a save the browser hasn't
     /// confirmed, or comment text not yet written to the document. The Start screen's
@@ -144,13 +162,11 @@ impl WordApp {
     fn unsaved_documents(&self) -> Vec<u64> {
         let active = self.session.document_id();
         let mut out = Vec::new();
-        if self.has_document() && (self.session.dirty || self.save_unconfirmed() || panes::has_comment_drafts(self)) {
+        if self.has_document() && (self.document_unsaved_by_id(active) || self.document_has_comment_drafts(active)) {
             out.push(active);
         }
         for d in self.session.documents() {
-            let unconfirmed = || self.saves_in_flight.iter().any(|s| s.marked_saved && s.document == d.id);
-            let drafts = || self.parked.get(&d.id).is_some_and(|p| !p.comment_drafts.is_empty());
-            if d.id != active && Some(d.id) != self.no_document && (d.dirty || unconfirmed() || drafts()) {
+            if d.id != active && Some(d.id) != self.no_document && (self.document_unsaved_by_id(d.id) || self.document_has_comment_drafts(d.id)) {
                 out.push(d.id);
             }
         }

crates/ui-egui/src/lib.rs

@@ -111,7 +111,7 @@ mod services;
 mod state;
 
 use app::control_click_as_secondary;
-pub(crate) use app::{discards_document, works_without_document};
+pub(crate) use app::works_without_document;
 pub use clock::now_ms;
 pub use services::{HostSaveFn, Inbox, Services};
 pub use state::{AutoSaveBlock, DefaultApp, SaveChoice, keeps_everything};

crates/ui-egui/src/tests.rs

@@ -4757,6 +4757,20 @@ fn web_close_while_a_picker_save_is_in_flight_asks_first() {
     assert!(body_text(&a).contains(UNSAVED), "nothing was replaced");
 }
 
+/// #3274: a save the browser's picker hasn't finished yet isn't written to a file, so a
+/// programmatic `file.closeDocument` refuses it as unsaved work.
+#[test]
+fn programmatic_close_document_keeps_a_save_the_picker_hasnt_finished() {
+    let (mut a, _saves, _last) = web_picker_app();
+    a.execute("file.save", json!({})).unwrap();
+    a.session.dirty = false;
+    let r = control_execute(&mut a, json!({"command": "file.closeDocument"}));
+    assert_eq!(r["ok"], json!(false), "{r}");
+    let e = r["error"].as_str().unwrap_or("");
+    assert!(e.contains("unsaved") && e.contains("force"), "{e}");
+    assert!(a.session.is_open(a.session.document_id()), "the document stays open");
+}
+
 /// #1315: a save that ends after another document is shown neither runs the held-back Close
 /// on that document nor marks it unsaved.
 #[test]
@@ -4875,6 +4889,108 @@ fn programmatic_close_guard_covers_every_form() {
     assert!(!a.has_document() && !a.quit_requested);
 }
 
+/// A programmatic `app.quit` closes every open document, so unsaved work needs the same
+/// explicit `force` as `file.close` rather than going silently (#3274).
+#[test]
+fn programmatic_quit_keeps_unsaved_work() {
+    let ctx = egui::Context::default();
+    let send = |a: &mut WordApp, params: Value| match control::handle(a, &ctx, &ControlRequest::new("app.quit", params).0) {
+        control::Outcome::Done(v) => v,
+        _ => json!({"ok": false, "error": "not a reply"}),
+    };
+
+    let mut a = typed();
+    let r = send(&mut a, json!({}));
+    assert_eq!(r["ok"], json!(false), "{r}");
+    let e = r["error"].as_str().unwrap_or("");
+    assert!(e.contains("unsaved") && e.contains("force"), "{e}");
+    assert!(!a.quit_requested, "the app keeps running");
+    assert!(body_text(&a).contains(UNSAVED), "the work stays");
+
+    // Only a real boolean true counts as `force`.
+    for force in [json!("yes"), json!(1)] {
+        let mut a = typed();
+        let r = send(&mut a, json!({"force": force}));
+        assert_eq!(r["ok"], json!(false), "{force}: {r}");
+        assert!(!a.quit_requested, "{force}");
+        assert!(body_text(&a).contains(UNSAVED), "{force}");
+    }
+
+    let r = send(&mut a, json!({"force": true}));
+    assert_eq!(r["ok"], json!(true), "{r}");
+    assert!(a.quit_requested);
+
+    // A clean document quits straight away, without a `force`.
+    let mut clean = app();
+    let r = send(&mut clean, json!({}));
+    assert_eq!(r["ok"], json!(true), "{r}");
+    assert!(clean.quit_requested);
+}
+
+/// Unsaved work in a document that isn't the active one is work a quit would lose too.
+#[test]
+fn programmatic_quit_keeps_the_work_of_a_background_document() {
+    let ctx = egui::Context::default();
+    let mut a = typed();
+    let first = a.session.document_id();
+    assert_eq!(control_execute(&mut a, json!({"command": "file.new"}))["ok"], json!(true));
+    assert_ne!(a.session.document_id(), first);
+    let r = match control::handle(&mut a, &ctx, &ControlRequest::new("app.quit", json!({})).0) {
+        control::Outcome::Done(v) => v,
+        _ => json!({"ok": false, "error": "not a reply"}),
+    };
+    assert_eq!(r["ok"], json!(false), "{r}");
+    assert!(!a.quit_requested);
+    assert!(a.session.is_open(first), "the background document stays open");
+}
+
+/// #3274: `file.closeDocument` closes a document without a prompt, so the unsaved work in the
+/// document it names needs the same explicit `force` as `file.close` rather than going
+/// silently.
+#[test]
+fn programmatic_close_document_keeps_unsaved_work() {
+    let mut a = typed();
+    let first = a.session.document_id();
+
+    let r = control_execute(&mut a, json!({"command": "file.closeDocument"}));
+    assert_eq!(r["ok"], json!(false), "{r}");
+    let e = r["error"].as_str().unwrap_or("");
+    assert!(e.contains("unsaved") && e.contains("force"), "{e}");
+    assert!(a.session.is_open(first), "the document stays open");
+    assert!(body_text(&a).contains(UNSAVED), "the work stays");
+
+    // Only a real boolean true counts as `force`.
+    for force in [json!("yes"), json!(1)] {
+        let mut a = typed();
+        let r = control_execute(&mut a, json!({"command": "file.closeDocument", "params": {"force": force}}));
+        assert_eq!(r["ok"], json!(false), "{force}: {r}");
+        assert!(a.session.is_open(a.session.document_id()), "{force}");
+    }
+
+    let r = control_execute(&mut a, json!({"command": "file.closeDocument", "params": {"force": true}}));
+    assert_eq!(r["ok"], json!(true), "{r}");
+}
+
+/// #3274: `file.closeDocument` names the document it closes, so unsaved work in a document in
+/// the background is refused too, and `force` closes it.
+#[test]
+fn programmatic_close_document_keeps_the_work_of_a_background_document() {
+    let mut a = typed();
+    let first = a.session.document_id();
+    assert_eq!(control_execute(&mut a, json!({"command": "file.new"}))["ok"], json!(true));
+    assert_ne!(a.session.document_id(), first);
+
+    let r = control_execute(&mut a, json!({"command": "file.closeDocument", "params": {"id": first}}));
+    assert_eq!(r["ok"], json!(false), "{r}");
+    let e = r["error"].as_str().unwrap_or("");
+    assert!(e.contains("unsaved") && e.contains("force"), "{e}");
+    assert!(a.session.is_open(first), "the background document stays open");
+
+    let r = control_execute(&mut a, json!({"command": "file.closeDocument", "params": {"id": first, "force": true}}));
+    assert_eq!(r["ok"], json!(true), "{r}");
+    assert!(!a.session.is_open(first), "the background document is closed");
+}
+
 const MAILINGS: [&str; 4] = ["mailings.envelopes", "mailings.labels", "mailings.finish", "mailings.printDocuments"];
 
 /// What the user gave: Envelopes and Labels without anything open their dialogs (#407).

crates/ui-egui/src/windows/tests.rs

@@ -533,6 +533,42 @@ fn closing_one_of_two_windows_on_a_document_asks_nothing() {
     assert!(a.quit_requested, "one prompt for the document");
 }
 
+/// #3274: a programmatic `file.close` on a document shown in two windows closes the window
+/// alone, as the window's own close does: the document and its work stay open, so nothing is
+/// lost and the control channel must not refuse it as unsaved work it would never lose.
+#[test]
+fn a_programmatic_close_of_one_of_two_windows_on_a_document_is_accepted() {
+    let ctx = backend();
+    let mut a = app("Kept");
+    let doc = a.session.document_id();
+    frames(&ctx, &mut a, 2);
+    a.run("view.newWindow", json!({})).unwrap();
+    frames(&ctx, &mut a, 2);
+    assert_eq!(a.window_count(), 2);
+    assert!(a.session.dirty, "the document has unsaved work");
+
+    // The control channel's refusal: it must not stand in the way of a close that keeps the
+    // document open.
+    let refused = crate::control::close_refused(&a, "file.close", &json!({}));
+    assert!(refused.is_none(), "closing a window alone loses nothing: {refused:?}");
+
+    // And the command itself runs, closing the window and leaving the document and its work.
+    let r = crate::control::handle(&mut a, &ctx, &crate::control::ControlRequest::new("engine.execute", json!({"command": "file.close"})).0);
+    let r = match r {
+        crate::control::Outcome::Done(v) => v,
+        _ => json!({"ok": false, "error": "not a reply"}),
+    };
+    assert_eq!(r["ok"], true, "{r}");
+    assert_eq!(a.window_count(), 1, "the window closed");
+    assert_eq!(a.session.document_id(), doc, "the document stays open");
+    assert!(a.session.dirty, "its work stays");
+    assert_eq!(text(&a), "Kept");
+
+    // The last window closes the document, so the same command is refused for its unsaved work.
+    let refused = crate::control::close_refused(&a, "file.close", &json!({}));
+    assert!(refused.is_some(), "closing the document would lose the work: {refused:?}");
+}
+
 /// Two windows, the second (focused) on an untouched blank document: an app whose host
 /// answers file dialogs only when the test does, plus the dialog's answer channel.
 fn blank_beside_a_document() -> (egui::Context, WordApp, Shown, u64) {

docs/control-protocol.md

@@ -145,17 +145,38 @@ an operation that depends on it; do not replay an edit simply because its effect
 
 ### Closing the document
 
-`engine.execute` with `file.close` never shows the Save / Don't Save / Cancel prompt. On a
-document with unsaved changes it replies with an error (the app keeps running and the edits
-stay); save first (`file.save`), or pass `{"force": true}` to discard the changes and close.
-The bare form `{"method": "file.close", "force": true}` (a method name that is a command id) is
-guarded the same way, and only a boolean `true` counts as `force`. A document without unsaved
-changes closes at once (#1268). Closing closes the document and its window, and the window
-focused before it becomes the active one. The last window stays open with no page, and
-`file.new` or `file.open` goes on from there (#1712); closing it doesn't quit the app. With
+`engine.execute` never shows the Save / Don't Save / Cancel prompt, so a command that would
+discard unsaved work is **refused with an error** (the app keeps running and the edits stay);
+save first (`file.save`), or pass `{"force": true}` to discard the changes and go ahead.
+
+`file.close` closes the shown document, so it is refused while that document has unsaved
+changes; the bare form `{"method": "file.close", "force": true}` (a method name that is a
+command id) is guarded the same way, and only a boolean `true` counts as `force`. A document
+without unsaved changes closes at once (#1268). Closing closes the document and its window, and
+the window focused before it becomes the active one. The last window stays open with no page,
+and `file.new` or `file.open` goes on from there (#1712); closing it doesn't quit the app. With
 `doc` naming a document in the background, `file.close` closes that document and its window under
 the same rule (unsaved changes need `{"force": true}`), and the active window stays active.
 
+When another window shows the same document, `file.close` closes only the window it is sent to:
+the document and its unsaved work stay open, so nothing is lost and the command is **not**
+refused (#3274).
+
+`file.new`, `file.open`, `file.recover` (with `restore`) and the mailings merges open their
+result as **another document beside** the open ones and leave the unsaved one alone, so they
+lose nothing and are never refused.
+
+`file.closeDocument` closes the document its `id` names (the active one when the `id` is
+absent), so it is refused while that document has unsaved work: changes that aren't written to
+a file, a save the browser's picker hasn't finished, or a comment still being typed. Pass
+`{"force": true}` to discard that work and close the document; only a boolean `true` counts as
+`force`. The refusal and the Save Changes prompt ask about the same work, so they cannot
+disagree (#3274).
+
+`app.quit` closes every open document, so it is refused while any of them has unsaved changes:
+the app keeps running and the work stays. Save first, or pass `{"force": true}` to discard the
+changes and quit. Only a boolean `true` counts as `force`.
+
 ### Native printing
 
 `engine.execute` can call `ui.printSettings` with `{}` to inspect the printer list, selected