diff --git a/crates/engine/src/cmd/track.rs b/crates/engine/src/cmd/track.rs index 87912389..cc496448 100644 --- a/crates/engine/src/cmd/track.rs +++ b/crates/engine/src/cmd/track.rs @@ -19,6 +19,7 @@ pub fn specs() -> Vec { cmd!("track.group", "Group...", ["Track"], Some("Cmd+G"), "{name?, tracks?, edit?: true, mix?: true}", has_selection, group), cmd!("track.ungroup", "Delete Group", [], None, "{group: id|name}", always, delete_group), cmd!("track.group_toggle", "Toggle Group Active", [], None, "{group: id|name}", always, toggle_group), + cmd!("track.group_modify", "Modify Group", [], None, "{group: id|name, name?, tracks?, edit?, mix?}", always, modify_group), cmd!( "track.duplicate", "Duplicate...", @@ -320,6 +321,55 @@ fn toggle_group(e: &mut Engine, p: &Value) -> Result { Ok(json!({"active": active})) } +/// Change an existing group's name, membership and Edit/Mix linking. Omitted fields keep their value. +fn modify_group(e: &mut Engine, p: &Value) -> Result { + const CMD: &str = "track.group_modify"; + let g = find_group(e, p, CMD)?; + // Absent `tracks` leaves membership alone; present, it replaces it. Only an array counts, so a + // mistyped param can't silently swap the members for whatever happens to be selected. + let members = match p.get("tracks") { + None => None, + Some(Value::Array(_)) => { + let mut t = tracks_param(e, CMD, p)?; + // A track listed twice would take every linked move twice over. + let mut seen = std::collections::HashSet::new(); + t.retain(|id| seen.insert(*id)); + if t.is_empty() { + return Err(bad(CMD, "a group needs at least one track")); + } + Some(t) + } + Some(_) => return Err(bad(CMD, "`tracks` must be an array of track ids or names")), + }; + let name = match str_param(p, "name") { + None => None, + Some(n) => { + let n = n.trim(); + if n.is_empty() { + return Err(bad(CMD, "`name` must not be empty")); + } + // `find_group` resolves by name, so duplicates would make a group unaddressable. + if e.session().groups.iter().any(|x| x.name == n && x.id != g) { + return Err(bad(CMD, format!("a group named `{n}` already exists"))); + } + let mut n = n.to_string(); + soundcraft_model::truncate_chars(&mut n, soundcraft_model::MAX_NAME_CHARS); + Some(n) + } + }; + let s = e.session_mut(); + let Some(gr) = s.groups.iter_mut().find(|x| x.id == g) else { return Err(bad(CMD, "unknown `group`")) }; + if let Some(n) = name { + gr.name = n; + } + if let Some(m) = members { + gr.members = m; + } + gr.edit = bool_or(p, "edit", gr.edit); + gr.mix = bool_or(p, "mix", gr.mix); + Ok(json!({"group": g, "name": gr.name, "tracks": gr.members, "edit": gr.edit, "mix": gr.mix})) +} + fn duplicate(e: &mut Engine, p: &Value) -> Result { let tracks = tracks_required(e, "track.duplicate", p)?; let (pl, auto, ins, snd) = @@ -872,6 +922,35 @@ mod tests { assert!(e.execute("track.playlist_promote", &json!({"track": kick, "playlist": 1, "start": 0, "end": 10})).is_err()); } + #[test] + fn group_modify_changes_name_membership_and_linking() { + let mut e = crate::demo::demo_engine(); + let g = e.execute("track.group", &json!({"tracks": ["Kick", "Snare"], "name": "Drums"})).unwrap()["group"].as_u64().unwrap(); + // Every field at once, the way the dialog submits it. + let r = e.execute("track.group_modify", &json!({"group": g, "name": "Rhythm", "tracks": ["Kick", "Bass"], "edit": false})).unwrap(); + assert_eq!(r["name"], "Rhythm"); + let gr = e.session().groups.iter().find(|x| x.id.0 == g).unwrap().clone(); + assert_eq!(gr.name, "Rhythm"); + assert_eq!(gr.members, vec![e.session().track_by_name("Kick").unwrap().id, e.session().track_by_name("Bass").unwrap().id]); + assert!(!gr.edit); + assert!(gr.mix, "an omitted field keeps its value"); + assert_eq!(gr.letter, 'a', "the letter is the group's stable id and never changes"); + // Omitting `tracks` leaves membership alone rather than falling back to the selection. + e.execute("edit.select", &json!({"tracks": ["Pad"], "exact": true})).unwrap(); + e.execute("track.group_modify", &json!({"group": g, "name": "Rhythm Section"})).unwrap(); + assert_eq!(e.session().groups.iter().find(|x| x.id.0 == g).unwrap().members, gr.members); + // Resolving the group by its new name keeps working, so names stay unique and non-empty. + e.execute("track.group", &json!({"tracks": ["Pad"], "name": "Pads"})).unwrap(); + assert!(e.execute("track.group_modify", &json!({"group": "Pads", "name": "Rhythm Section"})).is_err()); + assert!(e.execute("track.group_modify", &json!({"group": g, "name": " "})).is_err()); + assert!(e.execute("track.group_modify", &json!({"group": g, "tracks": []})).is_err()); + assert!(e.execute("track.group_modify", &json!({"group": g, "tracks": "Kick"})).is_err(), "`tracks` must be an array"); + assert!(e.execute("track.group_modify", &json!({"group": g, "tracks": ["Kick", "Kick"]})).is_ok()); + assert_eq!(e.session().groups.iter().find(|x| x.id.0 == g).unwrap().members.len(), 1, "a repeated track is listed once"); + assert!(e.execute("track.group_modify", &json!({"group": 9999, "name": "x"})).is_err()); + assert_eq!(e.session().groups.iter().find(|x| x.id.0 == g).unwrap().name, "Rhythm Section", "a rejected call changes nothing"); + } + #[test] fn folder_routes_members_through_its_bus() { let mut e = crate::demo::demo_engine(); diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 713e94b7..7b43afcb 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -5,7 +5,7 @@ use crate::theme::{Tokens, bold}; use egui::{Align2, vec2}; use serde_json::{Value, json}; use soundcraft_engine::Engine; -use soundcraft_model::{ClipId, Session, SourceId, TrackId}; +use soundcraft_model::{ClipId, GroupId, Session, SourceId, TrackId}; use soundcraft_time::Samples; #[derive(Debug, Clone)] @@ -23,7 +23,8 @@ pub enum Dialog { Spot(crate::spot::Spot), Fades { shape: String }, StripSilence { threshold: f64, min_ms: f64, pre_ms: f64, post_ms: f64 }, - Group { name: String, edit: bool, mix: bool, members: Vec<(u64, String, bool)> }, + // `id: None` creates (Track › Group…); `id: Some` modifies that group (Groups list › Modify Group…). + Group { id: Option, name: String, edit: bool, mix: bool, members: Vec<(u64, String, bool)> }, Session { frame_rate: String, bit_depth: String }, ScoreSetup { title: String, composer: String, bars_per_system: u32, show_track_names: bool }, SaveChanges { then: (String, Value) }, @@ -52,7 +53,8 @@ impl Dialogs { Dialog::Spot(_) => "spot", Dialog::Fades { .. } => "fades", Dialog::StripSilence { .. } => "strip_silence", - Dialog::Group { .. } => "group", + Dialog::Group { id: None, .. } => "group", + Dialog::Group { id: Some(_), .. } => "modify_group", Dialog::Session { .. } => "session", Dialog::ScoreSetup { .. } => "score_setup", Dialog::SaveChanges { .. } => "save_changes", @@ -71,6 +73,16 @@ impl Dialogs { self.open = Some(Dialog::RenameMarker { id, name: name.to_string() }); } + /// Groups list › Modify Group…: the creation dialog, pre-filled from an existing group. False + /// when the group is gone (deleted between the click and the menu). + pub fn open_modify_group(&mut self, e: &Engine, id: GroupId) -> bool { + let s = e.session(); + let Some(g) = s.group(id) else { return false }; + let members = s.tracks.iter().map(|t| (t.id.0, t.name.clone(), g.members.contains(&t.id))).collect(); + self.open = Some(Dialog::Group { id: Some(id), name: g.name.clone(), edit: g.edit, mix: g.mix, members }); + true + } + /// Clip List › Clear: confirm before clips and files leave the session. False when there is nothing to clear. pub fn open_clear(&mut self, e: &Engine, clips: Vec, sources: Vec) -> bool { let Some(message) = clear_message(e.session(), &clips, &sources) else { return false }; @@ -131,7 +143,7 @@ impl Dialogs { "track.group" => { let s = e.session(); let members = s.tracks.iter().map(|t| (t.id.0, t.name.clone(), s.edit.selected_tracks.contains(&t.id))).collect(); - Dialog::Group { name: format!("Group {}", s.groups.len() + 1), edit: true, mix: true, members } + Dialog::Group { id: None, name: format!("Group {}", s.groups.len() + 1), edit: true, mix: true, members } } "setup.session" => Dialog::Session { frame_rate: e.session().frame_rate.label().into(), bit_depth: "24".into() }, "file.score_setup" => { @@ -283,7 +295,8 @@ pub fn show(app: &mut SoundApp, ctx: &egui::Context) { Dialog::Spot(_) => "Spot", Dialog::Fades { .. } => "Fades", Dialog::StripSilence { .. } => "Strip Silence", - Dialog::Group { .. } => "Create Group", + Dialog::Group { id: None, .. } => "Create Group", + Dialog::Group { id: Some(_), .. } => "Modify Group", Dialog::Session { .. } => "Session Setup", Dialog::ScoreSetup { .. } => "Score Setup", Dialog::SaveChanges { then } if then.0 == "session.revert" => "Revert to Saved", @@ -508,7 +521,7 @@ pub fn show(app: &mut SoundApp, ctx: &egui::Context) { )); } } - Dialog::Group { name, edit, mix, members } => { + Dialog::Group { id, name, edit, mix, members } => { ui.horizontal(|ui| { ui.label("Name"); ui.text_edit_singleline(name); @@ -523,9 +536,18 @@ pub fn show(app: &mut SoundApp, ctx: &egui::Context) { crate::widgets::checkbox(ui, on, n.as_str()); } }); - if buttons(ui, "OK", enter) { - let ids: Vec = members.iter().filter(|m| m.2).map(|m| m.0).collect(); - action = Some(("track.group".into(), json!({"name": name, "edit": edit, "mix": mix, "tracks": ids}))); + let ids: Vec = members.iter().filter(|m| m.2).map(|m| m.0).collect(); + // Both commands reject an empty group, so don't offer an OK that can only fail. + if buttons_enabled(ui, "OK", enter, !ids.is_empty()) { + let mut params = json!({"name": name, "edit": edit, "mix": mix, "tracks": ids}); + let cmd = match id { + Some(g) => { + params["group"] = json!(g.0); + "track.group_modify" + } + None => "track.group", + }; + action = Some((cmd.into(), params)); } } Dialog::Session { frame_rate, bit_depth } => { @@ -635,13 +657,19 @@ pub fn show(app: &mut SoundApp, ctx: &egui::Context) { /// Cancel/OK row; returns true when OK was pressed (or Enter). fn buttons(ui: &mut egui::Ui, ok: &str, enter: bool) -> bool { - let mut pressed = enter; + buttons_enabled(ui, ok, enter, true) +} + +/// `buttons`, but a greyed-out OK when the dialog's values can't be submitted yet. Cancel always works. +fn buttons_enabled(ui: &mut egui::Ui, ok: &str, enter: bool, ready: bool) -> bool { + let mut pressed = enter && ready; ui.separator(); ui.horizontal(|ui| { if ui.button("Cancel").clicked() { ui.ctx().memory_mut(|m| m.data.insert_temp(egui::Id::new("dlg_cancel"), true)); } - if ui.add(egui::Button::new(egui::RichText::new(ok).strong()).fill(Tokens::current().accent)).clicked() { + let b = egui::Button::new(egui::RichText::new(ok).strong()).fill(Tokens::current().accent); + if ui.add_enabled(ready, b).clicked() { pressed = true; } }); diff --git a/crates/ui-egui/src/panels.rs b/crates/ui-egui/src/panels.rs index dda31ce0..de46e336 100644 --- a/crates/ui-egui/src/panels.rs +++ b/crates/ui-egui/src/panels.rs @@ -133,6 +133,10 @@ pub fn tracks_and_groups(app: &mut SoundApp, ui: &mut Ui) { let _ = app.run("track.group_toggle", json!({"group": id})); ui.close(); } + if ui.button("Modify Group…").clicked() { + let _ = app.dialogs.open_modify_group(&app.engine, soundcraft_model::GroupId(id)); + ui.close(); + } if ui.button("Delete Group").clicked() { let _ = app.run("track.ungroup", json!({"group": id})); ui.close(); diff --git a/crates/ui-egui/tests/group_dialog.rs b/crates/ui-egui/tests/group_dialog.rs index e91bfec1..bd17ede9 100644 --- a/crates/ui-egui/tests/group_dialog.rs +++ b/crates/ui-egui/tests/group_dialog.rs @@ -42,3 +42,44 @@ fn dragging_down_the_group_dialogs_track_list_adds_each_track() { let members: Vec = session.groups.last().map(|g| g.members.iter().map(|m| m.0).collect()).unwrap_or_default(); assert_eq!(members, tracks[..4].iter().map(|t| t.0).collect::>()); } + +/// The Groups list's Modify Group… reopens the creation dialog on an existing group: it comes up +/// pre-filled, and OK edits that group in place instead of adding a second one. +#[test] +fn modify_group_dialog_edits_the_existing_group() { + let mut app = SoundApp::new(soundcraft_engine::demo::demo_engine(), None, Services::default()); + let tracks: Vec<(u64, String)> = app.engine.session().tracks.iter().map(|t| (t.id.0, t.name.clone())).collect(); + assert!(tracks.len() >= 2); + let g = app.run("track.group", json!({"tracks": [tracks[0].0], "name": "Drums"})).unwrap()["group"].as_u64().unwrap(); + + assert!(app.dialogs.open_modify_group(&app.engine, soundcraft_model::GroupId(g))); + assert_eq!(app.dialogs.open_name(), Some("modify_group")); + // Pre-filled from the group, not from the track selection. + let Some(soundcraft_ui_egui::dialogs::Dialog::Group { id, name, edit, mix, members }) = &app.dialogs.open else { + panic!("Modify Group… did not open the group dialog"); + }; + assert_eq!((id.map(|i| i.0), name.as_str(), *edit, *mix), (Some(g), "Drums", true, true)); + assert_eq!(members.iter().filter(|m| m.2).map(|m| m.0).collect::>(), vec![tracks[0].0]); + + let mut h = Harness::builder().with_size(vec2(1600.0, 1000.0)).build_ui_state( + |ui, app: &mut SoundApp| { + let ctx = ui.ctx().clone(); + app.logic(&ctx); + app.ui(ui); + }, + app, + ); + h.run_steps(3); + // Add a second track and submit. + h.get_by_role_and_label(Role::CheckBox, &tracks[1].1).click(); + h.run_steps(2); + h.get_by_role_and_label(Role::Button, "OK").click(); + h.run_steps(2); + + let session = h.state().engine.session(); + assert_eq!(session.groups.len(), 1, "Modify edits the group in place rather than creating another"); + let gr = &session.groups[0]; + assert_eq!(gr.id.0, g); + assert_eq!(gr.name, "Drums"); + assert_eq!(gr.members.iter().map(|m| m.0).collect::>(), vec![tracks[0].0, tracks[1].0]); +}