Refactor the old menu bar plumbing to use standard TextButtons (#3444)

* Refactor the old menu bar plumbing to use standard TextButtons

* WIP: Fix Mac native menu bar

* WIP: fix desktop menu bar mac

* Refactor menu bar definitions to use the builder pattern

* WIP: fixup desktop

* cleanup

* fix linux

* Remove dead code that was failing to lint

---------

Co-authored-by: Timon Schelling <me@timon.zip>

editor/src/messages/layout/layout_message_handler.rs

@@ -25,7 +25,6 @@ impl MessageHandler<LayoutMessage, LayoutMessageContext<'_>> for LayoutMessageHa
 			LayoutMessage::ResendActiveWidget { layout_target, widget_id } => {
 				// Find the updated diff based on the specified layout target
 				let Some(diff) = (match &self.layouts[layout_target as usize] {
-					Layout::MenuLayout(_) => return,
 					Layout::WidgetLayout(layout) => Self::get_widget_path(layout, widget_id).map(|(widget, widget_path)| {
 						// Create a widget update diff for the relevant id
 						let new_value = DiffUpdate::Widget(widget.clone());
@@ -112,7 +111,10 @@ impl LayoutMessageHandler {
 			return;
 		};
 
-		let Some(widget_holder) = layout.iter_mut().find(|widget| widget.widget_id == widget_id) else {
+		let mut layout_iter = match layout {
+			Layout::WidgetLayout(widget_layout) => widget_layout.iter_mut(),
+		};
+		let Some(widget_holder) = layout_iter.find(|widget| widget.widget_id == widget_id) else {
 			warn!("handle_widget_callback was called referencing an invalid widget ID, although the layout target was valid. `widget_id: {widget_id}`, `layout_target: {layout_target:?}`",);
 			return;
 		};
@@ -309,14 +311,6 @@ impl LayoutMessageHandler {
 			}
 			Widget::ImageLabel(_) => {}
 			Widget::IconLabel(_) => {}
-			Widget::InvisibleStandinInput(invisible) => {
-				let callback_message = match action {
-					WidgetValueAction::Commit => (invisible.on_commit.callback)(&()),
-					WidgetValueAction::Update => (invisible.on_update.callback)(&()),
-				};
-
-				responses.add(callback_message);
-			}
 			Widget::NodeCatalog(node_type_input) => match action {
 				WidgetValueAction::Commit => {
 					let callback_message = (node_type_input.on_commit.callback)(&());
@@ -411,7 +405,34 @@ impl LayoutMessageHandler {
 			Widget::TextButton(text_button) => {
 				let callback_message = match action {
 					WidgetValueAction::Commit => (text_button.on_commit.callback)(&()),
-					WidgetValueAction::Update => (text_button.on_update.callback)(text_button),
+					WidgetValueAction::Update => {
+						let Some(value_path) = value.as_array() else {
+							error!("TextButton update was not of type: array");
+							return;
+						};
+
+						// Process the text button click, since no menu is involved if we're given an empty array.
+						if value_path.is_empty() {
+							(text_button.on_update.callback)(text_button)
+						}
+						// Process the text button's menu list entry click, since we have a path to the value of the contained menu entry.
+						else {
+							let mut current_submenu = &text_button.menu_list_children;
+							let mut final_entry: Option<&MenuListEntry> = None;
+
+							// Loop through all menu entry value strings in the path until we reach the final entry (which we store).
+							// Otherwise we exit early if we can't traverse the full path.
+							for value in value_path.iter().filter_map(|v| v.as_str().map(|s| s.to_string())) {
+								let Some(next_entry) = current_submenu.iter().flatten().find(|e| e.value == value) else { return };
+
+								current_submenu = &next_entry.children;
+								final_entry = Some(next_entry);
+							}
+
+							// If we've reached here without returning early, we have a final entry in the path and we should now execute its callback.
+							(final_entry.unwrap().on_commit.callback)(&())
+						}
+					}
 				};
 
 				responses.add(callback_message);
@@ -447,31 +468,26 @@ impl LayoutMessageHandler {
 		match new_layout {
 			Layout::WidgetLayout(_) => {
 				let mut widget_diffs = Vec::new();
-				self.layouts[layout_target as usize].diff(new_layout, &mut Vec::new(), &mut widget_diffs);
 
-				// Skip sending if no diff.
+				let Layout::WidgetLayout(current) = &mut self.layouts[layout_target as usize];
+				let Layout::WidgetLayout(new) = new_layout;
+				current.diff(new, &mut Vec::new(), &mut widget_diffs);
+
+				// Skip sending if no diff
 				if widget_diffs.is_empty() {
 					return;
 				}
 
-				self.send_diff(widget_diffs, layout_target, responses, action_input_mapping);
-			}
-			// We don't diff the menu bar layout yet.
-			Layout::MenuLayout(_) => {
-				// Skip update if the same
-				if self.layouts[layout_target as usize] == new_layout {
-					return;
+				// On Mac we need the full MenuBar layout to construct the native menu
+				#[cfg(target_os = "macos")]
+				if layout_target == LayoutTarget::MenuBar {
+					widget_diffs = vec![WidgetDiff {
+						widget_path: Vec::new(),
+						new_value: DiffUpdate::SubLayout(current.layout.clone()),
+					}];
 				}
 
-				// Update the backend storage
-				self.layouts[layout_target as usize] = new_layout;
-
-				// Update the UI
-				let Some(layout) = self.layouts[layout_target as usize].clone().as_menu_layout(action_input_mapping).map(|x| x.layout) else {
-					error!("Called unwrap_menu_layout on a widget layout");
-					return;
-				};
-				responses.add(FrontendMessage::UpdateMenuBarLayout { layout_target, layout });
+				self.send_diff(widget_diffs, layout_target, responses, action_input_mapping);
 			}
 		}
 	}
@@ -481,24 +497,25 @@ impl LayoutMessageHandler {
 		diff.iter_mut().for_each(|diff| diff.new_value.apply_keyboard_shortcut(action_input_mapping));
 
 		let message = match layout_target {
-			LayoutTarget::MenuBar => unreachable!("Menu bar is not diffed"),
+			LayoutTarget::DataPanel => FrontendMessage::UpdateDataPanelLayout { layout_target, diff },
 			LayoutTarget::DialogButtons => FrontendMessage::UpdateDialogButtons { layout_target, diff },
 			LayoutTarget::DialogColumn1 => FrontendMessage::UpdateDialogColumn1 { layout_target, diff },
 			LayoutTarget::DialogColumn2 => FrontendMessage::UpdateDialogColumn2 { layout_target, diff },
 			LayoutTarget::DocumentBar => FrontendMessage::UpdateDocumentBarLayout { layout_target, diff },
 			LayoutTarget::DocumentMode => FrontendMessage::UpdateDocumentModeLayout { layout_target, diff },
-			LayoutTarget::DataPanel => FrontendMessage::UpdateDataPanelLayout { layout_target, diff },
+			LayoutTarget::LayersPanelBottomBar => FrontendMessage::UpdateLayersPanelBottomBarLayout { layout_target, diff },
 			LayoutTarget::LayersPanelControlLeftBar => FrontendMessage::UpdateLayersPanelControlBarLeftLayout { layout_target, diff },
 			LayoutTarget::LayersPanelControlRightBar => FrontendMessage::UpdateLayersPanelControlBarRightLayout { layout_target, diff },
-			LayoutTarget::LayersPanelBottomBar => FrontendMessage::UpdateLayersPanelBottomBarLayout { layout_target, diff },
-			LayoutTarget::PropertiesPanel => FrontendMessage::UpdatePropertiesPanelLayout { layout_target, diff },
+			LayoutTarget::MenuBar => FrontendMessage::UpdateMenuBarLayout { layout_target, diff },
 			LayoutTarget::NodeGraphControlBar => FrontendMessage::UpdateNodeGraphControlBarLayout { layout_target, diff },
+			LayoutTarget::PropertiesPanel => FrontendMessage::UpdatePropertiesPanelLayout { layout_target, diff },
 			LayoutTarget::ToolOptions => FrontendMessage::UpdateToolOptionsLayout { layout_target, diff },
 			LayoutTarget::ToolShelf => FrontendMessage::UpdateToolShelfLayout { layout_target, diff },
 			LayoutTarget::WorkingColors => FrontendMessage::UpdateWorkingColorsLayout { layout_target, diff },
 
 			LayoutTarget::LayoutTargetLength => panic!("`LayoutTargetLength` is not a valid Layout Target and is used for array indexing"),
 		};
+
 		responses.add(message);
 	}
 }

editor/src/messages/layout/utility_types/layout_widget.rs

@@ -1,7 +1,6 @@
 use super::widgets::button_widgets::*;
 use super::widgets::input_widgets::*;
 use super::widgets::label_widgets::*;
-use super::widgets::menu_widgets::MenuLayout;
 use crate::application::generate_uuid;
 use crate::messages::input_mapper::utility_types::input_keyboard::KeysGroup;
 use crate::messages::input_mapper::utility_types::misc::ActionKeys;
@@ -101,56 +100,11 @@ pub trait DialogLayoutHolder: LayoutHolder {
 	}
 }
 
+// TODO: Unwrap this enum
 /// Wraps a choice of layout type. The chosen layout contains an arrangement of widgets mounted somewhere specific in the frontend.
 #[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize, specta::Type)]
 pub enum Layout {
 	WidgetLayout(WidgetLayout),
-	MenuLayout(MenuLayout),
-}
-
-impl Layout {
-	pub fn as_menu_layout(self, action_input_mapping: &impl Fn(&MessageDiscriminant) -> Option<KeysGroup>) -> Option<MenuLayout> {
-		if let Self::MenuLayout(mut menu) = self {
-			menu.layout
-				.iter_mut()
-				.for_each(|menu_column| menu_column.children.fill_in_shortcut_actions_with_keys(action_input_mapping));
-			Some(menu)
-		} else {
-			None
-		}
-	}
-
-	pub fn iter(&self) -> Box<dyn Iterator<Item = &WidgetHolder> + '_> {
-		match self {
-			Layout::MenuLayout(menu_layout) => Box::new(menu_layout.iter()),
-			Layout::WidgetLayout(widget_layout) => Box::new(widget_layout.iter()),
-		}
-	}
-
-	pub fn iter_mut(&mut self) -> Box<dyn Iterator<Item = &mut WidgetHolder> + '_> {
-		match self {
-			Layout::MenuLayout(menu_layout) => Box::new(menu_layout.iter_mut()),
-			Layout::WidgetLayout(widget_layout) => Box::new(widget_layout.iter_mut()),
-		}
-	}
-
-	/// 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<usize>, widget_diffs: &mut Vec<WidgetDiff>) {
-		match (self, new) {
-			// Simply diff the internal layout
-			(Self::WidgetLayout(current), Self::WidgetLayout(new)) => current.diff(new, widget_path, widget_diffs),
-			(current, Self::WidgetLayout(widget_layout)) => {
-				// Update current to the new value
-				*current = Self::WidgetLayout(widget_layout.clone());
-
-				// Push an update sublayout value
-				let new_value = DiffUpdate::SubLayout(widget_layout.layout);
-				let widget_path = widget_path.to_vec();
-				widget_diffs.push(WidgetDiff { widget_path, new_value });
-			}
-			(_, Self::MenuLayout(_)) => panic!("Cannot diff menu layout"),
-		}
-	}
 }
 
 impl Default for Layout {
@@ -159,6 +113,7 @@ impl Default for Layout {
 	}
 }
 
+// TODO: Unwrap this struct
 #[derive(Debug, Default, Clone, serde::Serialize, serde::Deserialize, PartialEq, specta::Type)]
 pub struct WidgetLayout {
 	pub layout: SubLayout,
@@ -327,7 +282,6 @@ pub enum LayoutGroup {
 		#[serde(rename = "tableWidgets")]
 		rows: Vec<Vec<WidgetHolder>>,
 	},
-	// TODO: Move this from being a child of `enum LayoutGroup` to being a child of `enum Layout`
 	#[serde(rename = "section")]
 	Section {
 		name: String,
@@ -378,7 +332,7 @@ impl LayoutGroup {
 				Widget::TextInput(x) => &mut x.tooltip_label,
 				Widget::TextLabel(x) => &mut x.tooltip_label,
 				Widget::BreadcrumbTrailButtons(x) => &mut x.tooltip_label,
-				Widget::InvisibleStandinInput(_) | Widget::ReferencePointInput(_) | Widget::RadioInput(_) | Widget::Separator(_) | Widget::WorkingColorsInput(_) | Widget::NodeCatalog(_) => continue,
+				Widget::ReferencePointInput(_) | Widget::RadioInput(_) | Widget::Separator(_) | Widget::WorkingColorsInput(_) | Widget::NodeCatalog(_) => continue,
 			};
 			if val.is_empty() {
 				val.clone_from(&label);
@@ -414,7 +368,7 @@ impl LayoutGroup {
 				Widget::TextInput(x) => &mut x.tooltip_description,
 				Widget::TextLabel(x) => &mut x.tooltip_description,
 				Widget::BreadcrumbTrailButtons(x) => &mut x.tooltip_description,
-				Widget::InvisibleStandinInput(_) | Widget::ReferencePointInput(_) | Widget::RadioInput(_) | Widget::Separator(_) | Widget::WorkingColorsInput(_) | Widget::NodeCatalog(_) => continue,
+				Widget::ReferencePointInput(_) | Widget::RadioInput(_) | Widget::Separator(_) | Widget::WorkingColorsInput(_) | Widget::NodeCatalog(_) => continue,
 			};
 			if val.is_empty() {
 				val.clone_from(&description);
@@ -520,6 +474,7 @@ impl LayoutGroup {
 	}
 }
 
+// TODO: Rename to WidgetInstance
 #[derive(Debug, Clone, serde::Serialize, serde::Deserialize, specta::Type)]
 pub struct WidgetHolder {
 	#[serde(rename = "widgetId")]
@@ -609,7 +564,6 @@ pub enum Widget {
 	IconLabel(IconLabel),
 	ImageButton(ImageButton),
 	ImageLabel(ImageLabel),
-	InvisibleStandinInput(InvisibleStandinInput),
 	NodeCatalog(NodeCatalog),
 	NumberInput(NumberInput),
 	ParameterExposeButton(ParameterExposeButton),
@@ -680,7 +634,6 @@ impl DiffUpdate {
 				Widget::IconLabel(_)
 				| Widget::ImageLabel(_)
 				| Widget::CurveInput(_)
-				| Widget::InvisibleStandinInput(_)
 				| Widget::NodeCatalog(_)
 				| Widget::ReferencePointInput(_)
 				| Widget::RadioInput(_)
@@ -709,10 +662,45 @@ impl DiffUpdate {
 			}
 		};
 
+		// Recursively fill menu list entries with their realized shortcut keys specific to the current bindings and platform
+		let apply_action_keys_to_menu_lists = |entry_sections: &mut MenuListEntrySections| {
+			struct RecursiveWrapper<'a>(&'a dyn Fn(&mut MenuListEntrySections, &RecursiveWrapper));
+			let recursive_wrapper = RecursiveWrapper(&|entry_sections: &mut MenuListEntrySections, recursive_wrapper| {
+				for entries in entry_sections {
+					for entry in entries {
+						// Convert the shortcut actions to keys for this menu entry
+						if let Some(shortcut_keys) = &mut entry.shortcut_keys {
+							shortcut_keys.to_keys(action_input_mapping);
+						}
+
+						// Recursively call this inner closure on the menu's children
+						(recursive_wrapper.0)(&mut entry.children, recursive_wrapper);
+					}
+				}
+			});
+			(recursive_wrapper.0)(entry_sections, &recursive_wrapper)
+		};
+
+		// Apply shortcut conversions to all widgets that have menu lists
+		let convert_menu_lists = |widget_holder: &mut WidgetHolder| match &mut widget_holder.widget {
+			Widget::DropdownInput(dropdown_input) => apply_action_keys_to_menu_lists(&mut dropdown_input.entries),
+			Widget::TextButton(text_button) => apply_action_keys_to_menu_lists(&mut text_button.menu_list_children),
+			_ => {}
+		};
+
 		match self {
-			Self::SubLayout(sub_layout) => sub_layout.iter_mut().flat_map(|layout_group| layout_group.iter_mut()).for_each(convert_tooltip),
-			Self::LayoutGroup(layout_group) => layout_group.iter_mut().for_each(convert_tooltip),
-			Self::Widget(widget_holder) => convert_tooltip(widget_holder),
+			Self::SubLayout(sub_layout) => sub_layout.iter_mut().flat_map(|layout_group| layout_group.iter_mut()).for_each(|widget_holder| {
+				convert_tooltip(widget_holder);
+				convert_menu_lists(widget_holder);
+			}),
+			Self::LayoutGroup(layout_group) => layout_group.iter_mut().for_each(|widget_holder| {
+				convert_tooltip(widget_holder);
+				convert_menu_lists(widget_holder);
+			}),
+			Self::Widget(widget_holder) => {
+				convert_tooltip(widget_holder);
+				convert_menu_lists(widget_holder);
+			}
 		}
 	}
 }

editor/src/messages/layout/utility_types/mod.rs

@@ -6,5 +6,4 @@ pub mod widget_prelude {
 	pub use super::widgets::button_widgets::*;
 	pub use super::widgets::input_widgets::*;
 	pub use super::widgets::label_widgets::*;
-	pub use super::widgets::menu_widgets::*;
 }

editor/src/messages/layout/utility_types/widgets/input_widgets.rs

@@ -133,15 +133,28 @@ pub struct MenuListEntry {
 
 	pub label: String,
 
+	pub font: String,
+
 	pub icon: String,
 
-	pub shortcut: Vec<String>,
+	pub disabled: bool,
+
+	#[serde(rename = "tooltipLabel")]
+	pub tooltip_label: String,
+
+	#[serde(rename = "tooltipDescription")]
+	pub tooltip_description: String,
+
+	#[serde(rename = "tooltipShortcut")]
+	pub tooltip_shortcut: String,
+
+	// TODO: Make this serde(skip)
+	#[serde(rename = "shortcutKeys")]
+	pub shortcut_keys: Option<ActionKeys>,
 
 	#[serde(rename = "shortcutRequiresLock")]
 	pub shortcut_requires_lock: bool,
 
-	pub disabled: bool,
-
 	pub children: MenuListEntrySections,
 
 	// Callbacks
@@ -192,21 +205,6 @@ pub struct FontInput {
 	pub on_commit: WidgetCallback<()>,
 }
 
-/// This widget allows for the flexible use of the layout system.
-/// In a custom layout, one can define a widget that is just used to trigger code on the backend.
-/// This is used in MenuLayout to pipe the triggering of messages from the frontend to backend.
-#[derive(Clone, serde::Serialize, serde::Deserialize, Derivative, Default, WidgetBuilder, specta::Type)]
-#[derivative(Debug, PartialEq)]
-pub struct InvisibleStandinInput {
-	#[serde(skip)]
-	#[derivative(Debug = "ignore", PartialEq = "ignore")]
-	pub on_update: WidgetCallback<()>,
-
-	#[serde(skip)]
-	#[derivative(Debug = "ignore", PartialEq = "ignore")]
-	pub on_commit: WidgetCallback<()>,
-}
-
 #[derive(Clone, serde::Serialize, serde::Deserialize, Derivative, WidgetBuilder, specta::Type)]
 #[derivative(Debug, PartialEq, Default)]
 pub struct NumberInput {

editor/src/messages/layout/utility_types/widgets/menu_widgets.rs

@@ -1,131 +0,0 @@
-use super::input_widgets::InvisibleStandinInput;
-use crate::messages::input_mapper::utility_types::input_keyboard::KeysGroup;
-use crate::messages::input_mapper::utility_types::misc::ActionKeys;
-use crate::messages::layout::utility_types::widget_prelude::*;
-use crate::messages::prelude::*;
-
-#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, PartialEq, Default, specta::Type)]
-pub struct MenuBarEntryChildren(pub Vec<Vec<MenuBarEntry>>);
-
-impl MenuBarEntryChildren {
-	pub fn empty() -> Self {
-		Self(Vec::new())
-	}
-
-	pub fn fill_in_shortcut_actions_with_keys(&mut self, action_input_mapping: &impl Fn(&MessageDiscriminant) -> Option<KeysGroup>) {
-		let entries = self.0.iter_mut().flatten();
-
-		for entry in entries {
-			if let Some(action_keys) = &mut entry.shortcut {
-				action_keys.to_keys(action_input_mapping);
-			}
-
-			// Recursively do this for the children also
-			entry.children.fill_in_shortcut_actions_with_keys(action_input_mapping);
-		}
-	}
-}
-
-#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, PartialEq, specta::Type)]
-pub struct MenuBarEntry {
-	pub label: String,
-	pub icon: Option<String>,
-	pub shortcut: Option<ActionKeys>,
-	pub action: WidgetHolder,
-	pub children: MenuBarEntryChildren,
-	pub disabled: bool,
-}
-
-impl MenuBarEntry {
-	pub fn new_root(label: String, disabled: bool, children: MenuBarEntryChildren) -> Self {
-		Self {
-			label,
-			disabled,
-			children,
-			..Default::default()
-		}
-	}
-
-	pub fn create_action(callback: impl Fn(&()) -> Message + 'static + Send + Sync) -> WidgetHolder {
-		InvisibleStandinInput::new().on_update(callback).widget_holder()
-	}
-
-	pub fn no_action() -> WidgetHolder {
-		MenuBarEntry::create_action(|_| Message::NoOp)
-	}
-}
-
-impl Default for MenuBarEntry {
-	fn default() -> Self {
-		Self {
-			label: "".into(),
-			icon: None,
-			shortcut: None,
-			action: MenuBarEntry::no_action(),
-			children: MenuBarEntryChildren::empty(),
-			disabled: false,
-		}
-	}
-}
-
-#[derive(Debug, Default, Clone, PartialEq, serde::Serialize, serde::Deserialize, specta::Type)]
-pub struct MenuLayout {
-	pub layout: Vec<MenuBarEntry>,
-}
-
-impl MenuLayout {
-	pub fn new(layout: Vec<MenuBarEntry>) -> Self {
-		Self { layout }
-	}
-
-	pub fn iter(&self) -> impl Iterator<Item = &WidgetHolder> + '_ {
-		MenuLayoutIter { stack: self.layout.iter().collect() }
-	}
-
-	pub fn iter_mut(&mut self) -> impl Iterator<Item = &mut WidgetHolder> + '_ {
-		MenuLayoutIterMut {
-			stack: self.layout.iter_mut().collect(),
-		}
-	}
-}
-
-#[derive(Debug, Default)]
-pub struct MenuLayoutIter<'a> {
-	pub stack: Vec<&'a MenuBarEntry>,
-}
-
-impl<'a> Iterator for MenuLayoutIter<'a> {
-	type Item = &'a WidgetHolder;
-
-	fn next(&mut self) -> Option<Self::Item> {
-		match self.stack.pop() {
-			Some(menu_entry) => {
-				let more_entries = menu_entry.children.0.iter().flat_map(|entry| entry.iter());
-				self.stack.extend(more_entries);
-
-				Some(&menu_entry.action)
-			}
-			None => None,
-		}
-	}
-}
-
-pub struct MenuLayoutIterMut<'a> {
-	pub stack: Vec<&'a mut MenuBarEntry>,
-}
-
-impl<'a> Iterator for MenuLayoutIterMut<'a> {
-	type Item = &'a mut WidgetHolder;
-
-	fn next(&mut self) -> Option<Self::Item> {
-		match self.stack.pop() {
-			Some(menu_entry) => {
-				let more_entries = menu_entry.children.0.iter_mut().flat_map(|entry| entry.iter_mut());
-				self.stack.extend(more_entries);
-
-				Some(&mut menu_entry.action)
-			}
-			None => None,
-		}
-	}
-}

editor/src/messages/layout/utility_types/widgets/mod.rs

@@ -1,4 +1,3 @@
 pub mod button_widgets;
 pub mod input_widgets;
 pub mod label_widgets;
-pub mod menu_widgets;