From c70e16cde8193591fb067a5fd2b843febe3cd38d Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Fri, 18 Sep 2026 16:44:00 +0300 Subject: [PATCH] feat: numbered "Reference video N" slot for widget-level references, plus a review chain of fixes The direct "ref:" picker for a widget-level reference (VHS_LoadVideo's "video", etc.) forced reselecting the field by hand every time the underlying reference file changed. Adds the same numbered indirection Input Sources already gives sockets: "Reference video 1" is whatever is marked first among video references, not a specific file, so re-marking in the Media Bin re-points the widget with no node-config change. Adversarial review of this addition, run to convergence, found and fixed in order: - a real installed file whose name collided with the sentinel pattern ("ref#video:1", "ref:whatever") could be silently hijacked instead of used as-is -- fixed by having a literal installed choice always win, identically in the builder and the picker; - the self-heal fallback for a stale plain value could reach into reference-slot space and silently attach a live Media Bin dependency the user never picked; - fixing that reopened the underlying onChange(undefined) hole the fallback chain had been papering over, wiping a stale plain value outright when nothing was installed -- restored the original choices-only self-heal invariant explicitly; - a reference-capable widget could be added to a Linked Inputs group, whose own value editor builds a spec with no ref_kind -- silently wiping the shared reference across every member on next render. Simplest safe fix: that kind of field doesn't link. Co-Authored-By: Claude Sonnet 5 --- movie_editor/backend/builder.py | 56 +++++++++++----- movie_editor/frontend/models.js | 103 +++++++++++++++++++++++------ movie_editor/tests/test_builder.py | 59 +++++++++++++++++ 3 files changed, 180 insertions(+), 38 deletions(-) diff --git a/movie_editor/backend/builder.py b/movie_editor/backend/builder.py index 04837e51..f13563f0 100644 --- a/movie_editor/backend/builder.py +++ b/movie_editor/backend/builder.py @@ -595,30 +595,54 @@ def build(object_info: dict, models_config: dict, params: dict, media: dict | No # 3a. widget-level references: a plain combo/upload field (VHS_LoadVideo's "video", # LoadImage's "image", ...) set in the node panel to a Media Bin reference, using the - # same "ref:" value Input sources uses for sockets — but landing straight in the - # widget instead of wiring one, since WIDGET_REFERENCE_FIELDS entries aren't sockets at - # all (see nodes.connection_inputs, which skips them). No "ref#:" numbered-slot - # form here: unlike a socket, nothing in the Editor ever offers or writes one for a - # widget, so there is nothing to resolve. + # same "ref:" / "ref#:" values Input sources uses for sockets — but landing + # straight in the widget instead of wiring one, since WIDGET_REFERENCE_FIELDS entries + # aren't sockets at all (see nodes.connection_inputs, which skips them). for s in slots: sid = slot_node_id[s["id"]] cls = s.get("node_class") + field_choices = _widget_choices(slot_def[s["id"]]) for wname, raw in (s.get("inputs") or {}).items(): - if not isinstance(raw, str) or not raw.startswith("ref:"): + if not isinstance(raw, str) or not (raw.startswith("ref:") or raw.startswith("ref#")): + continue + # A real installed file always wins over the sentinel reading of the same + # string — an unlikely but real collision (a file literally named "ref:x" or + # "ref#video:1") must not get silently swapped for whatever the media bin + # happens to have marked, just because its name looks like FunPack's own + # reference notation. + if raw in field_choices.get(wname, []): continue want_kind = WIDGET_REFERENCE_FIELDS.get((cls, wname)) if not want_kind: continue - ref = references.get(raw[4:]) - if not ref or not ref.get("filename"): - report["unsatisfied"].append( - f"{cls}.{wname}: reference media '{raw[4:]}' is no longer in the media bin.") - continue - ref_kind = ref.get("kind") or "image" - if ref_kind != want_kind: - report["unsatisfied"].append( - f"{cls}.{wname}: its reference is a {ref_kind}, but this field only takes a {want_kind}.") - continue + if raw.startswith("ref:"): + ref = references.get(raw[4:]) + if not ref or not ref.get("filename"): + report["unsatisfied"].append( + f"{cls}.{wname}: reference media '{raw[4:]}' is no longer in the media bin.") + continue + ref_kind = ref.get("kind") or "image" + if ref_kind != want_kind: + report["unsatisfied"].append( + f"{cls}.{wname}: its reference is a {ref_kind}, but this field only takes a {want_kind}.") + continue + else: + # "ref#:" — the Nth marked reference of that kind, whichever file + # that currently is. An empty slot is a normal, expected state here (nothing + # marked yet, or fewer than N marked) — same tone builder's socket-side + # "none marked, left unconnected" takes, not an error: the widget simply + # keeps whatever the node's own default already resolved it to above. + kind, _, num = raw[4:].partition(":") + if kind != want_kind or not num.isdigit(): + report["unsatisfied"].append( + f"{cls}.{wname}: '{raw}' is not a {want_kind} reference slot.") + continue + ref = _reference_by_slot(kind, int(num)) + if not ref or not ref.get("filename"): + report["wired"].append( + f"Reference {want_kind} {num} -> {sid}.{wname}: none marked, " + "left at the node's default") + continue graph[sid]["inputs"][wname] = ref["filename"] report["wired"].append(f"Reference {want_kind} -> {sid}.{wname}") diff --git a/movie_editor/frontend/models.js b/movie_editor/frontend/models.js index 6ab5c4c9..47e40fc6 100644 --- a/movie_editor/frontend/models.js +++ b/movie_editor/frontend/models.js @@ -614,6 +614,28 @@ return out; } + // Numbered reference SLOTS for a widget field — "Reference video 1" is whatever is + // marked first among the video references, not a particular file, same indirection + // referenceSlots() gives a connection socket. This is the one worth leaving set: pick it + // once and re-marking a different file as R1 in the Media Bin re-points it with no need + // to reopen the node and reselect (widgetRefOptions' direct-id picks require exactly + // that reselect every time the reference changes). Always offers at least "1" so the + // option exists before anything is marked yet. + function widgetRefSlotOptions(refKind) { + const st = window.Store?.get() || {}; + const marks = st.project?.references || []; + const bin = st.mediaBin || []; + const ofKind = window.References.referencesOfKind(marks, bin, refKind); + const top = Math.max(1, ofKind.length); + const out = []; + for (let n = 1; n <= top; n++) { + const m = ofKind[n - 1]; + out.push({ value: `ref#${refKind}:${n}`, + label: m ? `Reference ${refKind} ${n} · ${m.name}` : `Reference ${refKind} ${n}` }); + } + return out; + } + function widgetField(spec, value, onChange) { if (spec.kind === "list") return listField(spec, value, onChange); const wrap = el("label", "field"); @@ -621,9 +643,15 @@ let ctrl; if (spec.kind === "combo") { ctrl = el("select"); + const slotOpts = spec.ref_kind ? widgetRefSlotOptions(spec.ref_kind) : []; const refOpts = spec.ref_kind ? widgetRefOptions(spec.ref_kind) : []; + if (slotOpts.length) { + const og = el("optgroup"); og.label = "Reference slot (auto-follows the mark)"; + slotOpts.forEach((o) => { const opt = el("option", null, o.label); opt.value = o.value; if (o.value === value) opt.selected = true; og.append(opt); }); + ctrl.append(og); + } if (refOpts.length) { - const og = el("optgroup"); og.label = `Media Bin reference (${spec.ref_kind})`; + const og = el("optgroup"); og.label = `Specific reference (${spec.ref_kind})`; refOpts.forEach((o) => { const opt = el("option", null, o.label); opt.value = o.value; if (o.value === value) opt.selected = true; og.append(opt); }); ctrl.append(og); } @@ -637,34 +665,57 @@ if (refFilenames.has(c) && c !== value) return; const o = el("option", null, String(c)); o.value = c; if (c === value) o.selected = true; ctrl.append(o); }); - // Gated on spec.ref_kind, not just the "ref:" prefix: only the fields FunPack itself - // knows are reference-capable (WIDGET_REFERENCE_FIELDS, nodes.py) should ever be read - // as a sentinel. An ordinary combo can have a real installed file that happens to be - // named "ref:whatever" — treating that as a dangling reference would flag a working - // file as missing and, on the builder side, log a false "no longer in the media bin". - const isRef = !!spec.ref_kind && typeof value === "string" && value.startsWith("ref:"); - const refKnown = refOpts.some((o) => o.value === value); + // Gated on spec.ref_kind, not just the "ref:"/"ref#" prefix: only the fields FunPack + // itself knows are reference-capable (WIDGET_REFERENCE_FIELDS, nodes.py) should ever + // be read as a sentinel. An ordinary combo can have a real installed file that + // happens to be named "ref:whatever" (or, for the slot form, literally "ref#video:1") + // — treating that as a dangling reference would flag a working file as missing and, + // on the builder side, either log a false "no longer in the media bin" or, worse, + // silently swap in whatever's actually marked reference N instead of the file the + // combo says is selected. A real installed choice always wins over the sentinel + // reading of the same string — the builder applies the identical rule. + const isRef = !!spec.ref_kind && typeof value === "string" + && (value.startsWith("ref:") || value.startsWith("ref#")) + && !(spec.choices || []).includes(value); + const refKnown = refOpts.some((o) => o.value === value) || slotOpts.some((o) => o.value === value); if (isRef && !refKnown) { - // The reference this pointed at was unmarked or its file is gone. Unlike a stale - // plain filename, this is not "pick whatever's displayed" — silently swapping in - // spec.choices[0] here would persist a plain upload over a reference the user - // deliberately chose, the moment the panel next renders, with no report anywhere - // (the builder's unsatisfied check never sees it, since by build time the ref: - // value would already be gone). Surface it as missing instead, same treatment - // Input sources gives a dangling source (see the wire-select "(missing)" option - // below) — the builder's own "reference ... is no longer in the media bin" report - // is what actually declares this, not a silent default here. + // The reference this pointed at was unmarked or its file is gone (a "ref#kind:n" + // slot value is always in slotOpts — it offers at least "1" even unmarked — so + // this only fires for a direct "ref:" pick, or a slot number higher than what's + // currently marked). Unlike a stale plain filename, this is not "pick whatever's + // displayed": silently swapping in spec.choices[0] here would persist a plain + // upload over a reference the user deliberately chose, the moment the panel next + // renders, with no report anywhere (the builder's unsatisfied check never sees it, + // since by build time the ref: value would already be gone). Surface it as missing + // instead, same treatment Input sources gives a dangling source (see the + // wire-select "(missing)" option below) — the builder's own report is what + // actually declares this, not a silent default here. const o = el("option", null, "(reference missing)"); o.value = value; o.selected = true; ctrl.append(o); - } else if ((!spec.choices || !spec.choices.length) && !refOpts.length) { + } else if ((!spec.choices || !spec.choices.length) && !refOpts.length && !slotOpts.length) { ctrl.append(el("option", null, "(none installed)")); ctrl.disabled = true; } // A saved value that no longer exists (renamed/removed file) leaves nothing // selected — the browser then shows the first option without persisting it, so the // stale value silently goes to generation. Persist what's displayed. Never applies - // to a reference value: handled above instead, without auto-persisting over it. - else if (!isRef && value != null && !spec.choices.includes(value) && !refKnown) - onChange(spec.choices[0] ?? refOpts[0]?.value); + // to a reference value: handled above instead, without auto-persisting over it. And + // never falls into slotOpts/refOpts as a substitute default: those are never empty + // once spec.ref_kind is set (widgetRefSlotOptions always offers at least "1"), so + // reaching for them here would silently convert an ordinary stale plain value into a + // live Media Bin dependency the user never picked, the moment the panel opens on a + // node with nothing currently installed (spec.choices empty) — exactly the silent + // substitution this whole feature exists to avoid, just introduced from the other + // direction. A plain value only ever self-heals among plain choices, and only when + // there IS one to heal to: spec.choices.length is checked explicitly rather than + // relying on spec.choices[0] being truthy, because "no choices installed" must leave + // the stale value untouched, not overwrite it with onChange(undefined) — the prior + // form of this guard depended on the sibling "(none installed)" branch above always + // catching the empty-choices case first, which stopped being true the moment that + // branch grew a "&& !slotOpts.length" clause (slotOpts is never empty once + // spec.ref_kind is set), silently reopening the exact bug this comment describes. + else if (!isRef && value != null && spec.choices && spec.choices.length + && !spec.choices.includes(value) && !refKnown) + onChange(spec.choices[0]); ctrl.onchange = () => onChange(ctrl.value); } else if (spec.kind === "boolean") { ctrl = el("input"); ctrl.type = "checkbox"; ctrl.checked = !!value; ctrl.style.width = "auto"; @@ -999,7 +1050,15 @@ f.classList.add("linked"); const ctrl = f.querySelector("input,select"); if (ctrl) ctrl.disabled = true; f.append(el("span", "link-tag", "🔗 " + lk.name)); - } else if (linkMode && !isList && linkPickKindOk(spec.kind, spec.choices)) { + // A reference-capable combo (spec.ref_kind — VHS_LoadVideo.video, LoadImage.image, + // ...) is never offered for linking: Linked Inputs shares one plain widget VALUE + // across members, snapshotted with no ref-sentinel handling on either side (the + // link editor's own value control builds a spec with no ref_kind, so a linked + // "ref:"/"ref#" value would render there as an ordinary stale combo value and get + // silently self-healed to a plain default the next time the Linked Inputs view + // renders — wiping the reference across the whole group, unreported). Simplest safe + // answer: this kind of field just doesn't link. + } else if (linkMode && !isList && !spec.ref_kind && linkPickKindOk(spec.kind, spec.choices)) { const chk = el("button", "eye-btn link-pick" + (linkSelHas(slot.id, spec.name) ? " on" : ""), "+"); chk.type = "button"; chk.title = "Add to link selection"; chk.onclick = (e) => { diff --git a/movie_editor/tests/test_builder.py b/movie_editor/tests/test_builder.py index 1ffbb79d..1c782f1b 100644 --- a/movie_editor/tests/test_builder.py +++ b/movie_editor/tests/test_builder.py @@ -1112,6 +1112,65 @@ def test_a_widget_reference_that_is_gone_reports_instead_of_silently_defaulting( for u in report["unsatisfied"]) +# ── numbered reference SLOTS on a widget ("Reference video 1") ──────────────── +# The point, same as the socket-side version: pick it once and it keeps following whichever +# file is currently marked first, instead of needing to be reselected every time the +# reference changes. + + +def test_a_widget_reference_slot_resolves_to_the_first_marked_video(): + models = {"slots": [ + {"id": "lv", "node_class": "VHS_LoadVideo", "inputs": {"video": "ref#video:1"}, "wires": {}}, + ]} + params = _ref_params( + {"id": "m1", "kind": "video", "name": "a.mp4", "filename": "funpack_movie_m1.mp4"}, + {"id": "m2", "kind": "video", "name": "b.mp4", "filename": "funpack_movie_m2.mp4"}) + graph, report = builder.build(VIDEO_OI, models, params) + assert graph["slot_lv"]["inputs"]["video"] == "funpack_movie_m1.mp4" + assert any("slot_lv.video" in w for w in report["wired"]) + + +def test_reordering_the_marks_repoints_a_widget_reference_slot_with_no_config_change(): + models = {"slots": [ + {"id": "lv", "node_class": "VHS_LoadVideo", "inputs": {"video": "ref#video:1"}, "wires": {}}, + ]} + first = {"id": "m1", "kind": "video", "name": "a.mp4", "filename": "funpack_movie_m1.mp4"} + second = {"id": "m2", "kind": "video", "name": "b.mp4", "filename": "funpack_movie_m2.mp4"} + g1, _ = builder.build(VIDEO_OI, models, _ref_params(first, second)) + g2, _ = builder.build(VIDEO_OI, models, _ref_params(second, first)) + assert g1["slot_lv"]["inputs"]["video"] == "funpack_movie_m1.mp4" + assert g2["slot_lv"]["inputs"]["video"] == "funpack_movie_m2.mp4" + + +def test_a_widget_reference_slot_with_nothing_marked_yet_is_quiet_not_an_error(): + """An empty reference slot is a normal state before anything is marked, same tone the + socket-side "none marked, left unconnected" case takes — not something to alarm over.""" + models = {"slots": [ + {"id": "lv", "node_class": "VHS_LoadVideo", "inputs": {"video": "ref#video:1"}, "wires": {}}, + ]} + graph, report = builder.build(VIDEO_OI, models, _ref_params()) + assert graph["slot_lv"]["inputs"]["video"] == "a.mp4" # the node's own default, untouched + assert not any("VHS_LoadVideo.video" in u for u in report["unsatisfied"]) + assert any("slot_lv.video" in w and "none marked" in w for w in report["wired"]) + + +def test_a_real_installed_file_named_like_a_reference_sentinel_is_not_hijacked(): + """A file that happens to be named "ref#video:1" (or "ref:whatever") is a legitimate + installed choice, not FunPack's own notation -- picking it must not get silently + swapped for whatever the media bin happens to have marked reference video 1.""" + oi = dict(VIDEO_OI) + oi["VHS_LoadVideo"] = {"input": {"required": {"video": [["ref#video:1"]]}}, "output": ["IMAGE"], + "output_name": ["IMAGE"]} + models = {"slots": [ + {"id": "lv", "node_class": "VHS_LoadVideo", "inputs": {"video": "ref#video:1"}, "wires": {}}, + ]} + params = _ref_params({"id": "m1", "kind": "video", "name": "unrelated.mp4", + "filename": "funpack_movie_m1.mp4"}) + graph, report = builder.build(oi, models, params) + assert graph["slot_lv"]["inputs"]["video"] == "ref#video:1" + assert not any("slot_lv.video" in w for w in report["wired"]) + + def test_a_plain_widget_value_that_is_not_a_reference_is_left_alone(): models = {"slots": [ {"id": "lv", "node_class": "VHS_LoadVideo", "inputs": {"video": "a.mp4"}, "wires": {}},