From 532dc3002814fffb1c36cc7a1209e99f4610596c Mon Sep 17 00:00:00 2001 From: Dennis Kobert Date: Mon, 8 Dec 2025 13:30:29 +0100 Subject: [PATCH] Fix checkboxes by using deterministic widget IDs instead of random ones to make the diffing easier (#3457) * Move diffing to trait * Add deterministic widget ids * Cleanup --- .../messages/layout/layout_message_handler.rs | 11 +- .../layout/utility_types/layout_widget.rs | 276 +++++++++++++++--- .../utility_types/widgets/input_widgets.rs | 5 +- .../utility_types/widgets/label_widgets.rs | 1 - 4 files changed, 244 insertions(+), 49 deletions(-) diff --git a/editor/src/messages/layout/layout_message_handler.rs b/editor/src/messages/layout/layout_message_handler.rs index 7fe27b46c..d3d32abd9 100644 --- a/editor/src/messages/layout/layout_message_handler.rs +++ b/editor/src/messages/layout/layout_message_handler.rs @@ -5,6 +5,7 @@ use graphene_std::raster::color::Color; use graphene_std::text::Font; use graphene_std::vector::style::{FillChoice, GradientStops}; use serde_json::Value; +use std::collections::HashMap; #[derive(ExtractField)] pub struct LayoutMessageContext<'a> { @@ -470,10 +471,18 @@ impl LayoutMessageHandler { fn diff_and_send_layout_to_frontend( &mut self, layout_target: LayoutTarget, - new_layout: Layout, + mut new_layout: Layout, responses: &mut VecDeque, action_input_mapping: &impl Fn(&MessageDiscriminant) -> Option, ) { + // Step 1: Collect CheckboxId mappings from new layout + let mut checkbox_map = HashMap::new(); + new_layout.collect_checkbox_ids(layout_target, &mut Vec::new(), &mut checkbox_map); + + // Step 2: Replace all IDs in new layout with deterministic ones + new_layout.replace_widget_ids(layout_target, &mut Vec::new(), &checkbox_map); + + // Step 3: Diff with deterministic IDs let mut widget_diffs = Vec::new(); self.layouts[layout_target as usize].diff(new_layout, &mut Vec::new(), &mut widget_diffs); diff --git a/editor/src/messages/layout/utility_types/layout_widget.rs b/editor/src/messages/layout/utility_types/layout_widget.rs index 9b3397489..17c3a7acf 100644 --- a/editor/src/messages/layout/utility_types/layout_widget.rs +++ b/editor/src/messages/layout/utility_types/layout_widget.rs @@ -4,6 +4,9 @@ use super::widgets::label_widgets::*; use crate::application::generate_uuid; use crate::messages::input_mapper::utility_types::input_keyboard::KeysGroup; use crate::messages::prelude::*; +use std::collections::HashMap; +use std::collections::hash_map::DefaultHasher; +use std::hash::{Hash, Hasher}; use std::sync::Arc; #[repr(transparent)] @@ -101,6 +104,50 @@ pub trait DialogLayoutHolder: LayoutHolder { } } +/// Trait for types that can compute incremental diffs for UI updates. +/// +/// This trait unifies the diffing behavior across Layout, LayoutGroup, and WidgetInstance, +/// allowing each type to specify how it should be represented in a DiffUpdate. +pub trait Diffable: Clone + PartialEq { + /// Converts this value into a DiffUpdate variant. + fn into_diff_update(self) -> DiffUpdate; + + /// Computes the diff between self (old) and new, updating self and recording changes. + fn diff(&mut self, new: Self, widget_path: &mut Vec, widget_diffs: &mut Vec); + + /// Collects all CheckboxIds currently in use in this layout, computing stable replacements. + fn collect_checkbox_ids(&self, layout_target: LayoutTarget, widget_path: &mut Vec, checkbox_map: &mut HashMap); + + /// Replaces all widget IDs with deterministic IDs based on position and type. + /// Also replaces CheckboxIds using the provided mapping. + fn replace_widget_ids(&mut self, layout_target: LayoutTarget, widget_path: &mut Vec, checkbox_map: &HashMap); +} + +/// Computes a deterministic WidgetId based on layout target, path, and widget type. +fn compute_widget_id(layout_target: LayoutTarget, widget_path: &[usize], widget: &Widget) -> WidgetId { + let mut hasher = DefaultHasher::new(); + + (layout_target as u8).hash(&mut hasher); + widget_path.hash(&mut hasher); + std::mem::discriminant(widget).hash(&mut hasher); + + WidgetId(hasher.finish()) +} + +/// Computes a deterministic CheckboxId based on the same WidgetId algorithm. +fn compute_checkbox_id(layout_target: LayoutTarget, widget_path: &[usize], widget: &Widget) -> CheckboxId { + let mut hasher = DefaultHasher::new(); + + (layout_target as u8).hash(&mut hasher); + widget_path.hash(&mut hasher); + std::mem::discriminant(widget).hash(&mut hasher); + + // Add extra salt for checkbox to differentiate from widget ID + "checkbox".hash(&mut hasher); + + CheckboxId(hasher.finish()) +} + /// Contains an arrangement of widgets mounted somewhere specific in the frontend. #[derive(Debug, Default, Clone, serde::Serialize, serde::Deserialize, PartialEq, specta::Type)] pub struct Layout(pub Vec); @@ -119,9 +166,14 @@ impl Layout { ..Default::default() } } +} - /// Diffing updates self (where self is old) based on new, updating the list of modifications as it does so. - pub fn diff(&mut self, new: Self, widget_path: &mut Vec, widget_diffs: &mut Vec) { +impl Diffable for Layout { + fn into_diff_update(self) -> DiffUpdate { + DiffUpdate::Layout(self) + } + + fn diff(&mut self, new: Self, widget_path: &mut Vec, widget_diffs: &mut Vec) { // Check if the length of items is different // TODO: Diff insersion and deletion of items if self.0.len() != new.0.len() { @@ -129,10 +181,9 @@ impl Layout { self.0.clone_from(&new.0); // Push an update sublayout to the diff - let new = DiffUpdate::Layout(new); widget_diffs.push(WidgetDiff { widget_path: widget_path.to_vec(), - new_value: new, + new_value: new.into_diff_update(), }); return; } @@ -143,6 +194,22 @@ impl Layout { widget_path.pop(); } } + + fn collect_checkbox_ids(&self, layout_target: LayoutTarget, widget_path: &mut Vec, checkbox_map: &mut HashMap) { + for (index, child) in self.0.iter().enumerate() { + widget_path.push(index); + child.collect_checkbox_ids(layout_target, widget_path, checkbox_map); + widget_path.pop(); + } + } + + fn replace_widget_ids(&mut self, layout_target: LayoutTarget, widget_path: &mut Vec, checkbox_map: &HashMap) { + for (index, child) in self.0.iter_mut().enumerate() { + widget_path.push(index); + child.replace_widget_ids(layout_target, widget_path, checkbox_map); + widget_path.pop(); + } + } } #[derive(Debug, Default)] @@ -358,8 +425,20 @@ impl LayoutGroup { if is_col { Self::Column { widgets } } else { Self::Row { widgets } } } - /// Diffing updates self (where self is old) based on new, updating the list of modifications as it does so. - pub fn diff(&mut self, new: Self, widget_path: &mut Vec, widget_diffs: &mut Vec) { + pub fn iter_mut(&mut self) -> WidgetIterMut<'_> { + WidgetIterMut { + stack: vec![self], + ..Default::default() + } + } +} + +impl Diffable for LayoutGroup { + fn into_diff_update(self) -> DiffUpdate { + DiffUpdate::LayoutGroup(self) + } + + fn diff(&mut self, new: Self, widget_path: &mut Vec, widget_diffs: &mut Vec) { let is_column = matches!(new, Self::Column { .. }); match (self, new) { (Self::Column { widgets: current_widgets }, Self::Column { widgets: new_widgets }) | (Self::Row { widgets: current_widgets }, Self::Row { widgets: new_widgets }) => { @@ -370,7 +449,7 @@ impl LayoutGroup { current_widgets.clone_from(&new_widgets); // Push back a LayoutGroup update to the diff - let new_value = DiffUpdate::LayoutGroup(if is_column { Self::Column { widgets: new_widgets } } else { Self::Row { widgets: new_widgets } }); + let new_value = (if is_column { Self::Column { widgets: new_widgets } } else { Self::Row { widgets: new_widgets } }).into_diff_update(); let widget_path = widget_path.to_vec(); widget_diffs.push(WidgetDiff { widget_path, new_value }); return; @@ -418,14 +497,15 @@ impl LayoutGroup { current_layout.clone_from(&new_layout); // Push an update layout group to the diff - let new_value = DiffUpdate::LayoutGroup(Self::Section { + let new_value = Self::Section { name: new_name, description: new_description, visible: new_visible, pinned: new_pinned, id: new_id, layout: new_layout, - }); + } + .into_diff_update(); let widget_path = widget_path.to_vec(); widget_diffs.push(WidgetDiff { widget_path, new_value }); } @@ -440,17 +520,62 @@ impl LayoutGroup { } (current, new) => { *current = new.clone(); - let new_value = DiffUpdate::LayoutGroup(new); + let new_value = new.into_diff_update(); let widget_path = widget_path.to_vec(); widget_diffs.push(WidgetDiff { widget_path, new_value }); } } } - pub fn iter_mut(&mut self) -> WidgetIterMut<'_> { - WidgetIterMut { - stack: vec![self], - ..Default::default() + fn collect_checkbox_ids(&self, layout_target: LayoutTarget, widget_path: &mut Vec, checkbox_map: &mut HashMap) { + match self { + Self::Column { widgets } | Self::Row { widgets } => { + for (index, widget) in widgets.iter().enumerate() { + widget_path.push(index); + widget.collect_checkbox_ids(layout_target, widget_path, checkbox_map); + widget_path.pop(); + } + } + Self::Table { rows, .. } => { + for (row_idx, row) in rows.iter().enumerate() { + for (col_idx, widget) in row.iter().enumerate() { + widget_path.push(row_idx); + widget_path.push(col_idx); + widget.collect_checkbox_ids(layout_target, widget_path, checkbox_map); + widget_path.pop(); + widget_path.pop(); + } + } + } + Self::Section { layout, .. } => { + layout.collect_checkbox_ids(layout_target, widget_path, checkbox_map); + } + } + } + + fn replace_widget_ids(&mut self, layout_target: LayoutTarget, widget_path: &mut Vec, checkbox_map: &HashMap) { + match self { + Self::Column { widgets } | Self::Row { widgets } => { + for (index, widget) in widgets.iter_mut().enumerate() { + widget_path.push(index); + widget.replace_widget_ids(layout_target, widget_path, checkbox_map); + widget_path.pop(); + } + } + Self::Table { rows, .. } => { + for (row_idx, row) in rows.iter_mut().enumerate() { + for (col_idx, widget) in row.iter_mut().enumerate() { + widget_path.push(row_idx); + widget_path.push(col_idx); + widget.replace_widget_ids(layout_target, widget_path, checkbox_map); + widget_path.pop(); + widget_path.pop(); + } + } + } + Self::Section { layout, .. } => { + layout.replace_widget_ids(layout_target, widget_path, checkbox_map); + } } } } @@ -464,7 +589,7 @@ pub struct WidgetInstance { impl PartialEq for WidgetInstance { fn eq(&self, other: &Self) -> bool { - self.widget == other.widget + self.widget_id == other.widget_id && self.widget == other.widget } } @@ -476,40 +601,103 @@ impl WidgetInstance { widget, } } +} - /// Diffing updates self (where self is old) based on new, updating the list of modifications as it does so. - pub fn diff(&mut self, new: Self, widget_path: &mut [usize], widget_diffs: &mut Vec) { - if let (Widget::PopoverButton(button1), Widget::PopoverButton(button2)) = (&mut self.widget, &new.widget) - && button1.disabled == button2.disabled - && button1.style == button2.style - && button1.menu_direction == button2.menu_direction - && button1.icon == button2.icon - && button1.tooltip_label == button2.tooltip_label - && button1.tooltip_description == button2.tooltip_description - && button1.tooltip_shortcut == button2.tooltip_shortcut - && button1.popover_min_width == button2.popover_min_width - { - let mut new_widget_path = widget_path.to_vec(); - for (i, (a, b)) in button1.popover_layout.0.iter_mut().zip(button2.popover_layout.0.iter()).enumerate() { - new_widget_path.push(i); - a.diff(b.clone(), &mut new_widget_path, widget_diffs); - new_widget_path.pop(); - } +impl Diffable for WidgetInstance { + fn into_diff_update(self) -> DiffUpdate { + DiffUpdate::Widget(self) + } + + fn diff(&mut self, new: Self, widget_path: &mut Vec, widget_diffs: &mut Vec) { + if self == &new { + // Still need to update callbacks since PartialEq skips them + self.widget = new.widget; return; } - // If there have been changes to the actual widget (not just the id) - if self.widget != new.widget { - // We should update to the new widget value as well as a new widget id - *self = new.clone(); + // Special handling for PopoverButton: recursively diff nested layout if only the layout changed + if let (Widget::PopoverButton(button1), Widget::PopoverButton(button2)) = (&mut self.widget, &new.widget) { + // Check if only the popover layout changed (all other fields are the same) + if self.widget_id == new.widget_id + && button1.disabled == button2.disabled + && button1.style == button2.style + && button1.menu_direction == button2.menu_direction + && button1.icon == button2.icon + && button1.tooltip_label == button2.tooltip_label + && button1.tooltip_description == button2.tooltip_description + && button1.tooltip_shortcut == button2.tooltip_shortcut + && button1.popover_min_width == button2.popover_min_width + { + // Only the popover layout differs, diff it recursively + for (i, (a, b)) in button1.popover_layout.0.iter_mut().zip(button2.popover_layout.0.iter()).enumerate() { + widget_path.push(i); + a.diff(b.clone(), widget_path, widget_diffs); + widget_path.pop(); + } + return; + } + } - // Push a widget update to the diff - let new_value = DiffUpdate::Widget(new); - let widget_path = widget_path.to_vec(); - widget_diffs.push(WidgetDiff { widget_path, new_value }); - } else { - // Required to update the callback function, which the PartialEq check above skips - self.widget = new.widget; + // Widget or ID changed, send full update + *self = new.clone(); + let new_value = new.into_diff_update(); + let widget_path = widget_path.to_vec(); + widget_diffs.push(WidgetDiff { widget_path, new_value }); + } + + fn collect_checkbox_ids(&self, layout_target: LayoutTarget, widget_path: &mut Vec, checkbox_map: &mut HashMap) { + match &self.widget { + Widget::CheckboxInput(checkbox) => { + // Compute stable ID based on position and insert mapping + let checkbox_id = checkbox.for_label; + let stable_id = compute_checkbox_id(layout_target, widget_path, &self.widget); + checkbox_map.entry(checkbox_id).or_insert(stable_id); + } + Widget::TextLabel(label) => { + // Compute stable ID based on position and insert mapping + let checkbox_id = label.for_checkbox; + let stable_id = compute_checkbox_id(layout_target, widget_path, &self.widget); + checkbox_map.entry(checkbox_id).or_insert(stable_id); + } + Widget::PopoverButton(button) => { + // Recursively collect from nested popover layout + for (index, child) in button.popover_layout.0.iter().enumerate() { + widget_path.push(index); + child.collect_checkbox_ids(layout_target, widget_path, checkbox_map); + widget_path.pop(); + } + } + _ => {} + } + } + + fn replace_widget_ids(&mut self, layout_target: LayoutTarget, widget_path: &mut Vec, checkbox_map: &HashMap) { + // 1. Generate deterministic WidgetId + self.widget_id = compute_widget_id(layout_target, widget_path, &self.widget); + + // 2. Replace CheckboxIds if present + match &mut self.widget { + Widget::CheckboxInput(checkbox) => { + let old_id = checkbox.for_label; + if let Some(&new_id) = checkbox_map.get(&old_id) { + checkbox.for_label = new_id; + } + } + Widget::TextLabel(label) => { + let old_id = label.for_checkbox; + if let Some(&new_id) = checkbox_map.get(&old_id) { + label.for_checkbox = new_id; + } + } + Widget::PopoverButton(button) => { + // Recursively replace in nested popover layout + for (index, child) in button.popover_layout.0.iter_mut().enumerate() { + widget_path.push(index); + child.replace_widget_ids(layout_target, widget_path, checkbox_map); + widget_path.pop(); + } + } + _ => {} } } } 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 ca5bdbe27..c1dfe3d50 100644 --- a/editor/src/messages/layout/utility_types/widgets/input_widgets.rs +++ b/editor/src/messages/layout/utility_types/widgets/input_widgets.rs @@ -26,7 +26,6 @@ pub struct CheckboxInput { pub tooltip_shortcut: Option, #[serde(rename = "forLabel")] - #[derivative(Debug = "ignore", PartialEq = "ignore")] pub for_label: CheckboxId, // Callbacks @@ -55,8 +54,8 @@ impl Default for CheckboxInput { } } -#[derive(Copy, Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)] -pub struct CheckboxId(u64); +#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, serde::Serialize, serde::Deserialize)] +pub struct CheckboxId(pub u64); impl CheckboxId { pub fn new() -> Self { 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 3e21eda93..e91c17c7f 100644 --- a/editor/src/messages/layout/utility_types/widgets/label_widgets.rs +++ b/editor/src/messages/layout/utility_types/widgets/label_widgets.rs @@ -78,7 +78,6 @@ pub struct TextLabel { pub tooltip_shortcut: Option, #[serde(rename = "forCheckbox")] - #[derivative(PartialEq = "ignore")] pub for_checkbox: CheckboxId, // Body