feat: numbered "Reference video N" slot for widget-level references, plus a review chain of fixes
The direct "ref:<id>" 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 <noreply@anthropic.com>
This commit is contained in:
digital-garbage
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
516660bbb1
commit
c70e16cde8
@@ -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:<id>" 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#<kind>:<n>" 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:<id>" / "ref#<kind>:<n>" 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#<kind>:<n>" — 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}")
|
||||
|
||||
|
||||
@@ -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:<id>" 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) => {
|
||||
|
||||
@@ -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": {}},
|
||||
|
||||
Reference in New Issue
Block a user