forked from 1b-agent/opencadstudio-upstream
fix(tabs): close by stable document id instead of vector index
Bug: Message::TabClose carried a vector index captured when the view message was built. Closing several tabs in one batch (Close All / Close Others with unsaved-changes dialogs, or a queue of continuation updates) let an earlier close shift the vector before a later TabClose(idx) was handled: self.tabs.remove(idx) either panicked (index past the end) or closed a different tab than the one the user confirmed. Reproduce: 1. Open several drawings and modify each so they all prompt on close. 2. Use Close All, or close two tabs in quick succession so both messages are handled in one batch. 3. Before: panic in remove(idx), or the wrong tab closes while the tab the user chose stays open (or its work is lost). 4. After: the chosen tab closes; an id already closed by an earlier message is a no-op. Fix: TabClose now carries the tab's stable u64 id, resolved to a vector position when the message is handled, not when it is built; PendingClose::Tab stores ids the same way. All construction sites pass tab.id (a raw usize index no longer type-checks), and DocTabCloseAll/DocTabCloseOthers build their queues from live ids at handling time. A missing id means the tab is already gone, so the handler returns Task::none().
This commit is contained in:
1 parent
9c4aa5f896
commit
2d6f4fcfdf
10 files changed
+178
-25
No files matched your search
@@ -141,7 +141,10 @@ impl OpenCADStudio {
|
||||
// CLOSE — close the active drawing tab (with the unsaved-changes
|
||||
// prompt the tab-close handler already runs).
|
||||
"CLOSE" => {
|
||||
return Some(Task::done(Message::TabClose(self.active_tab)));
|
||||
return Some(match self.tabs.get(self.active_tab) {
|
||||
Some(tab) => Task::done(Message::TabClose(tab.id)),
|
||||
None => Task::none(),
|
||||
});
|
||||
}
|
||||
|
||||
// ARCHIVE / ETRANSMIT — package the drawing and its referenced files
|
||||
|
||||
@@ -727,7 +727,7 @@ impl OpenCADStudio {
|
||||
Some(crate::app::ModalKind::BlockDefinition) => Message::BlockDefApply,
|
||||
_ => return Err(failure("no_dialog", "No dialog with an OK button is open")),
|
||||
},
|
||||
"close_document" => Message::TabClose(self.active_tab),
|
||||
"close_document" => Message::TabClose(self.tabs[self.active_tab].id),
|
||||
"toggle_properties" => Message::ToggleProperties,
|
||||
"toggle_layers" => Message::ToggleLayers,
|
||||
"toggle_grid" => Message::ToggleGrid,
|
||||
|
||||
@@ -28,9 +28,12 @@ impl OpenCADStudio {
|
||||
}
|
||||
self.tabs[index].dirty = false;
|
||||
let remaining = self.tabs.len();
|
||||
// Resolve the stable id now; the message may be handled after other
|
||||
// batched messages have shifted `index`.
|
||||
let tab_id = self.tabs[index].id;
|
||||
// The close may queue async work (autosave cleanup) — return the
|
||||
// task so the runtime drives it.
|
||||
let close_task = self.update(Message::TabClose(index));
|
||||
let close_task = self.update(Message::TabClose(tab_id));
|
||||
let closed_document = req["document_id"].as_u64();
|
||||
self.set_control_result(json!({
|
||||
"closed": true,
|
||||
|
||||
+10
-4
@@ -1438,8 +1438,10 @@ pub(super) struct OpenCADStudio {
|
||||
/// What triggered the "unsaved changes" dialog.
|
||||
#[derive(Debug, Clone)]
|
||||
pub(super) enum PendingClose {
|
||||
/// User tried to close the tab at this index.
|
||||
Tab(usize),
|
||||
/// User tried to close the tab with this document id. Ids stay valid
|
||||
/// while the dialog is open; a vector index would go stale the moment
|
||||
/// another tab closes and Discard would remove the wrong tab.
|
||||
Tab(u64),
|
||||
/// User tried to quit the application.
|
||||
Quit,
|
||||
}
|
||||
@@ -2417,8 +2419,12 @@ pub enum Message {
|
||||
to: usize,
|
||||
after: bool,
|
||||
},
|
||||
/// Close the given tab index.
|
||||
TabClose(usize),
|
||||
/// Close the tab with this document id. Carries the stable id, not a
|
||||
/// vector index: iced dispatches every message built from one view
|
||||
/// snapshot before rebuilding it, so an index captured alongside other
|
||||
/// messages can already be stale (and point at a different tab) by the
|
||||
/// time it is handled.
|
||||
TabClose(u64),
|
||||
/// Save every drawing that already has a file path.
|
||||
DocTabSaveAll,
|
||||
/// Close every non-Start drawing tab.
|
||||
|
||||
+106
-2
@@ -149,7 +149,7 @@ pub(super) fn begin_tab_close_queue(&mut self, tab_ids: Vec<u64>) -> Task<Messag
|
||||
continue;
|
||||
}
|
||||
if self.tabs[idx].dirty {
|
||||
self.pending_close = Some(crate::app::PendingClose::Tab(idx));
|
||||
self.pending_close = Some(crate::app::PendingClose::Tab(tab_id));
|
||||
tasks.push(self.open_unsaved_dialog_window());
|
||||
break;
|
||||
}
|
||||
@@ -168,7 +168,8 @@ pub(super) fn on_tab_close(&mut self, idx: usize) -> Task<Message> {
|
||||
// now rather than risk it applying to a different tab's document.
|
||||
self.cancel_attr_editor();
|
||||
if self.tabs.get(idx).map_or(false, |t| t.dirty) {
|
||||
self.pending_close = Some(crate::app::PendingClose::Tab(idx));
|
||||
self.pending_close =
|
||||
Some(crate::app::PendingClose::Tab(self.tabs[idx].id));
|
||||
return self.open_unsaved_dialog_window();
|
||||
}
|
||||
// Pending client interactive requests pinned to this drawing
|
||||
@@ -4559,3 +4560,106 @@ mod plot_style_ctb_guard_tests {
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Reproductions for the stale-tab-index crashes: iced dispatches every
|
||||
/// message produced from one view snapshot before rebuilding it, so a second
|
||||
/// `TabClose` (or a `PendingClose::Tab` stashed while the unsaved dialog is
|
||||
/// open) can carry an index the first removal already invalidated.
|
||||
#[cfg(test)]
|
||||
mod tab_close_tests {
|
||||
use crate::app::{Message, OpenCADStudio};
|
||||
|
||||
/// `[Start, Drawing A, Drawing B]` — three clean tabs.
|
||||
fn three_tabs() -> OpenCADStudio {
|
||||
let mut app = OpenCADStudio::new_for_test();
|
||||
app.push_test_document();
|
||||
app.push_test_document();
|
||||
assert_eq!(app.tabs.len(), 3, "start page plus two drawings");
|
||||
assert!(!app.tabs[1].is_start && !app.tabs[2].is_start);
|
||||
app
|
||||
}
|
||||
|
||||
/// Two close messages built from the same view snapshot: the first one
|
||||
/// removes the tab, the second carries the now-outdated reference. The
|
||||
/// close request must degrade to a no-op, not panic.
|
||||
#[test]
|
||||
fn stale_tab_close_is_a_no_op() {
|
||||
let mut app = three_tabs();
|
||||
let id = app.tabs[app.tabs.len() - 1].id;
|
||||
|
||||
let _ = app.update(Message::TabClose(id));
|
||||
assert_eq!(app.tabs.len(), 2, "first close removed the tab");
|
||||
|
||||
// Same target again, as the batched second message from one snapshot.
|
||||
let _ = app.update(Message::TabClose(id));
|
||||
assert_eq!(
|
||||
app.tabs.len(),
|
||||
2,
|
||||
"stale close must not remove another tab or panic"
|
||||
);
|
||||
}
|
||||
|
||||
/// Double-clicking one tab's × in a single batch must close that tab
|
||||
/// exactly once; the second message must not slide onto the tab that
|
||||
/// shifted into its place.
|
||||
#[test]
|
||||
fn double_close_from_one_snapshot_closes_only_the_clicked_tab() {
|
||||
let mut app = three_tabs();
|
||||
let first_id = app.tabs[1].id;
|
||||
let other_id = app.tabs[2].id;
|
||||
|
||||
let _ = app.update(Message::TabClose(first_id));
|
||||
let _ = app.update(Message::TabClose(first_id));
|
||||
|
||||
assert!(
|
||||
app.tabs.iter().all(|t| t.id != first_id),
|
||||
"the clicked tab must be closed"
|
||||
);
|
||||
assert!(
|
||||
app.tabs.iter().any(|t| t.id == other_id),
|
||||
"the tab that shifted into index 1 must survive"
|
||||
);
|
||||
}
|
||||
|
||||
/// The unsaved dialog stashes a close target while it is open. Another
|
||||
/// close landing before the user answers shifts the vector, so Discard
|
||||
/// must still discard the tab the dialog was opened for.
|
||||
#[test]
|
||||
fn unsaved_dialog_discard_closes_the_tab_it_was_opened_for() {
|
||||
let mut app = OpenCADStudio::new_for_test();
|
||||
app.push_test_document(); // A
|
||||
app.push_test_document(); // B
|
||||
app.push_test_document(); // C
|
||||
assert_eq!(app.tabs.len(), 4);
|
||||
let a_id = app.tabs[1].id;
|
||||
let b_id = app.tabs[2].id;
|
||||
let c_id = app.tabs[3].id;
|
||||
app.tabs[2].dirty = true;
|
||||
|
||||
// Close dirty B: opens the dialog and stashes the pending target.
|
||||
let _ = app.update(Message::TabClose(b_id));
|
||||
assert!(
|
||||
app.pending_close.is_some(),
|
||||
"dirty close opens the unsaved dialog"
|
||||
);
|
||||
|
||||
// A second close lands while the dialog is pending (batched message /
|
||||
// automation `close` / save continuation) and shifts indices.
|
||||
let _ = app.update(Message::TabClose(a_id));
|
||||
assert!(app.tabs.iter().all(|t| t.id != a_id));
|
||||
|
||||
// User picks Discard.
|
||||
let _ = app.update(Message::UnsavedDialogDiscard);
|
||||
|
||||
assert!(
|
||||
app.tabs.iter().all(|t| t.id != b_id),
|
||||
"the dirty tab the dialog was opened for must be discarded"
|
||||
);
|
||||
assert!(
|
||||
app.tabs.iter().any(|t| t.id == c_id),
|
||||
"C must survive the stale pending index"
|
||||
);
|
||||
assert!(app.pending_close.is_none(), "pending target consumed");
|
||||
assert!(app.active_tab < app.tabs.len(), "active tab stays valid");
|
||||
}
|
||||
}
|
||||
@@ -200,8 +200,14 @@ pub(super) fn on_ribbon_tool_click(&mut self, tool_id: String, event: ModuleEven
|
||||
|
||||
pub(super) fn on_unsaved_dialog_discard(&mut self) -> Task<Message> {
|
||||
match self.pending_close.take() {
|
||||
Some(crate::app::PendingClose::Tab(idx)) => {
|
||||
Some(crate::app::PendingClose::Tab(tab_id)) => {
|
||||
let close_win = self.close_unsaved_dialog_window();
|
||||
// Resolve the id now: the tab may have closed while
|
||||
// the dialog was open (batched close, automation),
|
||||
// in which case there is nothing left to discard.
|
||||
let Some(idx) = self.tabs.iter().position(|t| t.id == tab_id) else {
|
||||
return Task::batch([close_win, self.continue_tab_close_queue()]);
|
||||
};
|
||||
// Discarded — drop this tab's autosave recovery copy.
|
||||
#[cfg(not(target_arch = "wasm32"))]
|
||||
let _ = std::fs::remove_file(self.autosave_target(idx));
|
||||
@@ -249,7 +255,17 @@ pub(super) fn on_ribbon_tool_click(&mut self, tool_id: String, event: ModuleEven
|
||||
return Task::none();
|
||||
};
|
||||
let (idx, continuation) = match pending {
|
||||
crate::app::PendingClose::Tab(idx) => {
|
||||
crate::app::PendingClose::Tab(tab_id) => {
|
||||
// The pending target is an id; resolve it against the
|
||||
// current tab list so a tab closed in the meantime can
|
||||
// neither panic the save path nor save the wrong file.
|
||||
let Some(idx) = self.tabs.iter().position(|t| t.id == tab_id) else {
|
||||
self.pending_close = None;
|
||||
return Task::batch([
|
||||
self.close_unsaved_dialog_window(),
|
||||
self.continue_tab_close_queue(),
|
||||
]);
|
||||
};
|
||||
(idx, crate::app::SaveContinuation::CloseTab)
|
||||
}
|
||||
crate::app::PendingClose::Quit => {
|
||||
@@ -298,8 +314,14 @@ pub(super) fn on_ribbon_tool_click(&mut self, tool_id: String, event: ModuleEven
|
||||
#[cfg(target_arch = "wasm32")]
|
||||
{
|
||||
match self.pending_close.take() {
|
||||
Some(crate::app::PendingClose::Tab(idx)) => {
|
||||
self.pending_close = Some(crate::app::PendingClose::Tab(idx));
|
||||
Some(crate::app::PendingClose::Tab(tab_id)) => {
|
||||
let Some(idx) = self.tabs.iter().position(|t| t.id == tab_id) else {
|
||||
return Task::batch([
|
||||
self.close_unsaved_dialog_window(),
|
||||
self.continue_tab_close_queue(),
|
||||
]);
|
||||
};
|
||||
self.pending_close = Some(crate::app::PendingClose::Tab(tab_id));
|
||||
self.save_dialog_for_unsaved = true;
|
||||
let close = self.close_unsaved_dialog_window();
|
||||
let save = self.save_with_default_format(idx);
|
||||
|
||||
+11
-7
@@ -2226,8 +2226,8 @@ impl OpenCADStudio {
|
||||
};
|
||||
if self.save_dialog_for_unsaved {
|
||||
if saved {
|
||||
if let Some(crate::app::PendingClose::Tab(index)) = self.pending_close.take() {
|
||||
let continuation = self.update(Message::TabClose(index));
|
||||
if let Some(crate::app::PendingClose::Tab(tab_id)) = self.pending_close.take() {
|
||||
let continuation = self.update(Message::TabClose(tab_id));
|
||||
let rest = self.continue_tab_close_queue();
|
||||
return Task::batch([recent_task, continuation, rest]);
|
||||
}
|
||||
@@ -2649,7 +2649,8 @@ impl OpenCADStudio {
|
||||
});
|
||||
match outcome.continuation {
|
||||
crate::app::SaveContinuation::CloseTab => {
|
||||
self.pending_close = Some(crate::app::PendingClose::Tab(i));
|
||||
self.pending_close =
|
||||
Some(crate::app::PendingClose::Tab(self.tabs[i].id));
|
||||
}
|
||||
crate::app::SaveContinuation::Quit => {
|
||||
self.pending_close = Some(crate::app::PendingClose::Quit);
|
||||
@@ -2663,7 +2664,7 @@ impl OpenCADStudio {
|
||||
.push_error(crate::tf!("Save failed: {error}").as_ref());
|
||||
return match outcome.continuation {
|
||||
crate::app::SaveContinuation::CloseTab => {
|
||||
self.pending_close = Some(crate::app::PendingClose::Tab(i));
|
||||
self.pending_close = Some(crate::app::PendingClose::Tab(self.tabs[i].id));
|
||||
self.open_unsaved_dialog_window()
|
||||
}
|
||||
crate::app::SaveContinuation::Quit => {
|
||||
@@ -2754,7 +2755,7 @@ impl OpenCADStudio {
|
||||
crate::app::SaveContinuation::CloseTab if snapshot_is_current => {
|
||||
self.pending_close = None;
|
||||
tasks.push(self.close_unsaved_dialog_window());
|
||||
tasks.push(self.update(Message::TabClose(i)));
|
||||
tasks.push(self.update(Message::TabClose(self.tabs[i].id)));
|
||||
tasks.push(self.continue_tab_close_queue());
|
||||
}
|
||||
crate::app::SaveContinuation::Quit if snapshot_is_current => {
|
||||
@@ -2768,7 +2769,7 @@ impl OpenCADStudio {
|
||||
}
|
||||
}
|
||||
crate::app::SaveContinuation::CloseTab => {
|
||||
self.pending_close = Some(crate::app::PendingClose::Tab(i));
|
||||
self.pending_close = Some(crate::app::PendingClose::Tab(self.tabs[i].id));
|
||||
tasks.push(self.open_unsaved_dialog_window());
|
||||
}
|
||||
crate::app::SaveContinuation::Quit => {
|
||||
@@ -2787,7 +2788,10 @@ impl OpenCADStudio {
|
||||
) {
|
||||
self.pending_close = match continuation {
|
||||
crate::app::SaveContinuation::None => None,
|
||||
crate::app::SaveContinuation::CloseTab => Some(crate::app::PendingClose::Tab(tab_idx)),
|
||||
crate::app::SaveContinuation::CloseTab => self
|
||||
.tabs
|
||||
.get(tab_idx)
|
||||
.map(|tab| crate::app::PendingClose::Tab(tab.id)),
|
||||
crate::app::SaveContinuation::Quit => Some(crate::app::PendingClose::Quit),
|
||||
};
|
||||
}
|
||||
|
||||
+12
-2
@@ -1954,13 +1954,23 @@ impl OpenCADStudio {
|
||||
Task::none()
|
||||
}
|
||||
|
||||
Message::TabClose(idx) => {
|
||||
Message::TabClose(tab_id) => {
|
||||
self.hovered_doc_tab = None;
|
||||
self.on_tab_close(idx)
|
||||
// Resolve the stable id when the message is handled, not when
|
||||
// it was built: an earlier close in the same batch may have
|
||||
// shifted or removed the tab a captured index referred to.
|
||||
match self.tabs.iter().position(|tab| tab.id == tab_id) {
|
||||
Some(idx) => self.on_tab_close(idx),
|
||||
// Already closed by an earlier message — nothing to do.
|
||||
None => Task::none(),
|
||||
}
|
||||
}
|
||||
|
||||
Message::DocTabSaveAll => self.dispatch_command("SAVEALL"),
|
||||
|
||||
// Both close-* variants resolve ids against the live tab list at
|
||||
// handling time (like `TabClose` above), so a batch that closed
|
||||
// tabs earlier cannot leave them stale.
|
||||
Message::DocTabCloseAll => {
|
||||
let ids = self
|
||||
.tabs
|
||||
|
||||
+1
-1
@@ -3127,7 +3127,7 @@ pub(super) fn doc_tab_bar<'a>(
|
||||
row![title_btn].spacing(0).align_y(iced::Center)
|
||||
} else {
|
||||
let close_btn = button(text("×").size(12))
|
||||
.on_press(Message::TabClose(idx))
|
||||
.on_press(Message::TabClose(tab.id))
|
||||
.height(Fill)
|
||||
.padding([5, 9])
|
||||
.style(move |theme: &Theme, status| {
|
||||
|
||||
@@ -1632,9 +1632,10 @@ impl OpenCADStudio {
|
||||
}
|
||||
super::super::ModalKind::Unsaved => {
|
||||
let tab_name = match &self.pending_close {
|
||||
Some(super::super::PendingClose::Tab(idx)) => self
|
||||
Some(super::super::PendingClose::Tab(tab_id)) => self
|
||||
.tabs
|
||||
.get(*idx)
|
||||
.iter()
|
||||
.find(|t| t.id == *tab_id)
|
||||
.map(|t| t.tab_display_name())
|
||||
.unwrap_or_default(),
|
||||
Some(super::super::PendingClose::Quit) => self
|
||||
|
||||
Reference in new issue
Block a user