From 8fad295e36aa18dfb5b5510a8f5c36128eb81c0a Mon Sep 17 00:00:00 2001 From: Timon Date: Mon, 4 Aug 2025 00:16:21 +0200 Subject: [PATCH] Make checkboxes not use interior mutability (#2976) * Make checkboxes not use interior mutability * Use copy instead of cloning * Fix --------- Co-authored-by: Keavon Chambers --- .../export_dialog_message_handler.rs | 6 +- .../new_document_dialog_message_handler.rs | 6 +- .../preferences_dialog_message_handler.rs | 22 ++--- .../utility_types/widgets/input_widgets.rs | 44 +++------- .../utility_types/widgets/label_widgets.rs | 13 +-- .../document/document_message_handler.rs | 84 +++++++++---------- .../messages/tool/tool_messages/path_tool.rs | 6 +- .../widgets/labels/TextLabel.svelte | 4 +- frontend/src/messages.ts | 2 +- 9 files changed, 76 insertions(+), 111 deletions(-) diff --git a/editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs b/editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs index 980f3e3e..85a834fc 100644 --- a/editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs +++ b/editor/src/messages/dialog/export_dialog/export_dialog_message_handler.rs @@ -145,14 +145,14 @@ impl LayoutHolder for ExportDialogMessageHandler { DropdownInput::new(entries).selected_index(Some(index as u32)).widget_holder(), ]; - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); let transparent_background = vec![ - TextLabel::new("Transparency").table_align(true).min_width(100).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Transparency").table_align(true).min_width(100).for_checkbox(checkbox_id).widget_holder(), Separator::new(SeparatorType::Unrelated).widget_holder(), CheckboxInput::new(self.transparent_background) .disabled(self.file_type == FileType::Jpg) .on_update(move |value: &CheckboxInput| ExportDialogMessage::TransparentBackground(value.checked).into()) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), ]; diff --git a/editor/src/messages/dialog/new_document_dialog/new_document_dialog_message_handler.rs b/editor/src/messages/dialog/new_document_dialog/new_document_dialog_message_handler.rs index 3b29475c..015b5d2f 100644 --- a/editor/src/messages/dialog/new_document_dialog/new_document_dialog_message_handler.rs +++ b/editor/src/messages/dialog/new_document_dialog/new_document_dialog_message_handler.rs @@ -80,13 +80,13 @@ impl LayoutHolder for NewDocumentDialogMessageHandler { .widget_holder(), ]; - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); let infinite = vec![ - TextLabel::new("Infinite Canvas").table_align(true).min_width(90).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Infinite Canvas").table_align(true).min_width(90).for_checkbox(checkbox_id).widget_holder(), Separator::new(SeparatorType::Unrelated).widget_holder(), CheckboxInput::new(self.infinite) .on_update(|checkbox_input: &CheckboxInput| NewDocumentDialogMessage::Infinite(checkbox_input.checked).into()) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), ]; diff --git a/editor/src/messages/dialog/preferences_dialog/preferences_dialog_message_handler.rs b/editor/src/messages/dialog/preferences_dialog/preferences_dialog_message_handler.rs index 66173d88..26bbd4c2 100644 --- a/editor/src/messages/dialog/preferences_dialog/preferences_dialog_message_handler.rs +++ b/editor/src/messages/dialog/preferences_dialog/preferences_dialog_message_handler.rs @@ -68,7 +68,7 @@ impl PreferencesDialogMessageHandler { .widget_holder(), ]; - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); let zoom_with_scroll_tooltip = "Use the scroll wheel for zooming instead of vertically panning (not recommended for trackpads)"; let zoom_with_scroll = vec![ Separator::new(SeparatorType::Unrelated).widget_holder(), @@ -81,12 +81,12 @@ impl PreferencesDialogMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), TextLabel::new("Zoom with Scroll") .table_align(true) .tooltip(zoom_with_scroll_tooltip) - .for_checkbox(&mut checkbox_id) + .for_checkbox(checkbox_id) .widget_holder(), ]; @@ -169,7 +169,7 @@ impl PreferencesDialogMessageHandler { graph_wire_style, ]; - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); let vello_tooltip = "Use the experimental Vello renderer (your browser must support WebGPU)"; let use_vello = vec![ Separator::new(SeparatorType::Unrelated).widget_holder(), @@ -178,17 +178,17 @@ impl PreferencesDialogMessageHandler { .tooltip(vello_tooltip) .disabled(!preferences.supports_wgpu()) .on_update(|checkbox_input: &CheckboxInput| PreferencesMessage::UseVello { use_vello: checkbox_input.checked }.into()) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), TextLabel::new("Vello Renderer") .table_align(true) .tooltip(vello_tooltip) .disabled(!preferences.supports_wgpu()) - .for_checkbox(&mut checkbox_id) + .for_checkbox(checkbox_id) .widget_holder(), ]; - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); let vector_mesh_tooltip = "Allow tools to produce vector meshes, where more than two segments can connect to an anchor point.\n\nCurrently this does not properly handle stroke joins and fills."; let vector_meshes = vec![ @@ -197,13 +197,9 @@ impl PreferencesDialogMessageHandler { CheckboxInput::new(preferences.vector_meshes) .tooltip(vector_mesh_tooltip) .on_update(|checkbox_input: &CheckboxInput| PreferencesMessage::VectorMeshes { enabled: checkbox_input.checked }.into()) - .for_label(checkbox_id.clone()) - .widget_holder(), - TextLabel::new("Vector Meshes") - .table_align(true) - .tooltip(vector_mesh_tooltip) - .for_checkbox(&mut checkbox_id) + .for_label(checkbox_id) .widget_holder(), + TextLabel::new("Vector Meshes").table_align(true).tooltip(vector_mesh_tooltip).for_checkbox(checkbox_id).widget_holder(), ]; Layout::WidgetLayout(WidgetLayout::new(vec![ diff --git a/editor/src/messages/layout/utility_types/widgets/input_widgets.rs b/editor/src/messages/layout/utility_types/widgets/input_widgets.rs index 9d8ce24b..02d1d3ee 100644 --- a/editor/src/messages/layout/utility_types/widgets/input_widgets.rs +++ b/editor/src/messages/layout/utility_types/widgets/input_widgets.rs @@ -5,8 +5,6 @@ use graphene_std::Color; use graphene_std::raster::curve::Curve; use graphene_std::transform::ReferencePoint; use graphite_proc_macros::WidgetBuilder; -use once_cell::sync::OnceCell; -use std::sync::Arc; #[derive(Clone, Derivative, serde::Serialize, serde::Deserialize, WidgetBuilder, specta::Type)] #[derivative(Debug, PartialEq)] @@ -20,7 +18,7 @@ pub struct CheckboxInput { pub tooltip: String, - #[serde(rename = "forLabel", skip_serializing_if = "checkbox_id_is_empty")] + #[serde(rename = "forLabel")] pub for_label: CheckboxId, #[serde(skip)] @@ -44,19 +42,24 @@ impl Default for CheckboxInput { icon: "Checkmark".into(), tooltip: Default::default(), tooltip_shortcut: Default::default(), - for_label: CheckboxId::default(), + for_label: CheckboxId::new(), on_update: Default::default(), on_commit: Default::default(), } } } -#[derive(Clone, Default, Debug, Eq, PartialEq)] -pub struct CheckboxId(Arc>); +#[derive(Copy, Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)] +pub struct CheckboxId(u64); impl CheckboxId { - pub fn fill(&mut self) { - let _ = self.0.set(graphene_std::uuid::generate_uuid()); + pub fn new() -> Self { + Self(graphene_std::uuid::generate_uuid()) + } +} +impl Default for CheckboxId { + fn default() -> Self { + Self::new() } } impl specta::Type for CheckboxId { @@ -65,31 +68,6 @@ impl specta::Type for CheckboxId { specta::datatype::DataType::Primitive(specta::datatype::PrimitiveType::u64) } } -impl serde::Serialize for CheckboxId { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - self.0.get().copied().serialize(serializer) - } -} -impl<'a> serde::Deserialize<'a> for CheckboxId { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'a>, - { - let optional_id: Option = Option::deserialize(deserializer)?; - // TODO: This is potentially weird because after deserialization the two labels will be decoupled if the value not existent - let id = optional_id.unwrap_or(0); - let checkbox_id = CheckboxId(OnceCell::new().into()); - checkbox_id.0.set(id).map_err(serde::de::Error::custom)?; - Ok(checkbox_id) - } -} - -fn checkbox_id_is_empty(id: &CheckboxId) -> bool { - id.0.get().is_none() -} #[derive(Clone, serde::Serialize, serde::Deserialize, Derivative, WidgetBuilder, specta::Type)] #[derivative(Debug, PartialEq, Default)] diff --git a/editor/src/messages/layout/utility_types/widgets/label_widgets.rs b/editor/src/messages/layout/utility_types/widgets/label_widgets.rs index caeca5a4..f89b6dbe 100644 --- a/editor/src/messages/layout/utility_types/widgets/label_widgets.rs +++ b/editor/src/messages/layout/utility_types/widgets/label_widgets.rs @@ -57,21 +57,12 @@ pub struct TextLabel { pub tooltip: String, - #[serde(rename = "checkboxId")] - #[widget_builder(skip)] - pub checkbox_id: CheckboxId, + #[serde(rename = "forCheckbox")] + pub for_checkbox: CheckboxId, // Body #[widget_builder(constructor)] pub value: String, } -impl TextLabel { - pub fn for_checkbox(mut self, id: &mut CheckboxId) -> Self { - id.fill(); - self.checkbox_id = id.clone(); - self - } -} - // TODO: Add UserInputLabel diff --git a/editor/src/messages/portfolio/document/document_message_handler.rs b/editor/src/messages/portfolio/document/document_message_handler.rs index 1a296249..291d7e55 100644 --- a/editor/src/messages/portfolio/document/document_message_handler.rs +++ b/editor/src/messages/portfolio/document/document_message_handler.rs @@ -2147,7 +2147,7 @@ impl DocumentMessageHandler { }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.artboard_name) .on_update(|optional_input: &CheckboxInput| { @@ -2157,15 +2157,15 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("Artboard Name".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Artboard Name".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.transform_measurement) .on_update(|optional_input: &CheckboxInput| { @@ -2175,9 +2175,9 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("G/R/S Measurement".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("G/R/S Measurement".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, @@ -2186,7 +2186,7 @@ impl DocumentMessageHandler { }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.quick_measurement) .on_update(|optional_input: &CheckboxInput| { @@ -2196,15 +2196,15 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("Quick Measurement".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Quick Measurement".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.transform_cage) .on_update(|optional_input: &CheckboxInput| { @@ -2214,15 +2214,15 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("Transform Cage".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Transform Cage".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.compass_rose) .on_update(|optional_input: &CheckboxInput| { @@ -2232,15 +2232,15 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("Transform Dial".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Transform Dial".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.pivot) .on_update(|optional_input: &CheckboxInput| { @@ -2250,15 +2250,15 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("Transform Pivot".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Transform Pivot".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.pivot) .on_update(|optional_input: &CheckboxInput| { @@ -2268,15 +2268,15 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("Transform Origin".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Transform Origin".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.hover_outline) .on_update(|optional_input: &CheckboxInput| { @@ -2286,15 +2286,15 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("Hover Outline".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Hover Outline".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.selection_outline) .on_update(|optional_input: &CheckboxInput| { @@ -2304,9 +2304,9 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("Selection Outline".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Selection Outline".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, @@ -2315,7 +2315,7 @@ impl DocumentMessageHandler { }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.path) .on_update(|optional_input: &CheckboxInput| { @@ -2325,15 +2325,15 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("Path".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Path".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.anchors) .on_update(|optional_input: &CheckboxInput| { @@ -2343,15 +2343,15 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new("Anchors".to_string()).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new("Anchors".to_string()).for_checkbox(checkbox_id).widget_holder(), ] }, }, LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(self.overlays_visibility_settings.handles) .disabled(!self.overlays_visibility_settings.anchors) @@ -2362,11 +2362,11 @@ impl DocumentMessageHandler { } .into() }) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), TextLabel::new("Handles".to_string()) .disabled(!self.overlays_visibility_settings.anchors) - .for_checkbox(&mut checkbox_id) + .for_checkbox(checkbox_id) .widget_holder(), ] }, @@ -2399,7 +2399,7 @@ impl DocumentMessageHandler { .into_iter() .chain(SNAP_FUNCTIONS_FOR_BOUNDING_BOXES.into_iter().map(|(name, closure, tooltip)| LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(*closure(&mut snapping_state)) .on_update(move |input: &CheckboxInput| { @@ -2410,9 +2410,9 @@ impl DocumentMessageHandler { .into() }) .tooltip(tooltip) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new(name).tooltip(tooltip).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new(name).tooltip(tooltip).for_checkbox(checkbox_id).widget_holder(), ] }, })) @@ -2421,7 +2421,7 @@ impl DocumentMessageHandler { }]) .chain(SNAP_FUNCTIONS_FOR_PATHS.into_iter().map(|(name, closure, tooltip)| LayoutGroup::Row { widgets: { - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); vec![ CheckboxInput::new(*closure(&mut snapping_state2)) .on_update(move |input: &CheckboxInput| { @@ -2432,9 +2432,9 @@ impl DocumentMessageHandler { .into() }) .tooltip(tooltip) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(), - TextLabel::new(name).tooltip(tooltip).for_checkbox(&mut checkbox_id).widget_holder(), + TextLabel::new(name).tooltip(tooltip).for_checkbox(checkbox_id).widget_holder(), ] }, })) diff --git a/editor/src/messages/tool/tool_messages/path_tool.rs b/editor/src/messages/tool/tool_messages/path_tool.rs index b9fbeb67..a0ee4a9c 100644 --- a/editor/src/messages/tool/tool_messages/path_tool.rs +++ b/editor/src/messages/tool/tool_messages/path_tool.rs @@ -236,7 +236,7 @@ impl LayoutHolder for PathTool { }) // TODO: Remove `unwrap_or_default` once checkboxes are capable of displaying a mixed state .unwrap_or_default(); - let mut checkbox_id = CheckboxId::default(); + let checkbox_id = CheckboxId::new(); let colinear_handle_checkbox = CheckboxInput::new(colinear_handles_state) .disabled(!self.tool_data.can_toggle_colinearity) .on_update(|&CheckboxInput { checked, .. }| { @@ -247,12 +247,12 @@ impl LayoutHolder for PathTool { } }) .tooltip(colinear_handles_tooltip) - .for_label(checkbox_id.clone()) + .for_label(checkbox_id) .widget_holder(); let colinear_handles_label = TextLabel::new("Colinear Handles") .disabled(!self.tool_data.can_toggle_colinearity) .tooltip(colinear_handles_tooltip) - .for_checkbox(&mut checkbox_id) + .for_checkbox(checkbox_id) .widget_holder(); let point_editing_mode = CheckboxInput::new(self.options.path_editing_mode.point_editing_mode) diff --git a/frontend/src/components/widgets/labels/TextLabel.svelte b/frontend/src/components/widgets/labels/TextLabel.svelte index 540aa164..4595c6f2 100644 --- a/frontend/src/components/widgets/labels/TextLabel.svelte +++ b/frontend/src/components/widgets/labels/TextLabel.svelte @@ -13,7 +13,7 @@ export let minWidth = 0; export let multiline = false; export let tooltip: string | undefined = undefined; - export let checkboxId: bigint | undefined = undefined; + export let forCheckbox: bigint | undefined = undefined; $: extraClasses = Object.entries(classes) .flatMap(([className, stateName]) => (stateName ? [className] : [])) @@ -34,7 +34,7 @@ style:min-width={minWidth > 0 ? `${minWidth}px` : undefined} style={`${styleName} ${extraStyles}`.trim() || undefined} title={tooltip} - for={checkboxId !== undefined ? `checkbox-input-${checkboxId}` : undefined} + for={forCheckbox !== undefined ? `checkbox-input-${forCheckbox}` : undefined} > diff --git a/frontend/src/messages.ts b/frontend/src/messages.ts index 6b0c6ffa..85fb176d 100644 --- a/frontend/src/messages.ts +++ b/frontend/src/messages.ts @@ -1348,7 +1348,7 @@ export class TextLabel extends WidgetProps { @Transform(({ value }: { value: string }) => value || undefined) tooltip!: string | undefined; - checkboxId!: bigint | undefined; + forCheckbox!: bigint | undefined; } export type ReferencePoint = "None" | "TopLeft" | "TopCenter" | "TopRight" | "CenterLeft" | "Center" | "CenterRight" | "BottomLeft" | "BottomCenter" | "BottomRight";