From 91219cbe643ce1468b5a570f8aeb6e6e4cee0f28 Mon Sep 17 00:00:00 2001 From: Alexi Date: Fri, 24 Jun 2022 09:35:07 -0500 Subject: [PATCH] Fix layer deletion bugs when tools are in use (#684) * Added test case of layer delete bug * Fixed crash in `PenTool` Updated the document `MessageHandler` to cancel all active tools if the layer is deleted. This prevents the tools from crashing due to the layer being pulled from under them. * Moved Abort into pre-graphene DeleteLayer message * Renamed test case for clarity * Moved tool crash tests to the `tools` module * Added `test-case` to the dev dependencies * Added crash test case for all tools * Ran cargo fmt Co-authored-by: otdavies Co-authored-by: Keavon Chambers --- Cargo.lock | 53 +++++++++++++++++++ editor/Cargo.toml | 1 + .../src/document/document_message_handler.rs | 1 + editor/src/document/transformation.rs | 6 ++- editor/src/viewport_tools/mod.rs | 29 ++++++++++ 5 files changed, 89 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index a7d2d6d3..5e6c0847 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -146,6 +146,7 @@ dependencies = [ "serde", "serde_json", "spin", + "test-case", "thiserror", ] @@ -267,6 +268,30 @@ version = "0.2.16" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "eb9f9e6e233e5c4a35559a617bf40a4ec447db2e84c20b55a6f83167b7e57872" +[[package]] +name = "proc-macro-error" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c" +dependencies = [ + "proc-macro-error-attr", + "proc-macro2", + "quote", + "syn", + "version_check", +] + +[[package]] +name = "proc-macro-error-attr" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869" +dependencies = [ + "proc-macro2", + "quote", + "version_check", +] + [[package]] name = "proc-macro2" version = "1.0.37" @@ -441,6 +466,28 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "test-case" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "196e8a70562e252cc51eaaaee3ecddc39803d9b7fd4a772b7c7dae7cdf42a859" +dependencies = [ + "test-case-macros", +] + +[[package]] +name = "test-case-macros" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8dd461f47ade621665c9f4e44b20449341769911c253275dc5cb03726cbb852c" +dependencies = [ + "cfg-if", + "proc-macro-error", + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "thiserror" version = "1.0.30" @@ -497,6 +544,12 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8ccb82d61f80a663efe1f787a51b16b5a51e3314d6ac365b08639f52387b33f3" +[[package]] +name = "version_check" +version = "0.9.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f" + [[package]] name = "wasm-bindgen" version = "0.2.80" diff --git a/editor/Cargo.toml b/editor/Cargo.toml index b06918aa..5927ba4d 100644 --- a/editor/Cargo.toml +++ b/editor/Cargo.toml @@ -32,3 +32,4 @@ package = "graphite-graphene" [dev-dependencies] env_logger = "0.8.4" +test-case = "2.1" diff --git a/editor/src/document/document_message_handler.rs b/editor/src/document/document_message_handler.rs index 9f440623..cc5e4488 100644 --- a/editor/src/document/document_message_handler.rs +++ b/editor/src/document/document_message_handler.rs @@ -991,6 +991,7 @@ impl MessageHandler { responses.push_front(DocumentOperation::DeleteLayer { path: layer_path.clone() }.into()); + responses.push_front(ToolMessage::AbortCurrentTool.into()); responses.push_back(PropertiesPanelMessage::CheckSelectedWasDeleted { path: layer_path }.into()); } DeleteSelectedLayers => { diff --git a/editor/src/document/transformation.rs b/editor/src/document/transformation.rs index fe752065..a6d34eea 100644 --- a/editor/src/document/transformation.rs +++ b/editor/src/document/transformation.rs @@ -199,7 +199,11 @@ impl<'a> Selected<'a> { pub fn new(original_transforms: &'a mut OriginalTransforms, pivot: &'a mut DVec2, selected: &'a [&'a Vec], responses: &'a mut VecDeque, document: &'a Document) -> Self { for path in selected { if !original_transforms.contains_key(*path) { - original_transforms.insert(path.to_vec(), document.layer(path).unwrap().transform); + if let Ok(layer) = document.layer(path) { + original_transforms.insert(path.to_vec(), layer.transform); + } else { + log::warn!("Didn't find a layer for {:?}", path); + } } } Self { diff --git a/editor/src/viewport_tools/mod.rs b/editor/src/viewport_tools/mod.rs index b52cadf5..a10c7123 100644 --- a/editor/src/viewport_tools/mod.rs +++ b/editor/src/viewport_tools/mod.rs @@ -4,3 +4,32 @@ pub mod tool_message; pub mod tool_message_handler; pub mod tools; pub mod vector_editor; + +#[cfg(test)] +mod tool_crash_on_layer_delete_tests { + use crate::communication::set_uuid_seed; + use crate::misc::test_utils::EditorTestUtils; + use crate::viewport_tools::tool::ToolType; + use crate::{DocumentMessage, Editor}; + + use test_case::test_case; + + #[test_case(ToolType::Pen ; "while using pen tool")] + #[test_case(ToolType::Freehand ; "while using freehand tool")] + #[test_case(ToolType::Spline ; "while using spline tool")] + #[test_case(ToolType::Line ; "while using line tool")] + #[test_case(ToolType::Rectangle ; "while using rectangle tool")] + #[test_case(ToolType::Ellipse ; "while using ellipse tool")] + #[test_case(ToolType::Shape ; "while using shape tool")] + #[test_case(ToolType::Path ; "while using path tool")] + fn should_not_crash_when_layer_is_deleted(tool: ToolType) { + set_uuid_seed(0); + let mut test_editor = Editor::new(); + + test_editor.select_tool(tool); + test_editor.lmb_mousedown(0.0, 0.0); + test_editor.move_mouse(100.0, 100.0); + + test_editor.handle_message(DocumentMessage::DeleteSelectedLayers); + } +}