From 7be09197d401e941431a4a44db1d2469d65eb5b9 Mon Sep 17 00:00:00 2001 From: UX-13 P2 review Date: Mon, 28 Sep 2026 03:43:47 +0200 Subject: [PATCH] UX-13 P2 review fixes (PR #145, comment 1880) 1. Discard reloads /config/model: after a stale save the page kept the new fingerprint with the old values, so Discard + Save overwrote the other surface's change without a 409. 2. Removing cards back to one restores the stored inverter shape, so a flat site stays flat after add + remove. 3. A choice the page can only keep ("Other time windows") stays offered while it is the stored value. 4. A load locks every locked value again. 5. at_zero() needs an accepted 0 W write; before the first one Home says "Going to 0 W" (the heartbeat writes every heartbeat_s, so it clears). 6. A field with a unit drops the translation's "(W)" from its label: the peak limit read "Peak limit (W)" beside a kW value. 7. A mirrored choice lists the mirrored field's keys, so a search for static_profile finds the contract on a fixed-price site. 8. test_settings: a save refused by ui_to_options writes nothing. Co-Authored-By: Claude Opus 5.5 --- goodwe_controller/app/homeview.py | 10 +++++--- goodwe_controller/app/static/settings.js | 22 +++++++++++------ goodwe_controller/app/uipage.py | 7 +++++- goodwe_controller/test_home.py | 9 ++++++- goodwe_controller/test_settings.py | 31 +++++++++++++++++++++--- goodwe_controller/tools/ux13_demo.py | 2 ++ 6 files changed, 64 insertions(+), 17 deletions(-) diff --git a/goodwe_controller/app/homeview.py b/goodwe_controller/app/homeview.py index 250720d..c6c62c6 100644 --- a/goodwe_controller/app/homeview.py +++ b/goodwe_controller/app/homeview.py @@ -511,10 +511,12 @@ def build_plan(c, now, shadow, running, held=False, settled=True): # Batteries, attention, month peak, control # --------------------------------------------------------------------------- # def at_zero(inv) -> bool: - """Has this inverter's last accepted power request reached 0 W? A stop or - an alarm asks for 0 W at once, but the inverter gets it with the next - write, so until then the page says "Going to 0 W" (UX-13 P1 test, a).""" - return abs(inv.standing_w()) < 1.0 + """Has this inverter accepted a 0 W power request? A stop or an alarm asks + for 0 W at once, but the inverter gets it with the next write, so until + then the page says "Going to 0 W" (UX-13 P1 test, a). Before the first + accepted write nothing is known to be held; the heartbeat writes every + `heartbeat_s`, stopped or not, so this does not stick.""" + return inv.last_write is not None and abs(inv.last_write) < 1.0 def battery_rows(c, shadow, running, held=False): diff --git a/goodwe_controller/app/static/settings.js b/goodwe_controller/app/static/settings.js index 074ad3e..18273be 100644 --- a/goodwe_controller/app/static/settings.js +++ b/goodwe_controller/app/static/settings.js @@ -87,8 +87,10 @@ } /* ---------------- controls ---------------- */ - function choicesOf(f, cur) { - const out = (f.choices || []).filter((c) => !c[2] || same(c[0], cur)); + // A choice the page can only keep stays offered while it is the stored value. + function choicesOf(f, card) { + const cur = get(f, card), was = valueIn(ORIG, f, card); + const out = (f.choices || []).filter((c) => !c[2] || same(c[0], cur) || same(c[0], was)); if (cur != null && cur !== "" && !out.some((c) => same(c[0], cur))) out.push([cur, String(cur)]); return out; } @@ -106,7 +108,7 @@ function radios(f, card, lid, cards) { const cur = get(f, card); const g = h("div", { class: cards ? "choices" : "seg", role: "radiogroup", "aria-labelledby": lid }); - const items = choicesOf(f, cur); + const items = choicesOf(f, card); const buttons = items.map(([v, label], i) => { const on = same(v, cur); const hint = cards && f.hints ? f.hints[v] : ""; @@ -132,7 +134,7 @@ function select(f, card, lid) { const cur = get(f, card); const s = h("select", { class: "sel", "aria-labelledby": lid }); - for (const [v, label] of choicesOf(f, cur)) s.append(h("option", { value: JSON.stringify(v), selected: same(v, cur) }, label)); + for (const [v, label] of choicesOf(f, card)) s.append(h("option", { value: JSON.stringify(v), selected: same(v, cur) }, label)); for (const [v, label] of f.later || []) s.append(h("option", { value: JSON.stringify(v), disabled: true }, label)); s.addEventListener("change", () => put(f, card, JSON.parse(s.value), true)); const preset = f.presets && f.presets[cur]; @@ -324,7 +326,10 @@ const box = h("div", { class: "inv" }, h("div", { class: "hd" }, h("span", { class: "grow" }, c.name || W.inverter + " " + (i + 1)), h("span", { class: "key" }, f.key + "[" + i + "]"), cards.length > 1 ? h("button", { type: "button", class: "btn link tiny", onclick: () => { - V[f.key].splice(i, 1); V[f.count] = V[f.key].length; put(f, null, V[f.key], true); + V[f.key].splice(i, 1); V[f.count] = V[f.key].length; + // Back to one card, the stored shape returns: a flat inverter stays flat. + if (V[f.key].length === 1) for (const k in f.grow) V[k] = clone(ORIG[k]); + put(f, null, V[f.key], true); } }, W.remove) : null)); for (const s of f.fields) box.append(fieldRow(s, i, gid)); wrap.append(box); @@ -496,6 +501,7 @@ return; } M = m; V = clone(m.ui); ORIG = clone(m.ui); FP = m.fingerprint; MSG = []; DEVICE = ""; + UNLOCKED.clear(); $("#set-search").hidden = false; FILLED = {}; LIST = null; @@ -541,9 +547,9 @@ finally { btn.disabled = false; btn.textContent = W.savebar.save; } } $("#sb-save").addEventListener("click", save); - $("#sb-discard").addEventListener("click", () => { - V = clone(ORIG); UNLOCKED.clear(); render(); validate(); toast(W.toast.discarded); - if ($("#set-banner .b-bad")) banner(); + // Discard reloads what is stored: after a stale save ORIG is no longer it. + $("#sb-discard").addEventListener("click", async () => { + await load(); toast(W.toast.discarded); }); addEventListener("beforeunload", (e) => { if (V && dirtyCount()) { e.preventDefault(); e.returnValue = ""; } }); document.addEventListener("ux13:tab", (e) => { diff --git a/goodwe_controller/app/uipage.py b/goodwe_controller/app/uipage.py index 9a84e4b..d82c4fa 100644 --- a/goodwe_controller/app/uipage.py +++ b/goodwe_controller/app/uipage.py @@ -91,6 +91,10 @@ def _choices(fld, node, T, value_type): def _field(fld, schema_ui, translations, lang, T): node = _node(schema_ui, fld.key) label, text = _words(translations, lang, fld.key, T) + if fld.unit: + # The page shows the unit beside the control, in its own scale (kW), + # so the translation's "(W)" goes; "(optional)" stays. + label = re.sub(r"\s*\([^()\s]{1,6}\)", "", label) value_type = ("float" if fld.control == L.NUMBER and fld.key.startswith("ui.") else node.get("type") or "") out = {"key": fld.key, "control": fld.control, "label": label, @@ -98,8 +102,9 @@ def _field(fld, schema_ui, translations, lang, T): "step": fld.step, "kind": fld.kind, "show": [list(c) for c in fld.show], "applies": fld.applies, "type": value_type, "optional": not node.get("required", False), + # A choice mirrored into a field also shows (and finds) its keys. "writes": sorted(k for k, how in L.DERIVED.items() - if how == fld.key)} + if how in (fld.key, fld.mirror))} if node.get("lengthMin") is not None: out["min"] = node["lengthMin"] if node.get("lengthMax") is not None: diff --git a/goodwe_controller/test_home.py b/goodwe_controller/test_home.py index 1adf98f..2a69591 100644 --- a/goodwe_controller/test_home.py +++ b/goodwe_controller/test_home.py @@ -123,11 +123,13 @@ MOCK = [.24, .22, .21, .20, .21, .22, .24, .27, .26, .22, .18, .16, .15, .14, def make(opts, *, soc=(62.0, 35.0), batt=(-2500.0, -300.0), grid=10.0, - auto=True): + auto=True, wrote=0.0): + """`wrote` is each inverter's last accepted power request (None: none yet).""" c = Controller(dict(opts), _Hass(), _Store(), _Pub(), stop=asyncio.Event()) c.auto = auto for i, inv in enumerate(c.inverters): inv.soc, inv.batt = soc[i], batt[i] + inv.last_write = wrote inv.dev_min, inv.dev_max = -5000.0, 5000.0 c.grid = grid return c @@ -471,6 +473,11 @@ check("an alarm while the last power request is still 20 W says 'Going to " c_sp = make(TWO, auto=False) c_sp.inverters[1].last_write = -1500.0 s_sp = status(c_sp) +s_new = status(make(TWO, auto=False, wrote=None)) +check("...and before any power request was accepted (just started, or every " + "write refused) it says 'Going to 0 W', not 'Held'", + [b["now"] for b in s_new["batteries"]] + == ["Going to 0 W while control is stopped."] * 2) check("...and a stop does the same, per battery", [b["now"] for b in s_sp["batteries"]] == ["Held at 0 W while control is stopped.", diff --git a/goodwe_controller/test_settings.py b/goodwe_controller/test_settings.py index 89455cb..5df6a2f 100644 --- a/goodwe_controller/test_settings.py +++ b/goodwe_controller/test_settings.py @@ -103,8 +103,12 @@ check(f"every slider and locked value has its range from the schema " f"({len(_ranged)})", all(lo is not None and hi is not None for _k, lo, hi in _ranged) and ("peak_cap_w", 500.0, 15000.0) in _ranged) -check("the peak limit is a kW slider over a value stored in W", - field(FIELD, "peak_cap_w")["scale"] == 1000) +check("the peak limit is a kW slider over a value stored in W, and its label " + "does not say W (the unit is shown beside the control)", + field(FIELD, "peak_cap_w")["scale"] == 1000 + and field(FIELD, "peak_cap_w")["label"] == "Peak limit" + and not [f["key"] for _g, f in _flat + if f["unit"] and re.search(r"\([^()\s]{1,6}\)", f["label"])]) _choice = [f for _g, f in _flat if f["control"] in (L.SEGMENTED, L.SELECT, L.CHOICE)] check("every choice control has choices with words", @@ -139,7 +143,8 @@ check("the inverter cards carry their count, and adding one makes a list", check("a page-only field names the option keys it writes, for the grey key " "line", _reg["writes"] == ["price_taxes_profile"] and field(FIELD, "ui.solar")["writes"] - == ["pv_entities", "pv_entity", "pv_invert_entities"]) + == ["pv_entities", "pv_entity", "pv_invert_entities"] + and "static_profile" in field(FIELD, "ui.contract")["writes"]) check("without a Supervisor schema the page still draws, with no ranges", len(uipage.layout([], {})["groups"]) == 6) @@ -183,6 +188,18 @@ check("a locked value shows no input until 'Change' is pressed", and "UNLOCKED.has(id)) { open(); return box; }" in JS) check("nothing is saved while the add-on's own checks refuse the model", "if (MSG.some((m) => m.tier === \"bad\")) { toast(" in JS) +check("Discard reloads the stored settings (after a stale save the page's " + "copy is no longer what is stored)", + re.search(r'"#sb-discard"\)\.addEventListener\("click", async \(\) => ' + r'\{\s*await load\(\);', JS) is not None) +check("a load locks every locked value again, and a choice the page can only " + "keep stays offered while it is the stored value", + "UNLOCKED.clear();" in JS.split("async function load")[1].split("render();")[0] + and "same(c[0], was)" in JS) +check("removing cards back to one restores the stored shape (a flat inverter " + "is not turned into a list of one)", + "if (V[f.key].length === 1) for (const k in f.grow) V[k] = clone(ORIG[k]);" + in JS) check("the old settings page stays reachable (until P5)", 'href="config" id="set-classic"' in web.page()) @@ -343,6 +360,11 @@ async def _refuse(s, root, sup): out["floor"] = await validate(peak_soc_floor=5) out["derive"] = await validate(ui__contract="dynamic", price_provider="") model = copy.deepcopy(base) + model.update({"ui.contract": "dynamic", "price_provider": ""}) + out["derive_save"] = await _post(s, root + "config/save", { + "model": model, "fingerprint": m["fingerprint"]}) + out["derive_writes"] = len(sup.writes) + model = copy.deepcopy(base) model["inverters"][1]["setpoint_entity"] = "" out["save"] = await _post(s, root + "config/save", {"model": model, "fingerprint": m["fingerprint"]}) @@ -374,6 +396,9 @@ check("a model no blob can say is refused beside its field, in plain words", _r["derive"]["messages"] == [{ "field": "price_provider", "card": None, "tier": "bad", "text": W["derive"]["no_source"]["description"]}]) +check("a save no blob can say answers 422 with its sentence and writes nothing", + _r["derive_save"][0] == 422 and _r["derive_writes"] == 0 + and _r["derive_save"][1]["messages"] == _r["derive"]["messages"]) check("a refused save answers 422 with the same placement and writes nothing", _r["save"][0] == 422 and ("inverters.setpoint_entity", 1) in _at(_r["save"][1]) and _sup.writes == []) diff --git a/goodwe_controller/tools/ux13_demo.py b/goodwe_controller/tools/ux13_demo.py index 028f4c3..a00ffca 100644 --- a/goodwe_controller/tools/ux13_demo.py +++ b/goodwe_controller/tools/ux13_demo.py @@ -230,6 +230,8 @@ def controller(state): for inv, soc, batt in zip(c.inverters, (62.0, 35.0), (-2500.0, -300.0)): inv.soc, inv.batt = soc, (0.0 if state == "shadow" else batt) inv.dev_min, inv.dev_max = -5000.0, 5000.0 + if state != "running": # 0 W accepted: Home says "Held" + inv.last_write = 0.0 c.grid = -10.0 if state != "shadow" else -2810.0 c.pv_w = 3900.0 if state == "alarm":