From e2b258cfd741939acd45d169c87144af74f0e8e5 Mon Sep 17 00:00:00 2001 From: Kayshen-X Date: Thu, 14 May 2026 18:44:04 +0800 Subject: [PATCH] fix(variables): end-push the new themed entry (no other-axis shadow) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third codex stop-gate on `set_color_hex` themed writes. The previous fix (599fc859) inserted the new entry at index 0 to guarantee precedence under the active theme, but that shadowed existing entries on UNRELATED axes: Existing: [{theme: Some({density: compact}), value: "#compact"}] Active: {mode: dark} Step 3 (no subset match) inserted at front: [{theme: Some({mode: dark}), value: "#new"}, {theme: Some({density: compact}), value: "#compact"}] Under future active = {mode: dark, density: compact}: resolve's first-match subset walk picked entry 0 ({mode: dark} ⊆ active) and returned "#new" — even though the user's edit was only meant to apply to mode = dark, not to override the density-specific entry under the combined axis. Switched to end-push. End-push is safe because Step 1 (subset match) already proved no existing entry is a subset of `active`, so the new entry is the UNIQUE subset match under the active theme regardless of position. Under OTHER actives, every pre-existing entry retains its original resolve precedence. Test (1 added, 290 shell-core total): - `set_color_hex_does_not_shadow_existing_entries_on_other_axes` builds the exact pathological case: existing {density: compact} entry; write under active {mode: dark}; then flips active to {mode: dark, density: compact} and asserts resolve_color still returns the compact value (#11ccaa), not the dark-only write (#000000). Pre-fix front-insert would have read #000000 — the codex BLOCK. The three-round set_color_hex saga is now closed: Round 1 (042a62c8): write path mirrors resolve's subset matching → no shadow by stale-vec ordering. Round 2 (599fc859): non-empty active never clobbers the theme=None default. Round 3 (this): end-push not front-insert → no shadow of other-axis themed entries. --- .../src/document/variables.rs | 96 +++++++++++++++---- 1 file changed, 80 insertions(+), 16 deletions(-) diff --git a/crates/openpencil-shell-core/src/document/variables.rs b/crates/openpencil-shell-core/src/document/variables.rs index 6221fb40c..62ccc3e75 100644 --- a/crates/openpencil-shell-core/src/document/variables.rs +++ b/crates/openpencil-shell-core/src/document/variables.rs @@ -257,23 +257,26 @@ impl VariableTable { } return true; } - // Active theme set + no subset match — push a new - // entry keyed to the active theme. The default entry - // (if any) stays untouched so other axes keep their - // original values. + // Active theme set + no subset match — append a + // new entry keyed to the active theme at the END + // of the vec (codex stop-gate: front insertion + // shadowed pre-existing themed entries on OTHER + // axes — e.g. an existing {density:compact} entry + // would never resolve under active=dark+compact if + // a {mode:dark} entry was front-inserted because + // resolve's first-match walk picks the front entry + // for ANY active whose keys are a superset). // - // Insert at the FRONT so the resolve walk reaches - // the new entry before any pre-existing themed entry - // whose theme is a superset of ours (resolve uses - // subset matching against active, so a more-specific - // entry placed first wins under the active theme). - entries.insert( - 0, - ThemedValue { - value: VariableScalar::Str(normalized), - theme: Some(active), - }, - ); + // End-push is safe because Step 1 (subset match) + // already proved no existing entry is a subset of + // `active`. So under the active theme our new + // entry is the unique subset match regardless of + // position; under OTHER actives, every pre-existing + // entry retains its original resolve precedence. + entries.push(ThemedValue { + value: VariableScalar::Str(normalized), + theme: Some(active), + }); true } } @@ -710,6 +713,67 @@ mod tests { ); } + #[test] + fn set_color_hex_does_not_shadow_existing_entries_on_other_axes() { + // Codex stop-gate (round 3): when the write pushes a new + // themed entry, it must NOT shadow pre-existing entries + // whose theme references a different axis. The pathological + // case is an existing entry keyed to `{density:compact}` + // and a write under `{mode:dark}`. A naive front-insert + // would mean active=`{mode:dark, density:compact}` resolves + // to the new entry instead of the compact entry, silently + // changing the resolved colour for the combined axis. + let mut tbl = super::VariableTable::default(); + tbl.variables.push(Variable { + name: "bg".into(), + kind: VariableKind::Color, + value: VariableValue::Themed(vec![ThemedValue { + value: VariableScalar::Str("#11ccaa".into()), + theme: Some({ + let mut m = BTreeMap::new(); + m.insert("density".into(), "compact".into()); + m + }), + }]), + }); + // Active theme = mode:dark only (no compact axis set, so the + // existing entry doesn't subset-match — the write must push + // a new entry). + tbl.set_active_theme("mode", "dark"); + assert!(tbl.set_color_hex("bg", "#000000")); + // Under mode:dark — the new entry is reachable. + assert_eq!( + tbl.resolve_color("bg"), + Some(crate::Color { + r: 0.0, + g: 0.0, + b: 0.0, + a: 1.0, + }) + ); + // Now flip active to the combined axes — mode:dark + + // density:compact. Under that resolve walk: + // - The pre-existing {density:compact} entry must still + // win (it's earlier in the vec AND its theme is a + // subset of the active combo). + // - The new {mode:dark} entry, end-pushed, is also a + // subset of the active combo, but it's later. First + // subset match wins. + // Pre-fix (front-insert) this would have read #000000 + // instead — that was the codex BLOCK. + tbl.set_active_theme("density", "compact"); + assert_eq!( + tbl.resolve_color("bg"), + Some(crate::Color { + r: 0x11 as f32 / 255.0, + g: 0xcc as f32 / 255.0, + b: 0xaa as f32 / 255.0, + a: 1.0, + }), + "compact entry must not be shadowed by the end-pushed dark entry" + ); + } + #[test] fn set_color_hex_empty_active_theme_targets_default_entry() { // The flip side: when active_theme IS empty (no axis