From b4e2f26ebcd4d6e99960ff11a7484cb388d54b9c Mon Sep 17 00:00:00 2001 From: Michael Mauderer Date: Fri, 3 Mar 2023 15:49:16 +0000 Subject: [PATCH 1/3] Implement fix for duplicate entries in visualisation list. --- Cargo.lock | 1 + app/gui/view/graph-editor/Cargo.toml | 3 ++ .../src/component/visualization/definition.rs | 8 +++++ .../src/component/visualization/registry.rs | 30 ++++++++++++++++++- 4 files changed, 41 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 05472d3172da..7d86a99e529a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4322,6 +4322,7 @@ dependencies = [ "span-tree", "uuid 0.8.2", "wasm-bindgen", + "wasm-bindgen-test", "web-sys", ] diff --git a/app/gui/view/graph-editor/Cargo.toml b/app/gui/view/graph-editor/Cargo.toml index 4cd1c9324ae6..fe8df83bbb0f 100644 --- a/app/gui/view/graph-editor/Cargo.toml +++ b/app/gui/view/graph-editor/Cargo.toml @@ -38,3 +38,6 @@ serde-wasm-bindgen = { workspace = true } [dependencies.web-sys] version = "0.3.4" features = ["TextMetrics", 'CanvasRenderingContext2d', 'HtmlHeadElement'] + +[dev-dependencies] +wasm-bindgen-test = "0.3.8" diff --git a/app/gui/view/graph-editor/src/component/visualization/definition.rs b/app/gui/view/graph-editor/src/component/visualization/definition.rs index af07ef14c60c..d339c1302186 100644 --- a/app/gui/view/graph-editor/src/component/visualization/definition.rs +++ b/app/gui/view/graph-editor/src/component/visualization/definition.rs @@ -87,6 +87,14 @@ impl Definition { } } +impl PartialEq for Definition { + fn eq(&self, other: &Self) -> bool { + self.signature == other.signature && Rc::ptr_eq(&self.constructor, &other.constructor) + } +} + +impl Eq for Definition {} + // === Result === diff --git a/app/gui/view/graph-editor/src/component/visualization/registry.rs b/app/gui/view/graph-editor/src/component/visualization/registry.rs index 0499c3e2c535..f1308f0c56c5 100644 --- a/app/gui/view/graph-editor/src/component/visualization/registry.rs +++ b/app/gui/view/graph-editor/src/component/visualization/registry.rs @@ -70,7 +70,11 @@ impl Registry { type_map.get(tp).cloned().unwrap_or_default(); if tp != &any_type { if let Some(vis_for_any) = type_map.get(&any_type) { - result.extend(vis_for_any.iter().cloned()); + for vis in vis_for_any { + if !result.contains(vis) { + result.push(vis.clone_ref()); + } + } } } result @@ -130,3 +134,27 @@ impl Default for Registry { Registry::new() } } + +#[cfg(test)] +mod tests { + use super::*; + + use wasm_bindgen_test::wasm_bindgen_test; + + fn assert_no_duplicates(items: &[T]) { + let vec_length = items.len(); + let set: HashSet = items.iter().cloned().collect(); + assert_eq!(set.len(), vec_length, "Duplicate entries found: {items:?}"); + } + + #[wasm_bindgen_test] + fn assert_no_duplicate_default_visualisations() { + let registry = Registry::new(); + registry.add_default_visualizations(); + + for entry in registry.type_map.borrow().values() { + let signatures = entry.iter().map(|class| class.signature.clone()).collect_vec(); + assert_no_duplicates(&signatures); + } + } +} From 96fabb54af47225092980f352bccc07a7a5b700c Mon Sep 17 00:00:00 2001 From: Michael Mauderer Date: Mon, 6 Mar 2023 14:47:57 +0000 Subject: [PATCH 2/3] Remove unreliable comparison. --- .../view/graph-editor/src/component/visualization/definition.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/gui/view/graph-editor/src/component/visualization/definition.rs b/app/gui/view/graph-editor/src/component/visualization/definition.rs index d339c1302186..d69ef11b76f1 100644 --- a/app/gui/view/graph-editor/src/component/visualization/definition.rs +++ b/app/gui/view/graph-editor/src/component/visualization/definition.rs @@ -89,7 +89,7 @@ impl Definition { impl PartialEq for Definition { fn eq(&self, other: &Self) -> bool { - self.signature == other.signature && Rc::ptr_eq(&self.constructor, &other.constructor) + self.signature == other.signature } } From 1afec4d6cbc630f8aa72c9d71f9f9cfdd2452b65 Mon Sep 17 00:00:00 2001 From: Michael Mauderer Date: Tue, 7 Mar 2023 11:05:19 +0000 Subject: [PATCH 3/3] Use IndexSet to deduplicate visualisation items. --- Cargo.lock | 1 + app/gui/view/graph-editor/Cargo.toml | 15 ++++++++------- .../src/component/visualization/definition.rs | 12 +++--------- .../src/component/visualization/registry.rs | 13 +++++-------- 4 files changed, 17 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7d86a99e529a..86dd5d82839d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4312,6 +4312,7 @@ dependencies = [ "ensogl-hardcoded-theme", "ensogl-text-msdf", "failure", + "indexmap", "js-sys", "nalgebra", "ordered-float", diff --git a/app/gui/view/graph-editor/Cargo.toml b/app/gui/view/graph-editor/Cargo.toml index fe8df83bbb0f..6fcab0d4780d 100644 --- a/app/gui/view/graph-editor/Cargo.toml +++ b/app/gui/view/graph-editor/Cargo.toml @@ -12,28 +12,29 @@ analytics = { path = "../../analytics" } ast = { path = "../../language/ast/impl" } base64 = "0.13" bimap = { version = "0.4.0" } +engine-protocol = { path = "../../controller/engine-protocol" } enso-config = { path = "../../config" } enso-frp = { path = "../../../../lib/rust/frp" } enso-prelude = { path = "../../../../lib/rust/prelude" } -engine-protocol = { path = "../../controller/engine-protocol" } enso-shapely = { path = "../../../../lib/rust/shapely" } enso-text = { path = "../../../../lib/rust/text" } ensogl = { path = "../../../../lib/rust/ensogl" } ensogl-component = { path = "../../../../lib/rust/ensogl/component" } -ensogl-text-msdf = { path = "../../../../lib/rust/ensogl/component/text/src/font/msdf" } -ensogl-hardcoded-theme = { path = "../../../../lib/rust/ensogl/app/theme/hardcoded" } ensogl-drop-manager = { path = "../../../../lib/rust/ensogl/component/drop-manager" } +ensogl-hardcoded-theme = { path = "../../../../lib/rust/ensogl/app/theme/hardcoded" } +ensogl-text-msdf = { path = "../../../../lib/rust/ensogl/component/text/src/font/msdf" } failure = { workspace = true } -ordered-float = { workspace = true } -span-tree = { path = "../../language/span-tree" } +indexmap = "1.9.2" js-sys = { workspace = true } nalgebra = { workspace = true } -serde_json = { workspace = true } +ordered-float = { workspace = true } serde = { version = "1.0", features = ["derive"] } +serde-wasm-bindgen = { workspace = true } +serde_json = { workspace = true } sourcemap = "6.0" +span-tree = { path = "../../language/span-tree" } uuid = { version = "0.8", features = ["serde", "v4", "wasm-bindgen"] } wasm-bindgen = { workspace = true } -serde-wasm-bindgen = { workspace = true } [dependencies.web-sys] version = "0.3.4" diff --git a/app/gui/view/graph-editor/src/component/visualization/definition.rs b/app/gui/view/graph-editor/src/component/visualization/definition.rs index d69ef11b76f1..a1be6ea46f32 100644 --- a/app/gui/view/graph-editor/src/component/visualization/definition.rs +++ b/app/gui/view/graph-editor/src/component/visualization/definition.rs @@ -59,11 +59,13 @@ impl Signature { /// Generic definition of a visualization. Provides information about the visualization `Signature`, /// and a way to create new instances. #[derive(Clone, CloneRef, Derivative)] -#[derivative(Debug)] +#[derivative(Debug, PartialEq, Eq, Hash)] #[allow(missing_docs)] pub struct Definition { pub signature: Signature, #[derivative(Debug = "ignore")] + #[derivative(PartialEq = "ignore")] + #[derivative(Hash = "ignore")] pub constructor: Rc InstantiationResult>, } @@ -87,14 +89,6 @@ impl Definition { } } -impl PartialEq for Definition { - fn eq(&self, other: &Self) -> bool { - self.signature == other.signature - } -} - -impl Eq for Definition {} - // === Result === diff --git a/app/gui/view/graph-editor/src/component/visualization/registry.rs b/app/gui/view/graph-editor/src/component/visualization/registry.rs index f1308f0c56c5..d15e79cefdf3 100644 --- a/app/gui/view/graph-editor/src/component/visualization/registry.rs +++ b/app/gui/view/graph-editor/src/component/visualization/registry.rs @@ -66,18 +66,15 @@ impl Registry { pub fn valid_sources(&self, tp: &enso::Type) -> Vec { let type_map = self.type_map.borrow(); let any_type = enso::Type::any(); - let mut result: Vec = - type_map.get(tp).cloned().unwrap_or_default(); + // IndexSet preserves insertion order. + let mut result: indexmap::IndexSet = default(); + result.extend(type_map.get(tp).cloned().unwrap_or_default()); if tp != &any_type { if let Some(vis_for_any) = type_map.get(&any_type) { - for vis in vis_for_any { - if !result.contains(vis) { - result.push(vis.clone_ref()); - } - } + result.extend(vis_for_any.iter().cloned()); } } - result + result.into_iter().collect() } /// Return the `visualization::Definition` that should be used as default for the given type.