Improve hotkeys with canonical flag, more industry-standard mappings, and fix GRS menu bar labels (#2827)

Author: Keavon

editor/src/dispatcher.rs

@@ -319,12 +319,11 @@ impl Dispatcher {
 		}))
 	}
 
-	/// Logs a message that is about to be executed,
-	/// either as a tree with a discriminant or the entire payload (depending on settings)
+	/// Logs a message that is about to be executed, either as a tree
+	/// with a discriminant or the entire payload (depending on settings)
 	fn log_message(&self, message: &Message, queues: &[VecDeque<Message>], message_logging_verbosity: MessageLoggingVerbosity) {
 		let discriminant = MessageDiscriminant::from(message);
-		let is_blocked = DEBUG_MESSAGE_BLOCK_LIST.iter().any(|&blocked_discriminant| discriminant == blocked_discriminant)
-			|| DEBUG_MESSAGE_ENDING_BLOCK_LIST.iter().any(|blocked_name| discriminant.local_name().ends_with(blocked_name));
+		let is_blocked = DEBUG_MESSAGE_BLOCK_LIST.contains(&discriminant) || DEBUG_MESSAGE_ENDING_BLOCK_LIST.iter().any(|blocked_name| discriminant.local_name().ends_with(blocked_name));
 
 		if !is_blocked {
 			match message_logging_verbosity {

editor/src/messages/input_mapper/input_mapper_message_handler.rs

@@ -1,6 +1,7 @@
 use super::utility_types::input_keyboard::KeysGroup;
 use super::utility_types::misc::Mapping;
 use crate::messages::input_mapper::utility_types::input_keyboard::{self, Key};
+use crate::messages::input_mapper::utility_types::misc::MappingEntry;
 use crate::messages::portfolio::utility_types::KeyboardPlatformLayout;
 use crate::messages::prelude::*;
 use std::fmt::Write;
@@ -47,12 +48,12 @@ impl InputMapperMessageHandler {
 				ma.map(|a| ((i as u8).try_into().unwrap(), a))
 			})
 			.for_each(|(k, a): (Key, _)| {
-				let _ = write!(output, "{}: {}, ", k.to_discriminant().local_name(), a.local_name().split('.').last().unwrap());
+				let _ = write!(output, "{}: {}, ", k.to_discriminant().local_name(), a.local_name().split('.').next_back().unwrap());
 			});
 		output.replace("Key", "")
 	}
 
-	pub fn action_input_mapping(&self, action_to_find: &MessageDiscriminant) -> Vec<KeysGroup> {
+	pub fn action_input_mapping(&self, action_to_find: &MessageDiscriminant) -> Option<KeysGroup> {
 		let all_key_mapping_entries = std::iter::empty()
 			.chain(self.mapping.key_up.iter())
 			.chain(self.mapping.key_down.iter())
@@ -66,55 +67,65 @@ impl InputMapperMessageHandler {
 		// Filter for the desired message
 		let found_actions = all_mapping_entries.filter(|entry| entry.action.to_discriminant() == *action_to_find);
 
+		// Get the `Key` for this platform's accelerator key
 		let keyboard_layout = || GLOBAL_PLATFORM.get().copied().unwrap_or_default().as_keyboard_platform_layout();
 		let platform_accel_key = match keyboard_layout() {
 			KeyboardPlatformLayout::Standard => Key::Control,
 			KeyboardPlatformLayout::Mac => Key::Command,
 		};
 
+		let entry_to_key = |entry: &MappingEntry| {
+			// Get the modifier keys for the entry (and convert them to Key)
+			let mut keys = entry
+				.modifiers
+				.iter()
+				.map(|i| {
+					// TODO: Use a safe solution eventually
+					assert!(
+						i < input_keyboard::NUMBER_OF_KEYS,
+						"Attempting to convert a Key with enum index {i}, which is larger than the number of Key enums",
+					);
+					(i as u8).try_into().unwrap()
+				})
+				.collect::<Vec<_>>();
+
+			// Append the key button for the entry
+			use InputMapperMessage as IMM;
+			match entry.input {
+				IMM::KeyDown(key) | IMM::KeyUp(key) | IMM::KeyDownNoRepeat(key) | IMM::KeyUpNoRepeat(key) => keys.push(key),
+				_ => (),
+			}
+
+			keys.sort_by(|&a, &b| {
+				// Order according to platform guidelines mentioned at https://ux.stackexchange.com/questions/58185/normative-ordering-for-modifier-key-combinations
+				const ORDER: [Key; 4] = [Key::Control, Key::Alt, Key::Shift, Key::Command];
+
+				// Treat the `Accel` virtual key as the platform's accel key for sorting comparison purposes
+				let a = if a == Key::Accel { platform_accel_key } else { a };
+				let b = if b == Key::Accel { platform_accel_key } else { b };
+
+				// Find where the keys are in the order, or put them at the end if they're not found
+				let a = ORDER.iter().position(|&key| key == a).unwrap_or(ORDER.len());
+				let b = ORDER.iter().position(|&key| key == b).unwrap_or(ORDER.len());
+
+				// Compare the positions of both keys
+				a.cmp(&b)
+			});
+
+			KeysGroup(keys)
+		};
+
+		// If a canonical key combination is found, return it
+		if let Some(canonical) = found_actions.clone().find(|entry| entry.canonical).map(entry_to_key) {
+			return Some(canonical);
+		}
+
 		// Find the key combinations for all keymaps matching the desired action
 		assert!(std::mem::size_of::<usize>() >= std::mem::size_of::<Key>());
-		found_actions
-			.map(|entry| {
-				// Get the modifier keys for the entry (and convert them to Key)
-				let mut keys = entry
-					.modifiers
-					.iter()
-					.map(|i| {
-						// TODO: Use a safe solution eventually
-						assert!(
-							i < input_keyboard::NUMBER_OF_KEYS,
-							"Attempting to convert a Key with enum index {i}, which is larger than the number of Key enums",
-						);
-						(i as u8).try_into().unwrap()
-					})
-					.collect::<Vec<_>>();
-
-				// Append the key button for the entry
-				use InputMapperMessage as IMM;
-				match entry.input {
-					IMM::KeyDown(key) | IMM::KeyUp(key) | IMM::KeyDownNoRepeat(key) | IMM::KeyUpNoRepeat(key) => keys.push(key),
-					_ => (),
-				}
-
-				keys.sort_by(|&a, &b| {
-					// Order according to platform guidelines mentioned at https://ux.stackexchange.com/questions/58185/normative-ordering-for-modifier-key-combinations
-					const ORDER: [Key; 4] = [Key::Control, Key::Alt, Key::Shift, Key::Command];
-
-					// Treat the `Accel` virtual key as the platform's accel key for sorting comparison purposes
-					let a = if a == Key::Accel { platform_accel_key } else { a };
-					let b = if b == Key::Accel { platform_accel_key } else { b };
-
-					// Find where the keys are in the order, or put them at the end if they're not found
-					let a = ORDER.iter().position(|&key| key == a).unwrap_or(ORDER.len());
-					let b = ORDER.iter().position(|&key| key == b).unwrap_or(ORDER.len());
-
-					// Compare the positions of both keys
-					a.cmp(&b)
-				});
-
-				KeysGroup(keys)
-			})
-			.collect::<Vec<_>>()
+		let mut key_sequences = found_actions.map(entry_to_key).collect::<Vec<_>>();
+
+		// Return the shortest key sequence, if any
+		key_sequences.sort_by_key(|keys| keys.0.len());
+		key_sequences.first().cloned()
 	}
 }

editor/src/messages/input_mapper/input_mappings.rs

@@ -8,7 +8,6 @@ use crate::messages::input_mapper::utility_types::misc::{KeyMappingEntries, Mapp
 use crate::messages::portfolio::document::node_graph::utility_types::Direction;
 use crate::messages::portfolio::document::utility_types::clipboards::Clipboard;
 use crate::messages::portfolio::document::utility_types::misc::GroupFolderType;
-use crate::messages::portfolio::document::utility_types::transformation::TransformType;
 use crate::messages::prelude::*;
 use crate::messages::tool::tool_messages::brush_tool::BrushToolMessageOptionsUpdate;
 use crate::messages::tool::tool_messages::select_tool::SelectToolPointerKeys;
@@ -221,7 +220,8 @@ pub fn input_mappings() -> Mapping {
 		entry!(PointerMove; refresh_keys=[KeyC, Space, Control, Shift, Alt], action_dispatch=PathToolMessage::PointerMove { toggle_colinear: KeyC, equidistant: Alt, move_anchor_with_handles: Space, snap_angle: Shift, lock_angle: Control, delete_segment: Alt, break_colinear_molding: Alt }),
 		entry!(KeyDown(Delete); action_dispatch=PathToolMessage::Delete),
 		entry!(KeyDown(KeyA); modifiers=[Accel], action_dispatch=PathToolMessage::SelectAllAnchors),
-		entry!(KeyDown(KeyA); modifiers=[Accel, Shift], action_dispatch=PathToolMessage::DeselectAllPoints),
+		entry!(KeyDown(KeyA); modifiers=[Accel, Shift], canonical, action_dispatch=PathToolMessage::DeselectAllPoints),
+		entry!(KeyDown(KeyA); modifiers=[Alt], action_dispatch=PathToolMessage::DeselectAllPoints),
 		entry!(KeyDown(Backspace); action_dispatch=PathToolMessage::Delete),
 		entry!(KeyUp(MouseLeft); action_dispatch=PathToolMessage::DragStop { extend_selection: Shift, shrink_selection: Alt }),
 		entry!(KeyDown(Enter); action_dispatch=PathToolMessage::Enter { extend_selection: Shift, shrink_selection: Alt }),
@@ -313,9 +313,10 @@ pub fn input_mappings() -> Mapping {
 		entry!(KeyDown(KeyE); action_dispatch=ToolMessage::ActivateToolShapeEllipse),
 		entry!(KeyDown(KeyY); action_dispatch=ToolMessage::ActivateToolShape),
 		entry!(KeyDown(KeyB); action_dispatch=ToolMessage::ActivateToolBrush),
-		entry!(KeyDown(KeyX); modifiers=[Accel, Shift], action_dispatch=ToolMessage::ResetColors),
+		entry!(KeyDown(KeyD); action_dispatch=ToolMessage::ResetColors),
 		entry!(KeyDown(KeyX); modifiers=[Shift], action_dispatch=ToolMessage::SwapColors),
-		entry!(KeyDown(KeyC); modifiers=[Alt], action_dispatch=ToolMessage::SelectRandomPrimaryColor),
+		entry!(KeyDown(KeyC); modifiers=[Alt], action_dispatch=ToolMessage::SelectRandomWorkingColor { primary: true }),
+		entry!(KeyDown(KeyC); modifiers=[Alt, Shift], action_dispatch=ToolMessage::SelectRandomWorkingColor { primary: false }),
 		//
 		// DocumentMessage
 		entry!(KeyDown(Space); modifiers=[Control], action_dispatch=DocumentMessage::GraphViewOverlayToggle),
@@ -327,20 +328,21 @@ pub fn input_mappings() -> Mapping {
 		entry!(KeyDown(KeyH); modifiers=[Accel], action_dispatch=DocumentMessage::ToggleSelectedVisibility),
 		entry!(KeyDown(KeyL); modifiers=[Accel], action_dispatch=DocumentMessage::ToggleSelectedLocked),
 		entry!(KeyDown(KeyG); modifiers=[Alt], action_dispatch=DocumentMessage::ToggleGridVisibility),
-		entry!(KeyDown(KeyZ); modifiers=[Accel, Shift], action_dispatch=DocumentMessage::Redo),
+		entry!(KeyDown(KeyZ); modifiers=[Accel, Shift], canonical, action_dispatch=DocumentMessage::Redo),
 		entry!(KeyDown(KeyY); modifiers=[Accel], action_dispatch=DocumentMessage::Redo),
 		entry!(KeyDown(KeyZ); modifiers=[Accel], action_dispatch=DocumentMessage::Undo),
 		entry!(KeyDown(KeyA); modifiers=[Accel], action_dispatch=DocumentMessage::SelectAllLayers),
-		entry!(KeyDown(KeyA); modifiers=[Accel, Shift], action_dispatch=DocumentMessage::DeselectAllLayers),
+		entry!(KeyDown(KeyA); modifiers=[Accel, Shift], canonical, action_dispatch=DocumentMessage::DeselectAllLayers),
+		entry!(KeyDown(KeyA); modifiers=[Alt], action_dispatch=DocumentMessage::DeselectAllLayers),
 		entry!(KeyDown(KeyS); modifiers=[Accel], action_dispatch=DocumentMessage::SaveDocument),
-		entry!(KeyDown(KeyD); modifiers=[Accel], action_dispatch=DocumentMessage::DuplicateSelectedLayers),
+		entry!(KeyDown(KeyD); modifiers=[Accel], canonical, action_dispatch=DocumentMessage::DuplicateSelectedLayers),
 		entry!(KeyDown(KeyJ); modifiers=[Accel], action_dispatch=DocumentMessage::DuplicateSelectedLayers),
 		entry!(KeyDown(KeyG); modifiers=[Accel], action_dispatch=DocumentMessage::GroupSelectedLayers { group_folder_type: GroupFolderType::Layer }),
 		entry!(KeyDown(KeyG); modifiers=[Accel, Shift], action_dispatch=DocumentMessage::UngroupSelectedLayers),
 		entry!(KeyDown(KeyN); modifiers=[Accel, Shift], action_dispatch=DocumentMessage::CreateEmptyFolder),
-		entry!(KeyDown(Backslash); modifiers=[Alt], action_dispatch=DocumentMessage::SelectParentLayer),
-		entry!(KeyDown(BracketLeft); modifiers=[Alt], action_dispatch=DocumentMessage::SelectionStepBack),
-		entry!(KeyDown(BracketRight); modifiers=[Alt], action_dispatch=DocumentMessage::SelectionStepForward),
+		entry!(KeyDown(Escape); modifiers=[Shift], action_dispatch=DocumentMessage::SelectParentLayer),
+		entry!(KeyDown(BracketLeft); modifiers=[Alt], canonical, action_dispatch=DocumentMessage::SelectionStepBack),
+		entry!(KeyDown(BracketRight); modifiers=[Alt], canonical, action_dispatch=DocumentMessage::SelectionStepForward),
 		entry!(KeyDown(MouseBack); action_dispatch=DocumentMessage::SelectionStepBack),
 		entry!(KeyDown(MouseForward); action_dispatch=DocumentMessage::SelectionStepForward),
 		entry!(KeyDown(Digit0); modifiers=[Accel], action_dispatch=DocumentMessage::ZoomCanvasToFitAll),
@@ -376,9 +378,9 @@ pub fn input_mappings() -> Mapping {
 		entry!(KeyDown(ArrowRight); action_dispatch=DocumentMessage::NudgeSelectedLayers { delta_x: NUDGE_AMOUNT, delta_y: 0., resize: Alt, resize_opposite_corner: Control }),
 		//
 		// TransformLayerMessage
-		entry!(KeyDown(KeyG); action_dispatch=TransformLayerMessage::BeginGRS { transform_type: TransformType::Grab }),
-		entry!(KeyDown(KeyR); action_dispatch=TransformLayerMessage::BeginGRS { transform_type: TransformType::Rotate }),
-		entry!(KeyDown(KeyS); action_dispatch=TransformLayerMessage::BeginGRS { transform_type: TransformType::Scale }),
+		entry!(KeyDown(KeyG); action_dispatch=TransformLayerMessage::BeginGrab),
+		entry!(KeyDown(KeyR); action_dispatch=TransformLayerMessage::BeginRotate),
+		entry!(KeyDown(KeyS); action_dispatch=TransformLayerMessage::BeginScale),
 		entry!(KeyDown(Digit0); action_dispatch=TransformLayerMessage::TypeDigit { digit: 0 }),
 		entry!(KeyDown(Digit1); action_dispatch=TransformLayerMessage::TypeDigit { digit: 1 }),
 		entry!(KeyDown(Digit2); action_dispatch=TransformLayerMessage::TypeDigit { digit: 2 }),

editor/src/messages/input_mapper/key_mapping/key_mapping_message_handler.rs

@@ -25,7 +25,7 @@ impl MessageHandler<KeyMappingMessage, KeyMappingMessageData<'_>> for KeyMapping
 }
 
 impl KeyMappingMessageHandler {
-	pub fn action_input_mapping(&self, action_to_find: &MessageDiscriminant) -> Vec<KeysGroup> {
+	pub fn action_input_mapping(&self, action_to_find: &MessageDiscriminant) -> Option<KeysGroup> {
 		self.mapping_handler.action_input_mapping(action_to_find)
 	}
 }

editor/src/messages/input_mapper/utility_types/macros.rs

@@ -24,44 +24,54 @@ macro_rules! modifiers {
 /// Each handler adds or removes actions in the form of message discriminants. Here, we tie an input condition (such as a hotkey) to an action's full message.
 /// When an action is currently available, and the user enters that input, the action's message is dispatched on the message bus.
 macro_rules! entry {
+	// Pattern with canonical parameter
+	($input:expr_2021; $(modifiers=[$($modifier:ident),*],)? $(refresh_keys=[$($refresh:ident),* $(,)?],)? canonical, action_dispatch=$action_dispatch:expr_2021$(,)?) => {
+		entry!($input; $($($modifier),*)?; $($($refresh),*)?; $action_dispatch; true)
+	};
+
+	// Pattern without canonical parameter
 	($input:expr_2021; $(modifiers=[$($modifier:ident),*],)? $(refresh_keys=[$($refresh:ident),* $(,)?],)? action_dispatch=$action_dispatch:expr_2021$(,)?) => {
+		entry!($input; $($($modifier),*)?; $($($refresh),*)?; $action_dispatch; false)
+	};
+
+	// Implementation macro to avoid code duplication
+	($input:expr; $($modifier:ident),*; $($refresh:ident),*; $action_dispatch:expr; $canonical:expr) => {
 		&[&[
 			// Cause the `action_dispatch` message to be sent when the specified input occurs.
 			MappingEntry {
 				action: $action_dispatch.into(),
 				input: $input,
-				modifiers: modifiers!($($($modifier),*)?),
+				modifiers: modifiers!($($modifier),*),
+				canonical: $canonical,
 			},
 
 			// Also cause the `action_dispatch` message to be sent when any of the specified refresh keys change.
-			//
-			// For example, a snapping state bound to the Shift key may change if the user presses or releases that key.
-			// In that case, we want to dispatch the action's message even though the pointer didn't necessarily move so
-			// the input handler can update the snapping state without making the user move the mouse to see the change.
-			$(
 			$(
 			MappingEntry {
 				action: $action_dispatch.into(),
 				input: InputMapperMessage::KeyDown(Key::$refresh),
 				modifiers: modifiers!(),
+				canonical: $canonical,
 			},
 			MappingEntry {
 				action: $action_dispatch.into(),
 				input: InputMapperMessage::KeyUp(Key::$refresh),
 				modifiers: modifiers!(),
+				canonical: $canonical,
 			},
 			MappingEntry {
 				action: $action_dispatch.into(),
 				input: InputMapperMessage::KeyDownNoRepeat(Key::$refresh),
 				modifiers: modifiers!(),
+				canonical: $canonical,
 			},
 			MappingEntry {
 				action: $action_dispatch.into(),
 				input: InputMapperMessage::KeyUpNoRepeat(Key::$refresh),
 				modifiers: modifiers!(),
+				canonical: $canonical,
 			},
 			)*
-			)*
 		]]
 	};
 }

editor/src/messages/input_mapper/utility_types/misc.rs

@@ -1,4 +1,4 @@
-use super::input_keyboard::{Key, KeysGroup, LayoutKeysGroup, all_required_modifiers_pressed};
+use super::input_keyboard::{KeysGroup, LayoutKeysGroup, all_required_modifiers_pressed};
 use crate::messages::input_mapper::key_mapping::MappingVariant;
 use crate::messages::input_mapper::utility_types::input_keyboard::{KeyStates, NUMBER_OF_KEYS};
 use crate::messages::input_mapper::utility_types::input_mouse::NUMBER_OF_MOUSE_BUTTONS;
@@ -120,6 +120,8 @@ pub struct MappingEntry {
 	pub input: InputMapperMessage,
 	/// Any additional keys that must be also pressed for this input mapping to match
 	pub modifiers: KeyStates,
+	/// True indicates that this takes priority as the labeled hotkey shown in UI menus and tooltips instead of an alternate binding for the same action
+	pub canonical: bool,
 }
 
 #[derive(Debug, Clone, Eq, PartialEq, serde::Serialize, serde::Deserialize, specta::Type)]
@@ -130,36 +132,12 @@ pub enum ActionKeys {
 }
 
 impl ActionKeys {
-	pub fn to_keys(&mut self, action_input_mapping: &impl Fn(&MessageDiscriminant) -> Vec<KeysGroup>) -> String {
+	pub fn to_keys(&mut self, action_input_mapping: &impl Fn(&MessageDiscriminant) -> Option<KeysGroup>) -> String {
 		match self {
 			Self::Action(action) => {
-				// Take the shortest sequence of keys
-				let mut key_sequences = action_input_mapping(action);
-				key_sequences.sort_by_key(|keys| keys.0.len());
-				let mut secondary_key_sequence = key_sequences.get(1).cloned();
-				let mut key_sequence = key_sequences.get_mut(0);
-
-				// TODO: Replace this exception with a per-action choice of canonical hotkey
-				if let Some(key_sequence) = &mut key_sequence {
-					if key_sequence.0.as_slice() == [Key::MouseBack] {
-						if let Some(replacement) = &mut secondary_key_sequence {
-							std::mem::swap(*key_sequence, replacement);
-						}
-					}
-				}
-				if let Some(key_sequence) = &mut key_sequence {
-					if key_sequence.0.as_slice() == [Key::MouseForward] {
-						if let Some(replacement) = &mut secondary_key_sequence {
-							std::mem::swap(*key_sequence, replacement);
-						}
-					}
-				}
-
-				if let Some(keys) = key_sequence {
-					let mut taken_keys = KeysGroup::default();
-					std::mem::swap(keys, &mut taken_keys);
-					let description = taken_keys.to_string();
-					*self = Self::Keys(taken_keys.into());
+				if let Some(keys) = action_input_mapping(action) {
+					let description = keys.to_string();
+					*self = Self::Keys(keys.into());
 					description
 				} else {
 					*self = Self::Keys(KeysGroup::default().into());

editor/src/messages/layout/layout_message_handler.rs

@@ -342,7 +342,7 @@ impl LayoutMessageHandler {
 	}
 }
 
-impl<F: Fn(&MessageDiscriminant) -> Vec<KeysGroup>> MessageHandler<LayoutMessage, F> for LayoutMessageHandler {
+impl<F: Fn(&MessageDiscriminant) -> Option<KeysGroup>> MessageHandler<LayoutMessage, F> for LayoutMessageHandler {
 	fn process_message(&mut self, message: LayoutMessage, responses: &mut std::collections::VecDeque<Message>, action_input_mapping: F) {
 		match message {
 			LayoutMessage::ResendActiveWidget { layout_target, widget_id } => {
@@ -385,7 +385,7 @@ impl LayoutMessageHandler {
 		layout_target: LayoutTarget,
 		new_layout: Layout,
 		responses: &mut VecDeque<Message>,
-		action_input_mapping: &impl Fn(&MessageDiscriminant) -> Vec<KeysGroup>,
+		action_input_mapping: &impl Fn(&MessageDiscriminant) -> Option<KeysGroup>,
 	) {
 		match new_layout {
 			Layout::WidgetLayout(_) => {
@@ -419,7 +419,7 @@ impl LayoutMessageHandler {
 	}
 
 	/// Send a diff to the frontend based on the layout target.
-	fn send_diff(&self, mut diff: Vec<WidgetDiff>, layout_target: LayoutTarget, responses: &mut VecDeque<Message>, action_input_mapping: &impl Fn(&MessageDiscriminant) -> Vec<KeysGroup>) {
+	fn send_diff(&self, mut diff: Vec<WidgetDiff>, layout_target: LayoutTarget, responses: &mut VecDeque<Message>, action_input_mapping: &impl Fn(&MessageDiscriminant) -> Option<KeysGroup>) {
 		diff.iter_mut().for_each(|diff| diff.new_value.apply_keyboard_shortcut(action_input_mapping));
 
 		let message = match layout_target {