fix: use normal shortcut modifier order and prefer shorter shortcuts (#666)

Behaviour changes:
- Sort modifiers so we don't say Shift+Cmd+Z for redo on Mac
- Ctrl+Y is the preferred redo shortcut on Windows and not all apps support the
  more cross-platform Ctrl+Shift+Z

Implementation change:
- Map to user-facing key names last to keep earlier modifier logic using the
  shortcut registry representation of the modifiers. This the same point we do
  translations of modifiers in the equivalent code in MakeCode.

Fixes #647

Author: microbit-matt-hillsdon

src/shortcut_formatting.ts

@@ -79,38 +79,52 @@ function getActionShortcutsAsKeys(
   modifierNames: Record<string, string>,
 ): string[][] {
   const shortcuts = ShortcutRegistry.registry.getKeyCodesByShortcutName(action);
+  if (shortcuts.length === 0) {
+    return [];
+  }
   // See ShortcutRegistry.createSerializedKey for the starting format.
-  const named = shortcuts.map((shortcut) => {
-    return shortcut
-      .split('+')
-      .map((maybeNumeric) => keyNames[maybeNumeric] ?? maybeNumeric)
-      .map((k) => upperCaseFirst(modifierNames[k] ?? k));
-  });
+  const shortcutsAsParts = shortcuts.map((shortcut) => shortcut.split('+'));
+
+  // Prefer e.g. Cmd+Shift to Shift+Cmd.
+  shortcutsAsParts.forEach((s) =>
+    s.sort((a, b) => {
+      const aValue = modifierOrder(a);
+      const bValue = modifierOrder(b);
+      return aValue - bValue;
+    }),
+  );
 
-  const command = modifierNames['Meta'];
-  const option = modifierNames['Alt'];
-  const control = modifierNames['Control'];
   // Needed to prefer Command to Option where we've bound Alt.
-  named.sort((a, b) => {
-    const aValue = a.includes(command) ? 1 : 0;
-    const bValue = b.includes(command) ? 1 : 0;
+  shortcutsAsParts.sort((a, b) => {
+    const aValue = a.includes('Meta') ? 1 : 0;
+    const bValue = b.includes('Meta') ? 1 : 0;
     return bValue - aValue;
   });
-  let currentPlatform = named.filter((shortcut) => {
-    const isMacShortcut =
-      shortcut.includes(command) || shortcut.includes(option);
+  let currentPlatform = shortcutsAsParts.filter((shortcut) => {
+    const isMacShortcut = shortcut.includes('Meta');
     return isMacShortcut === isMacPlatform;
   });
-  currentPlatform = currentPlatform.length === 0 ? named : currentPlatform;
+  currentPlatform =
+    currentPlatform.length === 0 ? shortcutsAsParts : currentPlatform;
+
+  // Prefer simpler shortcuts. This promotes Ctrl+Y for redo.
+  currentPlatform.sort((a, b) => {
+    return a.length - b.length;
+  });
 
   // If there are modifiers return only one shortcut on the assumption they are
   // intended for different platforms. Otherwise assume they are alternatives.
   const hasModifiers = currentPlatform.some((shortcut) =>
     shortcut.some(
-      (key) => command === key || option === key || control === key,
+      (key) => 'Meta' === key || 'Alt' === key || 'Control' === key,
     ),
   );
-  return hasModifiers ? [currentPlatform[0]] : currentPlatform;
+  const chosen = hasModifiers ? [currentPlatform[0]] : currentPlatform;
+  return chosen.map((shortcut) => {
+    return shortcut
+      .map((maybeNumeric) => keyNames[maybeNumeric] ?? maybeNumeric)
+      .map((k) => upperCaseFirst(modifierNames[k] ?? k));
+  });
 }
 
 /**
@@ -122,3 +136,14 @@ function getActionShortcutsAsKeys(
 export function upperCaseFirst(str: string) {
   return str.charAt(0).toUpperCase() + str.substring(1);
 }
+
+/**
+ * Preferred listing order of untranslated modifiers.
+ */
+const modifierOrdering: string[] = ['Meta', 'Control', 'Alt', 'Shift'];
+
+function modifierOrder(key: string): number {
+  const order = modifierOrdering.indexOf(key);
+  // Regular keys at the end.
+  return order === -1 ? Number.MAX_VALUE : order;
+}