From 76a6951152066ea74f39e65aa23afce597b39cd6 Mon Sep 17 00:00:00 2001 From: Nate Bross Date: Thu, 16 Jul 2026 18:22:11 -0500 Subject: [PATCH 1/3] fix(scripting): apply step params in place instead of rebuilding from display text --- src/SharpFM.Model/Scripting/FmScript.cs | 88 ++----- src/SharpFM.Model/Scripting/ScriptStep.cs | 11 + src/SharpFM.Model/Scripting/ScriptStepOfT.cs | 3 + .../Serialization/StepDisplayParser.cs | 2 +- .../Serialization/StepParamApplier.cs | 99 +++++++ .../Scripting/Steps/GoToLayoutStep.cs | 88 +++++-- .../Scripting/Steps/ShowCustomDialogStep.cs | 20 ++ .../Scripting/FmScriptApplyTests.cs | 248 ++++++++++++++++++ 8 files changed, 478 insertions(+), 81 deletions(-) create mode 100644 src/SharpFM.Model/Scripting/Serialization/StepParamApplier.cs diff --git a/src/SharpFM.Model/Scripting/FmScript.cs b/src/SharpFM.Model/Scripting/FmScript.cs index 9c07e484..b079db5c 100644 --- a/src/SharpFM.Model/Scripting/FmScript.cs +++ b/src/SharpFM.Model/Scripting/FmScript.cs @@ -192,15 +192,15 @@ private List ApplyAdd(ScriptStepOperation op) if (!Registry.StepRegistry.ByName.TryGetValue(op.StepName, out var metadata)) return [$"Unknown step name '{op.StepName}'."]; - // Route through each POCO's FromDisplay factory with the caller's - // param map synthesized into HR tokens. The synthesizer consults the - // step's metadata so positional params (no HrLabel) pass as raw - // values and labeled params get the canonical "Label: value" form. - var hrParams = SynthesizeHrParams(op.Params, metadata); - var step = StepDisplayFactory.TryCreate(op.StepName, op.Enabled ?? true, hrParams); + // Construct a blank instance through the registry factory, then set + // each caller param directly onto the POCO's shape-bound properties. + var step = StepDisplayFactory.TryCreate(metadata.Name, op.Enabled ?? true, []); if (step is null) return [$"No typed POCO factory registered for '{op.StepName}'."]; + var errors = ApplyParams(step, op.Params); + if (errors.Count > 0) return errors; + var index = op.Index < 0 || op.Index >= Steps.Count ? Steps.Count : op.Index; Steps.Insert(index, step); return []; @@ -211,74 +211,42 @@ private List ApplyUpdate(ScriptStepOperation op) if (ValidateStepIndex(op.Index) is { } err) return [err]; var step = Steps[op.Index]; - if (op.Enabled is not null) step.Enabled = op.Enabled.Value; - - if (op.Params is null) return []; - - // Param updates are rebuilt by re-parsing the display form with the - // new param map overlaid onto the old. This uses the POCO's own - // FromDisplay factory — the same path ApplyAdd takes — so the - // update never leaves the typed-POCO world. - var metadata = Registry.StepRegistry.MetadataFor(step); - if (metadata is null) + if (op.Params is not null && op.Params.Count > 0 + && Registry.StepRegistry.MetadataFor(step) is null) + { return [$"Apply/update is not supported for step kind '{step.GetType().Name}'."]; + } - var hrParams = SynthesizeHrParams(op.Params, metadata); - var updated = StepDisplayFactory.TryCreate(metadata.Name, step.Enabled, hrParams); - if (updated is null) - return [$"No typed POCO factory registered for '{metadata.Name}'."]; + if (op.Enabled is not null) step.Enabled = op.Enabled.Value; - Steps[op.Index] = updated; - return []; + return ApplyParams(step, op.Params); } /// - /// Convert a caller's param map into the ordered HR-token form each step's - /// FromDisplay factory expects. Iterates the shape's display slots in - /// display order, formatting each match as "HrLabel: value" when the - /// slot has a label and as a raw positional value when it doesn't (e.g. - /// SetVariableStep.Name, IfStep.Condition — these go straight - /// into <Name> / <Calculation> without prefix). - /// Keys may address a slot by its bound property, its XML element, or its - /// display label, so pre-cutover param names keep working. + /// Sets each param onto the step's shape-bound public properties in + /// place, through 's virtual dispatch. + /// Properties the map does not name keep their current values. Params + /// apply in map order and application is not atomic: on error, params + /// earlier in the map have already been set. /// - /// - /// An earlier implementation labeled every non-empty key, which caused - /// positional params to receive a "Name: $foo" string verbatim into - /// XML — structurally valid but semantically broken under FileMaker. - /// - private static string[] SynthesizeHrParams( - IReadOnlyDictionary? map, - Registry.StepMetadata metadata) + private static List ApplyParams(ScriptStep step, IReadOnlyDictionary? map) { if (map is null || map.Count == 0) return []; - var consumedKeys = new HashSet(StringComparer.OrdinalIgnoreCase); - var result = new List(map.Count); - - foreach (var node in Shapes.ShapeHrView.HrNodes(metadata.Shape)) + var errors = new List(); + foreach (var (name, value) in map) { - var matchedKey = map.Keys.FirstOrDefault(k => - !consumedKeys.Contains(k) && Shapes.ShapeHrView.MatchesName(node, k)); - if (matchedKey is null) continue; - - consumedKeys.Add(matchedKey); - var value = map[matchedKey]; - result.Add(node.HrLabel is not null - ? $"{node.HrLabel}: {value}" - : value); - } + if (value is null) + { + errors.Add($"Param '{name}' has no value."); + continue; + } - // Forward-compat: any keys we don't recognise pass through with the - // old formatting so newly-introduced params keep working until their - // metadata catches up. - foreach (var (k, v) in map) - { - if (consumedKeys.Contains(k)) continue; - result.Add(string.IsNullOrEmpty(k) ? v : $"{k}: {v}"); + if (step.ApplyParam(name, value) is { } error) + errors.Add(error); } - return result.ToArray(); + return errors; } private List ApplyRemove(ScriptStepOperation op) diff --git a/src/SharpFM.Model/Scripting/ScriptStep.cs b/src/SharpFM.Model/Scripting/ScriptStep.cs index fc4a2269..e7f46b67 100644 --- a/src/SharpFM.Model/Scripting/ScriptStep.cs +++ b/src/SharpFM.Model/Scripting/ScriptStep.cs @@ -42,6 +42,17 @@ protected ScriptStep(bool enabled) /// protected internal abstract void PopulateFromDisplay(string[] hrParams); + /// + /// Sets one shape-bound public property from an HR-friendly text value, + /// mutating this instance in place; properties not named are untouched. + /// Returns null on success, or a human-readable error message. Steps + /// whose slots need hand grammar (button blocks, variant targets) + /// override this and fall back to base for everything else. The base + /// rejects everything — RawStep has no param surface. + /// + protected internal virtual string? ApplyParam(string name, string value) => + $"Step '{GetType().Name}' does not support param updates."; + public virtual List Validate(int lineIndex) => new(); /// diff --git a/src/SharpFM.Model/Scripting/ScriptStepOfT.cs b/src/SharpFM.Model/Scripting/ScriptStepOfT.cs index 89fd1a59..92090050 100644 --- a/src/SharpFM.Model/Scripting/ScriptStepOfT.cs +++ b/src/SharpFM.Model/Scripting/ScriptStepOfT.cs @@ -30,6 +30,9 @@ protected internal override void PopulateFromXml(XElement step) => protected internal override void PopulateFromDisplay(string[] hrParams) => StepDisplayParser.Populate(this, hrParams, TSelf.Metadata); + protected internal override string? ApplyParam(string name, string value) => + StepParamApplier.Apply(this, TSelf.Metadata, name, value); + /// Typed counterpart of the dispatch /// for callers that already know the step kind. public static TSelf Parse(XElement step) diff --git a/src/SharpFM.Model/Scripting/Serialization/StepDisplayParser.cs b/src/SharpFM.Model/Scripting/Serialization/StepDisplayParser.cs index 6f1f4506..8f57d94c 100644 --- a/src/SharpFM.Model/Scripting/Serialization/StepDisplayParser.cs +++ b/src/SharpFM.Model/Scripting/Serialization/StepDisplayParser.cs @@ -131,7 +131,7 @@ private static void Assign(object target, ShapeNode node, string value) } /// Inverse of the renderer's wire→display translation. - private static string ToWireValue(ShapeNode node, string display) + internal static string ToWireValue(ShapeNode node, string display) { if (node.DisplayValues is null || node.ValidValues is null) return display; var i = node.DisplayValues.ToList().FindIndex(v => v.Equals(display, StringComparison.OrdinalIgnoreCase)); diff --git a/src/SharpFM.Model/Scripting/Serialization/StepParamApplier.cs b/src/SharpFM.Model/Scripting/Serialization/StepParamApplier.cs new file mode 100644 index 00000000..72f114a7 --- /dev/null +++ b/src/SharpFM.Model/Scripting/Serialization/StepParamApplier.cs @@ -0,0 +1,99 @@ +using System; +using System.Linq; +using SharpFM.Model.Scripting.Registry; +using SharpFM.Model.Scripting.Shapes; +using SharpFM.Model.Scripting.Values; + +namespace SharpFM.Model.Scripting.Serialization; + +/// +/// Applies one named HR-friendly param value onto a step POCO's shape-bound +/// public property, mutating the instance in place — the structured-caller +/// counterpart of . Where display parsing is +/// tolerant by contract (display text is user-edited), param application is +/// validating: unknown names, slots with no text representation, and +/// out-of-range values return an error instead of being silently dropped. +/// +internal static class StepParamApplier +{ + /// + /// Resolves to a display slot (by bound property, + /// XML element, or display label — see ) + /// and assigns the converted value. Returns null on success, or a + /// human-readable error message. + /// + public static string? Apply(ScriptStep target, StepMetadata meta, string name, string value) + { + var slot = ShapeHrView.HrNodes(meta.Shape).FirstOrDefault(n => ShapeHrView.MatchesName(n, name)); + if (slot is null) + return $"Unknown param '{name}' for step '{meta.Name}'."; + + return Assign(target, meta, slot, name, value); + } + + private static string? Assign(ScriptStep target, StepMetadata meta, ShapeNode node, string name, string value) + { + switch (node) + { + case BoolStateChild b: + if (!TryParseOnOff(value, out var state)) + return OnOffError(meta, name, value); + ShapeReflection.Set(target, b.PocoProperty ?? b.Element, state != b.DisplayInverted); + return null; + + case FlagChild f: + if (!TryParseOnOff(value, out var present)) + return OnOffError(meta, name, value); + ShapeReflection.Set(target, f.PocoProperty ?? f.Element, present != f.DisplayInverted); + return null; + + case EnumValueChild e: + var wire = StepDisplayParser.ToWireValue(e, value); + if (e.ValidValues is { Count: > 0 } valid && !valid.Contains(wire, StringComparer.OrdinalIgnoreCase)) + return $"Param '{name}' of step '{meta.Name}' must be one of: " + + $"{string.Join(", ", ShapeHrView.DisplayValuesOf(e))}. Got '{value}'."; + ShapeReflection.Set(target, e.PocoProperty ?? e.Element, wire); + return null; + + case BareCalcChild: + ShapeReflection.Set(target, node.PocoProperty ?? "Calculation", new Calculation(value)); + return null; + + case NamedCalcChild nc: + ShapeReflection.Set(target, nc.PocoProperty ?? nc.Element, new Calculation(value)); + return null; + + case NamedTextChild nt: + ShapeReflection.Set(target, nt.PocoProperty ?? nt.Element, value); + return null; + + case FieldChild f: + if (value.Length == 0) + return $"Param '{name}' of step '{meta.Name}' requires a field reference " + + "(e.g. \"Table::Field\" or \"$variable\")."; + ShapeReflection.Set(target, f.PocoProperty ?? f.Element, FieldRef.FromDisplayToken(value)); + return null; + + case NamedRefChild nr: + // The text form carries the name only; id 0 is the unknown sentinel. + ShapeReflection.Set(target, nr.PocoProperty ?? nr.Element, new NamedRef(0, value)); + return null; + + // HrOnly slots carry no XML binding of their own, and variant / + // value-type / list slots have no generic text form. A step that + // supports text values for these overrides ScriptStep.ApplyParam. + default: + return $"Param '{name}' of step '{meta.Name}' cannot be set from a text value; " + + "edit the step XML instead."; + } + } + + private static string OnOffError(StepMetadata meta, string name, string value) => + $"Param '{name}' of step '{meta.Name}' must be 'On' or 'Off' (got '{value}')."; + + private static bool TryParseOnOff(string value, out bool on) + { + on = value.Equals("On", StringComparison.OrdinalIgnoreCase); + return on || value.Equals("Off", StringComparison.OrdinalIgnoreCase); + } +} diff --git a/src/SharpFM.Model/Scripting/Steps/GoToLayoutStep.cs b/src/SharpFM.Model/Scripting/Steps/GoToLayoutStep.cs index 263199d3..275db1e7 100644 --- a/src/SharpFM.Model/Scripting/Steps/GoToLayoutStep.cs +++ b/src/SharpFM.Model/Scripting/Steps/GoToLayoutStep.cs @@ -140,27 +140,9 @@ protected internal override void PopulateFromDisplay(string[] hrParams) if (!string.IsNullOrEmpty(wire)) animation = new Animation(wire); } - else if (token.Equals("original layout", StringComparison.OrdinalIgnoreCase)) + else if (TryParseTargetToken(token, out var parsed)) { - target = new LayoutTarget.Original(); - } - else if (token.StartsWith("Layout Name:", StringComparison.OrdinalIgnoreCase)) - { - var expr = token.Substring("Layout Name:".Length).Trim(); - target = new LayoutTarget.ByNameCalc(new Calculation(expr)); - } - else if (token.StartsWith("Layout Number:", StringComparison.OrdinalIgnoreCase)) - { - var expr = token.Substring("Layout Number:".Length).Trim(); - target = new LayoutTarget.ByNumberCalc(new Calculation(expr)); - } - // Named layout with (#id) suffix is the lossless form. Bare - // quoted names without an id degrade to a NamedRef with id 0 — - // the user edited the display text and dropped the id, there's - // nothing better we can do. - else if (DisplayQuoting.TryParseNamedRef(token, out var namedRef)) - { - target = new LayoutTarget.Named(namedRef); + target = parsed; } } @@ -168,6 +150,72 @@ protected internal override void PopulateFromDisplay(string[] hrParams) Animation = animation; } + /// + /// Recognizes the four display forms of a layout target: + /// original layout, a quoted name with optional (#id) + /// suffix, Layout Name: <calc>, and + /// Layout Number: <calc>. + /// + private static bool TryParseTargetToken(string token, out LayoutTarget target) + { + if (token.Equals("original layout", StringComparison.OrdinalIgnoreCase)) + { + target = new LayoutTarget.Original(); + return true; + } + + if (token.StartsWith("Layout Name:", StringComparison.OrdinalIgnoreCase)) + { + var expr = token.Substring("Layout Name:".Length).Trim(); + target = new LayoutTarget.ByNameCalc(new Calculation(expr)); + return true; + } + + if (token.StartsWith("Layout Number:", StringComparison.OrdinalIgnoreCase)) + { + var expr = token.Substring("Layout Number:".Length).Trim(); + target = new LayoutTarget.ByNumberCalc(new Calculation(expr)); + return true; + } + + // Named layout with (#id) suffix is the lossless form. Bare quoted + // names without an id degrade to a NamedRef with id 0 — the caller + // didn't supply an id, there's nothing better we can do. + if (DisplayQuoting.TryParseNamedRef(token, out var namedRef)) + { + target = new LayoutTarget.Named(namedRef); + return true; + } + + target = new LayoutTarget.Original(); + return false; + } + + // Hand-written: the target is a discriminated union selected by the + // value's display form, which the shape applier cannot convert. + protected internal override string? ApplyParam(string name, string value) + { + if (name.Equals("Layout", StringComparison.OrdinalIgnoreCase) + || name.Equals("LayoutDestination", StringComparison.OrdinalIgnoreCase) + || name.Equals("Target", StringComparison.OrdinalIgnoreCase)) + { + if (!TryParseTargetToken(value.Trim(), out var target)) + return $"Param '{name}' of step '{XmlName}' must be 'original layout', a quoted " + + "layout name (optionally with a (#id) suffix), 'Layout Name: ', or " + + $"'Layout Number: '. Got '{value}'."; + Target = target; + return null; + } + + if (name.Equals("Animation", StringComparison.OrdinalIgnoreCase)) + { + Animation = string.IsNullOrWhiteSpace(value) ? null : new Animation(value.Trim()); + return null; + } + + return base.ApplyParam(name, value); + } + public static StepMetadata Metadata { get; } = new() { Name = XmlName, diff --git a/src/SharpFM.Model/Scripting/Steps/ShowCustomDialogStep.cs b/src/SharpFM.Model/Scripting/Steps/ShowCustomDialogStep.cs index 9ad1d03b..1943c837 100644 --- a/src/SharpFM.Model/Scripting/Steps/ShowCustomDialogStep.cs +++ b/src/SharpFM.Model/Scripting/Steps/ShowCustomDialogStep.cs @@ -160,6 +160,26 @@ private static string FormatInputSlot(ShowCustomDialogInputField input) return $"{labelText} {password} {target}"; } + // Hand-written: the button/input blocks use the nested display grammar + // ToDisplayLine renders, which the shape applier cannot convert. + protected internal override string? ApplyParam(string name, string value) + { + if (name.Equals("Buttons", StringComparison.OrdinalIgnoreCase)) + { + Buttons = ParseButtonBlock(value); + return null; + } + + if (name.Equals("InputFields", StringComparison.OrdinalIgnoreCase) + || name.Equals("Inputs", StringComparison.OrdinalIgnoreCase)) + { + InputFields = ParseInputBlock(value); + return null; + } + + return base.ApplyParam(name, value); + } + protected internal override void PopulateFromDisplay(string[] hrParams) { var title = new Calculation(""); diff --git a/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs b/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs index 6c634a27..daffb894 100644 --- a/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs +++ b/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs @@ -2,6 +2,7 @@ using System.Linq; using SharpFM.Model.Scripting; using SharpFM.Model.Scripting.Steps; +using SharpFM.Model.Scripting.Values; using Xunit; namespace SharpFM.Tests.Scripting; @@ -122,4 +123,251 @@ public void ApplyUpdate_SetVariable_PositionalNameDoesNotReceiveLabelPrefix() var step = Assert.IsType(script.Steps[0]); Assert.Equal("$new", step.Name); } + + [Fact] + public void ApplyUpdate_PreservesParamsNotInMap() + { + var script = EmptyScript(); + script.Apply(new ScriptStepOperation( + Action: "add", + StepName: "Set Variable", + Params: new Dictionary { ["Name"] = "$x", ["Value"] = "1" })); + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary { ["Value"] = "2" }); + + Assert.Empty(script.Apply(update)); + + var step = Assert.IsType(script.Steps[0]); + Assert.Equal("$x", step.Name); + Assert.Equal("2", step.Value.Text); + } + + [Fact] + public void ApplyUpdate_MutatesStepInstanceInPlace() + { + var script = EmptyScript(); + script.Apply(new ScriptStepOperation( + Action: "add", + StepName: "Set Variable", + Params: new Dictionary { ["Name"] = "$x", ["Value"] = "1" })); + var before = script.Steps[0]; + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary { ["Value"] = "2" }); + + Assert.Empty(script.Apply(update)); + + Assert.Same(before, script.Steps[0]); + } + + [Fact] + public void ApplyUpdate_PreservesStateDisplayTextCannotCarry() + { + // Custom dialog buttons only exist in the XML; a Title-only update + // must not reset them (or the Message) to defaults. + var script = FmScript.FromXml(""" + + + <Calculation><![CDATA["Hi"]]></Calculation> + + + + + internal static class StepParamApplier { @@ -24,15 +35,47 @@ internal static class StepParamApplier /// public static string? Apply(ScriptStep target, StepMetadata meta, string name, string value) { - var slot = ShapeHrView.HrNodes(meta.Shape).FirstOrDefault(n => ShapeHrView.MatchesName(n, name)); - if (slot is null) + // Several shapes declare an HrOnly display alias next to the slot + // that actually binds the property (e.g. Insert Audio/Video's + // UniversalPathList), so prefer a typed-assignable match over the + // first name match. + var matches = ShapeHrView.HrNodes(meta.Shape).Where(n => ShapeHrView.MatchesName(n, name)).ToList(); + if (matches.Count == 0) return $"Unknown param '{name}' for step '{meta.Name}'."; + var slot = matches.FirstOrDefault(n => IsTypedAssignable(target, n)) ?? matches[0]; return Assign(target, meta, slot, name, value); } + /// + /// True when the typed conversion in can set this + /// slot directly: a convertible node kind whose bound property is + /// writable. Emit-only projections (e.g. Perform Script's + /// ParameterBeforeScript) and enums whose display values have no paired + /// wire values are excluded — they route through the display grammar, + /// which reaches the real storage. + /// + private static bool IsTypedAssignable(ScriptStep target, ShapeNode node) => node switch + { + BoolStateChild b => ShapeReflection.CanWrite(target, b.PocoProperty ?? b.Element), + FlagChild f => ShapeReflection.CanWrite(target, f.PocoProperty ?? f.Element), + // Display→wire translation pairs DisplayValues with ValidValues by + // index; DisplayValues alone leaves the wire form unknowable. + EnumValueChild e => ShapeReflection.CanWrite(target, e.PocoProperty ?? e.Element) + && !(e.DisplayValues is { Count: > 0 } && e.ValidValues is not { Count: > 0 }), + BareCalcChild => ShapeReflection.CanWrite(target, node.PocoProperty ?? "Calculation"), + NamedCalcChild nc => ShapeReflection.CanWrite(target, nc.PocoProperty ?? nc.Element), + NamedTextChild nt => ShapeReflection.CanWrite(target, nt.PocoProperty ?? nt.Element), + FieldChild f => ShapeReflection.CanWrite(target, f.PocoProperty ?? f.Element), + NamedRefChild nr => ShapeReflection.CanWrite(target, nr.PocoProperty ?? nr.Element), + _ => false, + }; + private static string? Assign(ScriptStep target, StepMetadata meta, ShapeNode node, string name, string value) { + if (!IsTypedAssignable(target, node)) + return ApplyViaDisplayGrammar(target, meta, node, name, value); + switch (node) { case BoolStateChild b: @@ -79,15 +122,96 @@ internal static class StepParamApplier ShapeReflection.Set(target, nr.PocoProperty ?? nr.Element, new NamedRef(0, value)); return null; - // HrOnly slots carry no XML binding of their own, and variant / - // value-type / list slots have no generic text form. A step that - // supports text values for these overrides ScriptStep.ApplyParam. default: - return $"Param '{name}' of step '{meta.Name}' cannot be set from a text value; " + - "edit the step XML instead."; + return ApplyViaDisplayGrammar(target, meta, node, name, value); + } + } + + /// + /// Routes a value through the step's own display-token grammar. The + /// synthesized token is validated against a blank probe first: it must + /// produce a wire-level change there, otherwise the grammar did not + /// recognize it and re-parsing could silently reset state. The accepted + /// token is then re-parsed onto the live instance together with the + /// step's current display tokens — those re-assert the display-visible + /// state, and properties display text does not carry stay untouched. + /// Current tokens carrying the same label are dropped so the new token + /// wins under both first-match and last-match parser styles; raw tokens + /// are appended, which the raw-form parsers resolve last-wins. + /// + private static string? ApplyViaDisplayGrammar(ScriptStep target, StepMetadata meta, ShapeNode node, string name, string value) + { + var blank = StepDisplayFactory.TryCreate(meta.Name, true, []); + if (blank is null) + return $"No typed POCO factory registered for '{meta.Name}'."; + var blankXml = blank.ToXml().ToString(); + + foreach (var (token, labelPrefix) in CandidateTokens(target, node, value)) + { + var probe = StepDisplayFactory.TryCreate(meta.Name, true, [token]); + if (probe is null || probe.ToXml().ToString() == blankXml) continue; + + IEnumerable retained = ScriptLineParser.ParseLine(target.ToDisplayLine()).Params; + if (labelPrefix is not null) + retained = retained.Where(t => !t.TrimStart().StartsWith(labelPrefix, StringComparison.OrdinalIgnoreCase)); + var current = retained.ToList(); + + // The value is already in the display line — nothing to change. + if (current.Any(t => t.Trim().Equals(token, StringComparison.OrdinalIgnoreCase))) + return null; + + // Parsers differ in whether the first or the last matching token + // wins, so try the new token in both positions and require a + // wire-level change. A no-change re-parse only re-assigns the + // display-visible state to itself, so the failed order is + // harmless. + var before = target.ToXml().ToString(); + + target.PopulateFromDisplay([.. current, token]); + if (target.ToXml().ToString() != before) return null; + + target.PopulateFromDisplay([token, .. current]); + if (target.ToXml().ToString() != before) return null; } + + return $"Param '{name}' of step '{meta.Name}' did not accept value '{value}'; " + + "it may match the step's default. Edit the step XML for exact control."; } + // A raw (unlabeled) token is only meaningful to a hand-written display + // parser, which recognizes tokens by form; the shape-driven parser would + // bind it to the first unused positional slot — the wrong one. For a + // slot with no HrLabel the raw form is canonical and goes first (a + // free-text grammar would swallow a name-prefixed token verbatim); the + // name-prefixed form is the rescue for grammars keyed on the slot name + // (e.g. Show Custom Dialog's "Buttons:"). + private static IEnumerable<(string Token, string? LabelPrefix)> CandidateTokens(ScriptStep target, ShapeNode node, string value) + { + var handParser = HasHandWrittenDisplayParser(target.GetType()); + var label = node.HrLabel ?? ShapeHrView.NameOf(node); + + if (node.HrLabel is null && handParser) + yield return (value, null); + + if (label.Length > 0) + yield return ($"{label}: {value}", $"{label}:"); + + if (node.HrLabel is not null && handParser) + yield return (value, null); + } + + private static readonly ConcurrentDictionary _handDisplayParser = new(); + + /// + /// True when the step overrides + /// itself rather than inheriting the shape-driven default declared on the + /// generic ScriptStep<TSelf> base. + /// + private static bool HasHandWrittenDisplayParser(Type stepType) => + _handDisplayParser.GetOrAdd(stepType, t => + t.GetMethod(nameof(ScriptStep.PopulateFromDisplay), BindingFlags.NonPublic | BindingFlags.Instance) + ?.DeclaringType?.IsGenericType == false); + private static string OnOffError(StepMetadata meta, string name, string value) => $"Param '{name}' of step '{meta.Name}' must be 'On' or 'Off' (got '{value}')."; diff --git a/src/SharpFM.Model/Scripting/Steps/GoToLayoutStep.cs b/src/SharpFM.Model/Scripting/Steps/GoToLayoutStep.cs index 275db1e7..263199d3 100644 --- a/src/SharpFM.Model/Scripting/Steps/GoToLayoutStep.cs +++ b/src/SharpFM.Model/Scripting/Steps/GoToLayoutStep.cs @@ -140,9 +140,27 @@ protected internal override void PopulateFromDisplay(string[] hrParams) if (!string.IsNullOrEmpty(wire)) animation = new Animation(wire); } - else if (TryParseTargetToken(token, out var parsed)) + else if (token.Equals("original layout", StringComparison.OrdinalIgnoreCase)) { - target = parsed; + target = new LayoutTarget.Original(); + } + else if (token.StartsWith("Layout Name:", StringComparison.OrdinalIgnoreCase)) + { + var expr = token.Substring("Layout Name:".Length).Trim(); + target = new LayoutTarget.ByNameCalc(new Calculation(expr)); + } + else if (token.StartsWith("Layout Number:", StringComparison.OrdinalIgnoreCase)) + { + var expr = token.Substring("Layout Number:".Length).Trim(); + target = new LayoutTarget.ByNumberCalc(new Calculation(expr)); + } + // Named layout with (#id) suffix is the lossless form. Bare + // quoted names without an id degrade to a NamedRef with id 0 — + // the user edited the display text and dropped the id, there's + // nothing better we can do. + else if (DisplayQuoting.TryParseNamedRef(token, out var namedRef)) + { + target = new LayoutTarget.Named(namedRef); } } @@ -150,72 +168,6 @@ protected internal override void PopulateFromDisplay(string[] hrParams) Animation = animation; } - /// - /// Recognizes the four display forms of a layout target: - /// original layout, a quoted name with optional (#id) - /// suffix, Layout Name: <calc>, and - /// Layout Number: <calc>. - /// - private static bool TryParseTargetToken(string token, out LayoutTarget target) - { - if (token.Equals("original layout", StringComparison.OrdinalIgnoreCase)) - { - target = new LayoutTarget.Original(); - return true; - } - - if (token.StartsWith("Layout Name:", StringComparison.OrdinalIgnoreCase)) - { - var expr = token.Substring("Layout Name:".Length).Trim(); - target = new LayoutTarget.ByNameCalc(new Calculation(expr)); - return true; - } - - if (token.StartsWith("Layout Number:", StringComparison.OrdinalIgnoreCase)) - { - var expr = token.Substring("Layout Number:".Length).Trim(); - target = new LayoutTarget.ByNumberCalc(new Calculation(expr)); - return true; - } - - // Named layout with (#id) suffix is the lossless form. Bare quoted - // names without an id degrade to a NamedRef with id 0 — the caller - // didn't supply an id, there's nothing better we can do. - if (DisplayQuoting.TryParseNamedRef(token, out var namedRef)) - { - target = new LayoutTarget.Named(namedRef); - return true; - } - - target = new LayoutTarget.Original(); - return false; - } - - // Hand-written: the target is a discriminated union selected by the - // value's display form, which the shape applier cannot convert. - protected internal override string? ApplyParam(string name, string value) - { - if (name.Equals("Layout", StringComparison.OrdinalIgnoreCase) - || name.Equals("LayoutDestination", StringComparison.OrdinalIgnoreCase) - || name.Equals("Target", StringComparison.OrdinalIgnoreCase)) - { - if (!TryParseTargetToken(value.Trim(), out var target)) - return $"Param '{name}' of step '{XmlName}' must be 'original layout', a quoted " + - "layout name (optionally with a (#id) suffix), 'Layout Name: ', or " + - $"'Layout Number: '. Got '{value}'."; - Target = target; - return null; - } - - if (name.Equals("Animation", StringComparison.OrdinalIgnoreCase)) - { - Animation = string.IsNullOrWhiteSpace(value) ? null : new Animation(value.Trim()); - return null; - } - - return base.ApplyParam(name, value); - } - public static StepMetadata Metadata { get; } = new() { Name = XmlName, diff --git a/src/SharpFM.Model/Scripting/Steps/ShowCustomDialogStep.cs b/src/SharpFM.Model/Scripting/Steps/ShowCustomDialogStep.cs index 1943c837..130084bc 100644 --- a/src/SharpFM.Model/Scripting/Steps/ShowCustomDialogStep.cs +++ b/src/SharpFM.Model/Scripting/Steps/ShowCustomDialogStep.cs @@ -160,26 +160,6 @@ private static string FormatInputSlot(ShowCustomDialogInputField input) return $"{labelText} {password} {target}"; } - // Hand-written: the button/input blocks use the nested display grammar - // ToDisplayLine renders, which the shape applier cannot convert. - protected internal override string? ApplyParam(string name, string value) - { - if (name.Equals("Buttons", StringComparison.OrdinalIgnoreCase)) - { - Buttons = ParseButtonBlock(value); - return null; - } - - if (name.Equals("InputFields", StringComparison.OrdinalIgnoreCase) - || name.Equals("Inputs", StringComparison.OrdinalIgnoreCase)) - { - InputFields = ParseInputBlock(value); - return null; - } - - return base.ApplyParam(name, value); - } - protected internal override void PopulateFromDisplay(string[] hrParams) { var title = new Calculation(""); @@ -408,7 +388,10 @@ private static (string CalcText, string Keyword) SplitTrailingKeyword(string slo new NamedCalcChild("DistanceFromLeft") { Optional = true, Display = DisplayMode.Hidden }, new Passthrough { PocoProperty = "ButtonsAndInputsWire" }, new HrOnly("Buttons"), - new HrOnly("InputFields"), + // HrLabel matches the display grammar's "Inputs:" prefix so + // label-addressed lookups resolve to the token form the parser + // actually recognizes. + new HrOnly("InputFields") { HrLabel = "Inputs" }, ], }; } diff --git a/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs b/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs index daffb894..eacb9b27 100644 --- a/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs +++ b/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs @@ -335,6 +335,150 @@ public void ApplyUpdate_ShowCustomDialog_ButtonsParam_ReplacesButtons() Assert.True(button.CommitState); } + [Fact] + public void ApplyUpdate_ShowCustomDialog_InputsParam_SetsInputFields() + { + var script = EmptyScript(); + script.Apply(new ScriptStepOperation( + Action: "add", + StepName: "Show Custom Dialog", + Params: new Dictionary { ["Title"] = "\"T\"" })); + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary { ["Inputs"] = "[ \"Age\" plain $age ]" }); + + Assert.Empty(script.Apply(update)); + + var step = Assert.IsType(script.Steps[0]); + Assert.Equal("\"T\"", step.Title.Text); + var input = Assert.Single(step.InputFields!); + Assert.Equal("Age", input.Label?.Text); + Assert.Equal("$age", input.Target.ToDisplayString()); + } + + [Fact] + public void ApplyAdd_PerformScript_ScriptParam_SetsByReferenceTarget() + { + var script = EmptyScript(); + + var op = new ScriptStepOperation( + Action: "add", + StepName: "Perform Script", + Params: new Dictionary { ["Script"] = "\"Subroutine\" (#12)" }); + + Assert.Empty(script.Apply(op)); + + var step = Assert.IsType(script.Steps.Single()); + var byRef = Assert.IsType(step.Target); + Assert.Equal("Subroutine", byRef.Script.Name); + Assert.Equal(12, byRef.Script.Id); + } + + [Fact] + public void ApplyAdd_PerformScript_ByNameCalcParam_SetsCalculationTarget() + { + var script = EmptyScript(); + + var op = new ScriptStepOperation( + Action: "add", + StepName: "Perform Script", + Params: new Dictionary { ["Target"] = "By name: Get ( ScriptName )" }); + + Assert.Empty(script.Apply(op)); + + var step = Assert.IsType(script.Steps.Single()); + var byCalc = Assert.IsType(step.Target); + Assert.Equal("Get ( ScriptName )", byCalc.NameCalc.Text); + } + + [Fact] + public void ApplyUpdate_PerformScript_ParameterParam_SetsAndPreservesTarget() + { + // The Parameter slot binds to an emit-only wire projection; the + // display-grammar route must reach the real Parameter property + // without disturbing the script target. + var script = EmptyScript(); + script.Apply(new ScriptStepOperation( + Action: "add", + StepName: "Perform Script", + Params: new Dictionary { ["Script"] = "\"Subroutine\" (#12)" })); + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary { ["Parameter"] = "42" }); + + Assert.Empty(script.Apply(update)); + + var step = Assert.IsType(script.Steps[0]); + Assert.Equal("42", step.Parameter?.Text); + var byRef = Assert.IsType(step.Target); + Assert.Equal("Subroutine", byRef.Script.Name); + Assert.Equal(12, byRef.Script.Id); + } + + [Fact] + public void ApplyUpdate_ConvertFile_PositionalEnumParam_SetsValue() + { + // Convert File's data source renders as an unlabeled positional token + // and its parser takes the first match, so the applier must place the + // new token ahead of the step's current display tokens. + var script = EmptyScript(); + script.Apply(new ScriptStepOperation(Action: "add", StepName: "Convert File")); + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary { ["DataSourceType"] = "XMLSource" }); + + Assert.Empty(script.Apply(update)); + + var step = Assert.IsType(script.Steps[0]); + Assert.Equal("XMLSource", step.DataSourceType); + } + + [Fact] + public void ApplyUpdate_SameValueAgain_SucceedsWithoutChange() + { + var script = EmptyScript(); + script.Apply(new ScriptStepOperation( + Action: "add", + StepName: "Go to Layout", + Params: new Dictionary { ["Layout"] = "\"Detail\" (#7)" })); + var xmlBefore = script.Steps[0].ToXml().ToString(); + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary { ["Layout"] = "\"Detail\" (#7)" }); + + Assert.Empty(script.Apply(update)); + Assert.Equal(xmlBefore, script.Steps[0].ToXml().ToString()); + } + + [Fact] + public void ApplyUpdate_ValueTheStepCannotExpress_ReturnsErrorWithoutChange() + { + // Dial Phone's use-dial-preferences flag has no addressable display + // token; the applier must reject it rather than write the raw display + // value into the wire-facing property. + var script = EmptyScript(); + script.Apply(new ScriptStepOperation(Action: "add", StepName: "Dial Phone")); + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary { ["UseDialPreferences"] = "On" }); + + var errors = script.Apply(update); + + Assert.NotEmpty(errors); + var step = Assert.IsType(script.Steps[0]); + Assert.False(step.UseDialPreferences); + } + [Fact] public void ApplyAdd_GoToLayout_NamedLayoutParam_SetsTarget() { From b5f8150291292f5b8c58935b4a7096ceb002e6b6 Mon Sep 17 00:00:00 2001 From: Nate Bross Date: Thu, 16 Jul 2026 19:01:23 -0500 Subject: [PATCH 3/3] feat(scripting): accept XML fragments as step param values --- .../Serialization/StepParamApplier.cs | 67 ++++++++++++++- .../Scripting/FmScriptApplyTests.cs | 84 +++++++++++++++++++ 2 files changed, 149 insertions(+), 2 deletions(-) diff --git a/src/SharpFM.Model/Scripting/Serialization/StepParamApplier.cs b/src/SharpFM.Model/Scripting/Serialization/StepParamApplier.cs index 3f001bd8..5da0b367 100644 --- a/src/SharpFM.Model/Scripting/Serialization/StepParamApplier.cs +++ b/src/SharpFM.Model/Scripting/Serialization/StepParamApplier.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Linq; using System.Reflection; +using System.Xml.Linq; using SharpFM.Model.Scripting.Registry; using SharpFM.Model.Scripting.Shapes; using SharpFM.Model.Scripting.Values; @@ -74,7 +75,11 @@ internal static class StepParamApplier private static string? Assign(ScriptStep target, StepMetadata meta, ShapeNode node, string name, string value) { if (!IsTypedAssignable(target, node)) - return ApplyViaDisplayGrammar(target, meta, node, name, value); + { + return value.TrimStart().StartsWith('<') + ? ApplyXmlFragment(target, meta, node, name, value) + : ApplyViaDisplayGrammar(target, meta, node, name, value); + } switch (node) { @@ -174,8 +179,66 @@ internal static class StepParamApplier if (target.ToXml().ToString() != before) return null; } + var elements = WireElementsOf(node); + var hint = elements.Count > 0 + ? $" This param also accepts a <{elements[0]}> XML fragment." + : " Edit the step XML for exact control."; return $"Param '{name}' of step '{meta.Name}' did not accept value '{value}'; " + - "it may match the step's default. Edit the step XML for exact control."; + $"it may match the step's default.{hint}"; + } + + /// + /// Grafts an XML fragment into the step's wire form: the fragment + /// replaces the step element's existing children of the same name and + /// the merged element is re-read in place through + /// . The fragment's root must be + /// one of the slot's wire elements (an slot names + /// its wire element), and the re-emitted step must retain it — a reader + /// that drops the element would silently discard the caller's data, so + /// the original state is restored and an error returned instead. + /// + private static string? ApplyXmlFragment(ScriptStep target, StepMetadata meta, ShapeNode node, string name, string value) + { + XElement fragment; + try + { + fragment = XElement.Parse(value); + } + catch (System.Xml.XmlException ex) + { + return $"Param '{name}' of step '{meta.Name}' looks like an XML fragment but does not parse: {ex.Message}"; + } + + var elements = WireElementsOf(node); + if (elements.Count == 0) + return $"Param '{name}' of step '{meta.Name}' has no wire element to set from an XML fragment; " + + "edit the full step XML instead."; + if (!elements.Contains(fragment.Name.LocalName, StringComparer.OrdinalIgnoreCase)) + return $"Param '{name}' of step '{meta.Name}' takes " + + $"{string.Join(" or ", elements.Select(e => $"<{e}>"))} as an XML fragment; got <{fragment.Name.LocalName}>."; + + var merged = target.ToXml(); + var before = merged.ToString(); + merged.Elements() + .Where(e => string.Equals(e.Name.LocalName, fragment.Name.LocalName, StringComparison.OrdinalIgnoreCase)) + .Remove(); + merged.Add(fragment); + target.PopulateFromXml(merged); + + var after = target.ToXml(); + if (after.Elements().Any(e => string.Equals(e.Name.LocalName, fragment.Name.LocalName, StringComparison.OrdinalIgnoreCase))) + return null; + + target.PopulateFromXml(XElement.Parse(before)); + return $"Step '{meta.Name}' did not retain the <{fragment.Name.LocalName}> element; " + + "edit the full step XML instead."; + } + + private static List WireElementsOf(ShapeNode node) + { + var elements = StepXmlValidator.ElementNamesOf(node).ToList(); + if (node is HrOnly h) elements.Add(h.Name); + return elements; } // A raw (unlabeled) token is only meaningful to a hand-written display diff --git a/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs b/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs index eacb9b27..81f1a0f2 100644 --- a/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs +++ b/tests/SharpFM.Tests/Scripting/FmScriptApplyTests.cs @@ -479,6 +479,90 @@ public void ApplyUpdate_ValueTheStepCannotExpress_ReturnsErrorWithoutChange() Assert.False(step.UseDialPreferences); } + [Fact] + public void ApplyUpdate_SortRecords_SortListXmlFragment_SetsSortOrder() + { + var script = EmptyScript(); + script.Apply(new ScriptStepOperation(Action: "add", StepName: "Sort Records")); + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary + { + ["Sort"] = """""", + }); + + Assert.Empty(script.Apply(update)); + + var step = Assert.IsType(script.Steps[0]); + Assert.NotNull(step.Sort); + Assert.Contains("PrimaryField", step.ToXml().ToString()); + } + + [Fact] + public void ApplyUpdate_PerformFind_QueryXmlFragment_ReplacesQuery() + { + // Perform Find's query lives in the passthrough bag behind an HrOnly + // slot; a second fragment must replace the first, not accumulate. + var script = EmptyScript(); + script.Apply(new ScriptStepOperation(Action: "add", StepName: "Perform Find")); + + var first = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary + { + ["Query"] = """==CA""", + }); + Assert.Empty(script.Apply(first)); + + var second = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary + { + ["Query"] = """==NY""", + }); + Assert.Empty(script.Apply(second)); + + var xml = script.Steps[0].ToXml().ToString(); + Assert.Contains("==NY", xml); + Assert.DoesNotContain("==CA", xml); + } + + [Fact] + public void ApplyUpdate_XmlFragmentWithWrongRootElement_ReturnsError() + { + var script = EmptyScript(); + script.Apply(new ScriptStepOperation(Action: "add", StepName: "Sort Records")); + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary { ["Sort"] = "" }); + + var errors = script.Apply(update); + + Assert.Contains(errors, e => e.Contains("SortList")); + } + + [Fact] + public void ApplyUpdate_MalformedXmlFragment_ReturnsError() + { + var script = EmptyScript(); + script.Apply(new ScriptStepOperation(Action: "add", StepName: "Sort Records")); + + var update = new ScriptStepOperation( + Action: "update", + Index: 0, + Params: new Dictionary { ["Sort"] = "" }); + + var errors = script.Apply(update); + + Assert.NotEmpty(errors); + } + [Fact] public void ApplyAdd_GoToLayout_NamedLayoutParam_SetsTarget() {