Merge PR #2136: #2024: dialog commands the engine defaults can be bound to a key

crates/engine/src/clip_ops.rs

@@ -185,7 +185,7 @@ pub(crate) fn commands() -> Vec<CommandSpec> {
             "Frame Hold Options…",
             &["Clip", "Video Options"],
             None,
-            r#"{"clips":[id]?,"enabled":bool=true,"holdOn":"sourceTimecode|sequenceTime|in|out|playhead","time":ticks?,"timecode":str?,"holdFilters":bool=false}"#,
+            r#"{"clips":[id]?,"enabled":bool=true,"holdOn":"sourceTimecode|sequenceTime|in|out|playhead"?,"time":ticks?,"timecode":str?,"holdFilters":bool=false}"#,
             has_selection,
             frame_hold_options,
         ),
@@ -203,7 +203,7 @@ pub(crate) fn commands() -> Vec<CommandSpec> {
             "Field Options…",
             &["Clip", "Video Options"],
             None,
-            r#"{"clips":[id]?,"reverseFieldDominance":bool=false,"processing":"none|alwaysDeinterlace|flickerRemoval"}"#,
+            r#"{"clips":[id]?,"reverseFieldDominance":bool=false,"processing":"none|alwaysDeinterlace|flickerRemoval"?}"#,
             has_selection,
             field_options,
         ),

crates/engine/src/commands.rs

@@ -1483,7 +1483,7 @@ fn build() -> Vec<CommandSpec> {
             Label::Lavender,
             p
         )),
-        cmd!("file.newColorMatte", "Color Matte…", ["File", "New"], None, r##"{"color":"#rrggbb","seconds":f64}"##, always, |s, p| {
+        cmd!("file.newColorMatte", "Color Matte…", ["File", "New"], None, r##"{"color":"#rrggbb"?,"seconds":f64=5}"##, always, |s, p| {
             let c = str_p(p, "color").and_then(filmcraft_color::parse_hex).unwrap_or([0.1, 0.1, 0.1, 1.0]);
             new_generator(s, Generator::ColorMatte { color: c }, "Color Matte", Label::Lavender, p)
         }),
@@ -2243,7 +2243,7 @@ fn build() -> Vec<CommandSpec> {
         cmd!("clip.frameHold", "Add Frame Hold", ["Clip", "Video Options"], None, r#"{"clips":[id]?,"time":ticks?}"#, has_seq, |s, p| {
             crate::clip_ops::add_frame_hold(s, p)
         }),
-        cmd!("clip.nest", "Nest…", ["Clip"], None, r#"{"name":str}"#, has_selection, |s, p| nest(s, p)),
+        cmd!("clip.nest", "Nest…", ["Clip"], None, r#"{"name":str?}"#, has_selection, |s, p| nest(s, p)),
         // Reveal in Project (clip context menu) and Reveal Sequence in Project (Timeline tab menu):
         // select the item in the Project panel and show it there
         cmd!("clip.revealInProject", "Reveal in Project", [], None, r#"{"clip":id?}"#, has_seq, |s, p| {

crates/engine/src/project_tools.rs

@@ -187,7 +187,7 @@ pub(crate) fn commands() -> Vec<CommandSpec> {
             "Automate to Sequence…",
             &["Clip"],
             None,
-            r#"{"items":[id]?,"ordering":"sort|selection","placement":"sequentially|unnumberedMarkers","method":"insert|overwrite","overlapFrames":n=30,"stillFrames":n?,"videoTransition":bool=true,"audioTransition":bool=true,"ignoreAudio":bool=false,"ignoreVideo":bool=false}"#,
+            r#"{"items":[id]?,"ordering":"sort|selection"?,"placement":"sequentially|unnumberedMarkers"?,"method":"insert|overwrite"?,"overlapFrames":n=30,"stillFrames":n?,"videoTransition":bool=true,"audioTransition":bool=true,"ignoreAudio":bool=false,"ignoreVideo":bool=false}"#,
             can_automate,
             automate_to_sequence,
         ),

crates/engine/src/proxies.rs

@@ -586,7 +586,7 @@ pub fn commands() -> Vec<CommandSpec> {
             label: "Create Proxies…",
             menu: &["Clip", "Proxy"],
             shortcut: None,
-            params: r#"{"items":[id]?,"preset":"prores_proxy_quarter|prores_proxy_half|prores_lt_half|h264_quarter|h264_half","destination":str?,"attach":bool=true,"wait":bool=false}"#,
+            params: r#"{"items":[id]?,"preset":"prores_proxy_quarter|prores_proxy_half|prores_lt_half|h264_quarter|h264_half"?,"destination":str?,"attach":bool=true,"wait":bool=false}"#,
             enabled: has_file_media,
             run: create,
             journal: true,

crates/engine/src/sequence_extras.rs

@@ -65,7 +65,7 @@ pub(crate) fn commands() -> Vec<CommandSpec> {
             "Simplify Sequence…",
             &["Sequence"],
             None,
-            r#"{"name":str?,"removeDisabled":bool=true,"removeEmptyTracks":bool=true,"closeGaps":bool=false,"moveClipsDown":bool=false,"removeVideoEffects":bool=false,"removeAudioEffects":bool=false,"removeText":bool=false,"keep":"both|video|audio"}"#,
+            r#"{"name":str?,"removeDisabled":bool=true,"removeEmptyTracks":bool=true,"closeGaps":bool=false,"moveClipsDown":bool=false,"removeVideoEffects":bool=false,"removeAudioEffects":bool=false,"removeText":bool=false,"keep":"both|video|audio"?}"#,
             has_seq,
             simplify,
         ),

crates/engine/src/shortcuts_tests.rs

@@ -291,6 +291,61 @@ fn list_searches_labels_and_keys() {
     assert_eq!(v.as_array().unwrap().len(), 3, "{v}");
 }
 
+/// A command run without parameters whose parameters are all optional or defaulted opens its
+/// dialog instead of running (ui-egui routes them through clip_dialogs / menu_dialogs /
+/// media_dialogs), so it belongs in the shortcut editor. A parameter left marked required takes it
+/// out of the list even though the engine defaults it (#2024).
+///
+/// The engine cannot see ui-egui's dialog tables, so this list is the dialog commands of today.
+/// ui-egui's `every_command_with_a_clip_dialog_can_be_bound` and
+/// `every_command_with_a_menu_dialog_can_be_bound` walk the registry against `clip_dialogs::meta`
+/// and `menu_dialogs::meta` themselves, so a command added to either table later is checked too.
+#[test]
+fn commands_that_open_dialogs_can_be_bound() {
+    let r = Session::default().shortcuts.commands();
+    for id in [
+        // clip_dialogs::meta
+        "edit.pasteAttributes",
+        "edit.removeAttributes",
+        "file.newOfflineFile",
+        "clip.nest",
+        "clip.makeSubclip",
+        "clip.editSubclip",
+        "clip.audioChannels",
+        "clip.modifyTimecode",
+        "clip.frameHoldOptions",
+        "clip.fieldOptions",
+        "clip.speedDuration",
+        "file.closeProject",
+        // menu_dialogs::meta, minus file.mediaProperties and clip.sourceSettings (views, not
+        // undoable actions: they stay out of the editor on purpose even though a default key
+        // names mediaProperties)
+        "edit.find",
+        "file.newSearchBin",
+        "clip.editOffline",
+        "clip.updateMetadata",
+        "clip.automateToSequence",
+        "clip.sceneEditDetection",
+        "sequence.normalizeMixTrack",
+        "sequence.simplify",
+        "sequence.transcribe",
+        "file.saveAsTemplate",
+        "markers.addFlashCue",
+        "file.newColorMatte",
+        // media_dialogs::route, minus the ones that relink a file picked in a native dialog
+        // (media.attachProxies, media.reconnectFullRes) and media.linkMedia (a dialog the app
+        // opens but does not edit the project, so it is not a bindable action)
+        "media.makeOffline",
+        "media.createProxies",
+    ] {
+        assert!(r.iter().any(|c| c.id == id), "{id} opens its dialog when run without parameters, so it can be bound");
+    }
+    // and the assignment goes through, so the key can be recorded and shown in the editor
+    let mut s = Session::default();
+    s.execute("shortcuts.set", json!({"command": "clip.nest", "keys": "Cmd+Alt+N"})).unwrap();
+    assert_eq!(s.shortcuts.primary("clip.nest").as_deref(), Some("Cmd+Alt+N"));
+}
+
 #[test]
 fn default_keymaps_have_no_conflicts_on_any_platform() {
     // The Premiere audit used to check keys on macOS only, so off macOS `⌃9`/`⌘9`, `⌃⇧M`/`⌘⇧M` and

crates/ui-egui/src/panels/clip_dialogs.rs

@@ -711,3 +711,33 @@ fn finish(d: &ClipDialogDraft) -> Value {
     }
     p
 }
+
+#[cfg(test)]
+pub(crate) mod tests {
+    use super::*;
+
+    /// Asserts that every undoable engine command that `opens` says opens a dialog of `table` (run
+    /// with no parameters) can be bound to a key, unless `except` names it with a reason (#2024).
+    /// The registry is walked instead of listing ids, so a command added to the table later is
+    /// checked as well. Non-undoable commands (views such as Properties) stay out of the shortcut
+    /// editor by design, so they are not checked.
+    pub(crate) fn assert_dialog_commands_bindable(table: &str, opens: impl Fn(&str) -> bool, except: &[&str]) {
+        let bindable = filmcraft_engine::Session::default().shortcuts.commands();
+        let checked: Vec<&str> =
+            filmcraft_engine::command_specs().iter().filter(|c| c.journal && opens(c.id) && !except.contains(&c.id)).map(|c| c.id).collect();
+        assert!(!checked.is_empty(), "{table}: no command was checked");
+        let missing: Vec<&str> = checked.into_iter().filter(|id| !bindable.iter().any(|b| b.id == *id)).collect();
+        assert!(
+            missing.is_empty(),
+            "{table}: {missing:?} open their dialog when run without parameters, so they must be bindable (mark the parameters the engine defaults as optional)"
+        );
+        for id in except {
+            assert!(opens(id), "{table}: {id} is excepted but no longer opens a dialog here");
+        }
+    }
+
+    #[test]
+    fn every_command_with_a_clip_dialog_can_be_bound() {
+        assert_dialog_commands_bindable("clip_dialogs::meta", |id| meta(id).is_some(), &[]);
+    }
+}

crates/ui-egui/src/panels/menu_dialogs.rs

@@ -985,3 +985,15 @@ pub fn search_bin_rows(
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn every_command_with_a_menu_dialog_can_be_bound() {
+        // project.matteColor recolours the selected matte and needs a colour: the engine refuses it
+        // without one, so its parameter is required and its dialog is reached from the matte itself
+        crate::panels::clip_dialogs::tests::assert_dialog_commands_bindable("menu_dialogs::meta", |id| meta(id).is_some(), &["project.matteColor"]);
+    }
+}