From 2819ee5a49fbfcfd8c240966db86a63d21f088db Mon Sep 17 00:00:00 2001 From: James Mackay Date: Mon, 12 Oct 2026 12:36:15 +0100 Subject: [PATCH] fix(media_player,config_flow): close the batch review's O2 and O3 (t17) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both observations from the t8-t12 batch review, settled by execution rather than by argument. O2 — WHICH STATES MAY CARRY A NOW-PLAYING ROW. The review read `extra_state_attributes` and argued the row publishes the entity state verbatim, so `now_playing_state: on` is reachable, while the README documents only playing/paused/buffering/idle; t10's author had reported that `on` and `off` publish no row at all. The probe settles both halves: * the t10 account is true on the NATURAL ladder — `on` means no source reported a power or playback fact, and every tier that can produce a reading sets `reported = True` in the ladder, so the two cannot meet; * the review is right about the RULE. It was "publish unless the state is off", an assumption nothing enforced: forcing the ladder to `on` (and to `standby`, which the README does not even mention) published `now_playing_state: on`/`standby` beside a real title. So the rule is now explicit — `NOW_PLAYING_STATE_VALUES = ("playing", "paused", "buffering", "idle")` — and a reading is dropped whole when the entity state is anything else, exactly as `off` always dropped it. Nothing legitimate is lost: a tier only ever answers in one of those four (`idle` for "reported, nothing playing", which is where the foreground tier's remembered-playback bridge lands). The README says the same thing in the table row and in prose, and tests/test_now_playing_publication.py walks EVERY `MediaPlayerState` member with a reading present (the four publish themselves; on/off/standby publish nothing), then reads the four values back OUT OF THE README and compares them to the constant, so code and docs cannot drift again. O3 — A REJECTED APP PASTE NAMES ITS BAD LINES. The bulk paste was all-or-nothing and reported a generic `invalid_app_map`, so a 30-row paste with one typo had to be bisected by hand. `_app_map_parse` now returns `(map | None, problems)` where each problem carries the 1-based line NUMBER, the line as the user typed it, and why (empty/space-bearing package, empty name, space in the icon), and the rejection puts them in the form's `description_placeholders["invalid_lines"]`, which the error copy interpolates — capped at 4 named lines with the remaining count, so the message stays readable. The map is still None when anything is wrong (never half-stored) and the detail is cleared as soon as a paste parses. The single-row editor is untouched and keeps its own `invalid_app_row`; a test asserts the new per-line report and `_valid_app_row` accept and refuse the same rows, so a paste and a typed row cannot disagree. Verify: 794 passed / 4 skipped, 795 passed / 3 skipped (with androidtvremote2), ruff clean. --- README.md | 9 +- .../nvidia_shield/config_flow.py | 158 +++++++++++++--- .../nvidia_shield/media_player.py | 22 +++ custom_components/nvidia_shield/strings.json | 2 +- .../nvidia_shield/translations/en.json | 2 +- tests/test_now_playing_publication.py | 175 ++++++++++++++++++ tests/test_standalone_setup.py | 154 +++++++++++++++ 7 files changed, 489 insertions(+), 33 deletions(-) create mode 100644 tests/test_now_playing_publication.py diff --git a/README.md b/README.md index 27fa24c..f23f4ff 100644 --- a/README.md +++ b/README.md @@ -876,7 +876,7 @@ TOP-LEVEL attributes**, not one nested object — that object read badly in YAML | Attribute | What it holds | | --- | --- | | `now_playing_via` | which TIER produced the reading: `jellyfin`, `media_session` or `app` | -| `now_playing_state` | the reading's own state: `playing` / `paused` / `buffering` / `idle` | +| `now_playing_state` | the reading's own state — exactly one of `playing`, `paused`, `buffering`, `idle` | | `now_playing_title` | the title — or, in privacy mode, the app identity | | `now_playing_artist`, `now_playing_album` | the session's artist and album, when it has them | | `now_playing_media_type` | the content type the source reported | @@ -893,6 +893,13 @@ The rules that keep the set predictable: gone stale must not put a title beside a device that is off, and the state already says nothing is playing. `on` (no source reported a power or playback fact) publishes none either — there is no reading to publish. +- **`now_playing_state` is one of FOUR values, and `on` is not one of them.** The + full set is `playing`, `paused`, `buffering`, `idle` — the states a source can + report when it has something to say. The row is published only while the entity + state is one of those four, so `on` (nothing reported), `off` and any frontend- + or app-side state can never appear beside a title. The list is code + (`NOW_PLAYING_STATE_VALUES` in `media_player.py`) and a test compares THIS + paragraph and the table above against it, so neither can drift. - **`now_playing_via` is deliberately not `source`.** On a media player, `source` and `source_list` mean the selected app input (`Netflix`, `TiviMate`, …) and they still do; naming the tier `source` is exactly what made the reading look like diff --git a/custom_components/nvidia_shield/config_flow.py b/custom_components/nvidia_shield/config_flow.py index 5964288..3fa4316 100644 --- a/custom_components/nvidia_shield/config_flow.py +++ b/custom_components/nvidia_shield/config_flow.py @@ -117,6 +117,90 @@ def _app_map_to_text(app_map: dict, icon_map: dict | None = None) -> str: return "\n".join(lines) +def _app_row_problems(package: str, name: str, icon: str) -> list[str]: + """Why ONE ``package=name[|icon]`` row is invalid (t17/O3, empty when it is fine). + + The reasons are separated because the fix differs: a package id with a space + is a typo in the FIRST column, an empty name is a missing SECOND column, and a + space in the icon column is a bad picture reference. The row editor already + reports them apart for a single row; naming them apart here is what lets a + rejected PASTE say which column of which line to look at. The rule itself is + :func:`_valid_app_row`'s, said out loud. + """ + problems: list[str] = [] + if not package or " " in package: + problems.append("the package id before the '=' is empty or contains a space") + if not name: + problems.append("the name after the '=' is empty") + if icon and " " in icon: + problems.append( + "the icon after the '|' contains a space — use a picture URL or a " + "camera entity id" + ) + return problems + + +def _app_map_parse( + text: str, icons: dict[str, str] | None = None +) -> tuple[dict[str, str] | None, list[tuple[int, str, list[str]]]]: + """Parse the apps table, reporting EVERY bad line instead of only refusing (t17/O3). + + Returns ``(app_map, problems)``: the map is None when anything is wrong (never + half-stored, the standing rule), and ``problems`` names each offending line by + its 1-based line NUMBER in the submitted text, the line as the user wrote it, + and why. That is the point of the split: a 30-row paste with one typo used to + be rejected with a generic message, so the user had to bisect the paste by hand + to find the row at fault. + + A blank line is not a row and never a problem. ``icons`` is written only for + rows that passed, and a rejected parse returns no map at all, so a partial + write is not possible. + """ + icons = icons if icons is not None else {} + app_map: dict[str, str] = {} + problems: list[tuple[int, str, list[str]]] = [] + for number, raw in enumerate(text.split("\n"), start=1): + line = raw.strip() + if not line: + continue + if "=" not in line: + problems.append((number, line, ["no '=' — a row is package=name"])) + continue + package, value = line.split("=", 1) + parts = value.strip().split("|", 1) + name = parts[0].strip() + icon = parts[1].strip() if len(parts) > 1 else "" + package = package.strip() + row_problems = _app_row_problems(package, name, icon) + if row_problems: + problems.append((number, line, row_problems)) + continue + if icon: + icons[package] = icon + app_map[package] = name + if problems: + return None, problems + return app_map, [] + + +#: How many offending lines a rejection message names before it stops listing them. +#: Bounded on purpose: the message has to be readable in a form, and a paste made +#: entirely of junk does not need 300 lines quoted back — the total is always given. +APP_MAP_PROBLEM_LIMIT = 4 + + +def _app_map_problem_text(problems) -> str: + """The rejection body: the numbered bad lines, capped, with the total (t17/O3).""" + shown = problems[:APP_MAP_PROBLEM_LIMIT] + parts = [ + f"line {number}: “{line}” — {'; '.join(reasons)}" + for number, line, reasons in shown + ] + if len(problems) > len(shown): + parts.append(f"…and {len(problems) - len(shown)} more line(s)") + return "; ".join(parts) + + def _app_map_from_text( text: str, icons: dict[str, str] | None = None ) -> dict[str, str] | None: @@ -126,27 +210,13 @@ def _app_map_from_text( entity_picture through CONF_APP_ICON_MAP; single-column maps stay valid. Malformed icon text (spaces) invalidates the row the same way a malformed name does. + + A thin wrapper over :func:`_app_map_parse` for the callers that only need the + map; the rejection REPORT is read through that function, so the two cannot + disagree about which rows are valid. """ - icons = icons if icons is not None else {} - app_map: dict[str, str] = {} - for line in text.split("\n"): - line = line.strip() - if not line: - continue - if "=" not in line: - return None - package, value = line.split("=", 1) - parts = value.strip().split("|", 1) - name = parts[0].strip() - icon = parts[1].strip() if len(parts) > 1 else "" - package = package.strip() - name = name.strip() - if not _valid_app_row(package, name, icon): - return None - if icon: - icons[package] = icon - app_map[package] = name - return app_map + app_map, problems = _app_map_parse(text, icons) + return None if problems else app_map def _apps_table_text( @@ -222,17 +292,28 @@ def _app_map_submission(value) -> tuple[dict, dict] | None: A blank field is the documented way to clear the map; anything else must parse as ``package=name[|icon]`` lines or the whole submission is rejected - (never half-stored). + (never half-stored). Kept for the callers that only need the map — the + rejection detail comes from :func:`_app_map_submission_report`. """ - if value is None: - return {}, {} - if not str(value).strip(): - return {}, {} + parsed, _problems = _app_map_submission_report(value) + return parsed + + +def _app_map_submission_report( + value, +) -> tuple[tuple[dict, dict] | None, str]: + """Parse the field AND describe every bad line (t17/O3). + + Returns ``((app_map, icon_map) | None, problem_text)``. ``problem_text`` is + empty for a value that parses (including a blank one, which clears the map). + """ + if value is None or not str(value).strip(): + return ({}, {}), "" icons: dict[str, str] = {} - app_map = _app_map_from_text(str(value), icons) + app_map, problems = _app_map_parse(str(value), icons) if app_map is None: - return None - return app_map, icons + return None, _app_map_problem_text(problems) + return (app_map, icons), "" def _host_field_key(field: str, host: str): @@ -1237,6 +1318,10 @@ class NvidiaShieldOptionsFlow(config_entries.OptionsFlowWithReload): self._discover_note: str | None = None self._editing_package: str | None = None self._removing_package: str | None = None + # t17/O3: the apps-text rejection's DETAIL (which lines were bad, and + # why). Flow state like the note above: it shapes the next render and + # never becomes an option. + self._app_map_problems: str = "" # t15: the throwaway pairing client of a protocol_pair visit (see # async_step_protocol_pair). Flow state, never an option. self._protocol_client = None @@ -1402,13 +1487,20 @@ class NvidiaShieldOptionsFlow(config_entries.OptionsFlowWithReload): # ── the apps table: parse or reject, never half-store ── if CONF_APP_NAME_MAP in present: - parsed = _app_map_submission(present[CONF_APP_NAME_MAP]) + parsed, problems = _app_map_submission_report(present[CONF_APP_NAME_MAP]) if parsed is None: errors["base"] = "invalid_app_map" + # t17/O3: NAME the offending lines. The rejection stays + # all-or-nothing (nothing is half-stored), but the message quotes + # each bad line with its number and the reason, so a long paste + # with one typo is fixed instead of bisected by hand. + self._app_map_problems = problems return False app_map, icon_map = parsed present[CONF_APP_NAME_MAP] = app_map present[CONF_APP_ICON_MAP] = icon_map + # the paste parsed, so the previous rejection's detail is spent + self._app_map_problems = "" # ── the channel digit delay (§7 window) ── if CONF_CHANNEL_DIGIT_DELAY in present: @@ -2024,12 +2116,18 @@ class NvidiaShieldOptionsFlow(config_entries.OptionsFlowWithReload): discovered, ) icons = sorted({app.get("icon") for app in discovered if app.get("icon")}) + # t17/O3: when a paste was rejected, the SAME render says which lines were + # bad — the message rides the placeholder the error string interpolates, + # and it is cleared as soon as a submission parses. + placeholders = {"icons": ", ".join(icons) or "none known"} + if self._app_map_problems: + placeholders["invalid_lines"] = self._app_map_problems return self._show_sub_form( "apps_text", {vol.Optional(CONF_APP_NAME_MAP): _build_selectors()["app_map"]}, errors, override={CONF_APP_NAME_MAP: suggested}, - placeholders={"icons": ", ".join(icons) or "none known"}, + placeholders=placeholders, ) async def async_step_scripts(self, user_input=None): diff --git a/custom_components/nvidia_shield/media_player.py b/custom_components/nvidia_shield/media_player.py index 2854e89..921d10e 100644 --- a/custom_components/nvidia_shield/media_player.py +++ b/custom_components/nvidia_shield/media_player.py @@ -221,6 +221,22 @@ NOW_PLAYING_READING_KEYS = ( "media_info", ) +#: The state values a now-playing ROW may carry, and the whole documented set +#: (t17/O2). A reading comes from a tier that answered, and a tier answers in one +#: of these four: ``playing``/``paused``/``buffering`` from a live session or +#: Jellyfin, ``idle`` for a source that reported "nothing is playing" (including +#: the foreground tier's remembered-playback bridge, which is why the ROW's state +#: is the LADDER's answer and not the tier's own label). +#: +#: Anything else is deliberately NOT publishable. ``off`` means the device is off +#: (a stale session must not put a title beside it), ``on`` means nothing anywhere +#: reported a power or playback fact — so there is no reading to describe — and +#: ``standby`` is deprecated and never leaves the ladder. The gate used to be +#: "not off", which published whatever the entity state said; the README documents +#: four values, so the code now publishes exactly those four and a test walks +#: EVERY ``MediaPlayerState`` member against the README to keep the two in step. +NOW_PLAYING_STATE_VALUES = ("playing", "paused", "buffering", "idle") + def now_playing_attribute_names() -> frozenset[str]: """The flat now-playing attribute names — the whole published surface (t65).""" @@ -1365,6 +1381,12 @@ class NvidiaShieldMediaPlayer(MediaPlayerEntity): if self.state != MediaPlayerState.OFF: state = self.state.value reading = self._now_playing() + # t17/O2: only the four states a READING can carry are published, so + # a state outside the documented set can never leak into + # ``now_playing_state`` (see NOW_PLAYING_STATE_VALUES). The reading is + # dropped whole, exactly as it is for a device that reads off. + if state not in NOW_PLAYING_STATE_VALUES: + reading = None if reading: # The published state is the ENTITY's own state, so a dashboard # never reads two answers: the tier can briefly serve a diff --git a/custom_components/nvidia_shield/strings.json b/custom_components/nvidia_shield/strings.json index 304ead0..5f9ac28 100644 --- a/custom_components/nvidia_shield/strings.json +++ b/custom_components/nvidia_shield/strings.json @@ -120,7 +120,7 @@ "protocol_unavailable": "The androidtvremote2 library is not installed, so the remote protocol cannot pair. Install it and restart Home Assistant, or turn on 'Continue without protocol pairing' to finish setup anyway.", "protocol_unreachable": "The Shield did not answer at that address. Check the address above and that the Shield is awake, then select Submit to try again — or turn on 'Continue without protocol pairing' to finish setup without it.", "no_state_source": "No state source selected. Choose at least one: 'Add the Nvidia Remote Protocol' or 'Read state over ADB'.", - "invalid_app_map": "Each line must be package=name with a non-empty package (no spaces) and a non-empty name — for example ar.tvplayer.tv=TiviMate.", + "invalid_app_map": "The apps text was not saved — nothing was changed. {invalid_lines} Fix those lines and select Submit again, or clear the field to go back to the built-in list.", "invalid_digit_delay": "The digit delay must be between 0.05 and 1.0 seconds.", "invalid_app_row": "A row needs a package id with no spaces and a non-empty name — for example ar.tvplayer.tv=TiviMate. An icon, if given, cannot contain spaces.", "unknown_app_row": "That app is no longer in the list. Pick one of the rows offered, or add it again." diff --git a/custom_components/nvidia_shield/translations/en.json b/custom_components/nvidia_shield/translations/en.json index 304ead0..5f9ac28 100644 --- a/custom_components/nvidia_shield/translations/en.json +++ b/custom_components/nvidia_shield/translations/en.json @@ -120,7 +120,7 @@ "protocol_unavailable": "The androidtvremote2 library is not installed, so the remote protocol cannot pair. Install it and restart Home Assistant, or turn on 'Continue without protocol pairing' to finish setup anyway.", "protocol_unreachable": "The Shield did not answer at that address. Check the address above and that the Shield is awake, then select Submit to try again — or turn on 'Continue without protocol pairing' to finish setup without it.", "no_state_source": "No state source selected. Choose at least one: 'Add the Nvidia Remote Protocol' or 'Read state over ADB'.", - "invalid_app_map": "Each line must be package=name with a non-empty package (no spaces) and a non-empty name — for example ar.tvplayer.tv=TiviMate.", + "invalid_app_map": "The apps text was not saved — nothing was changed. {invalid_lines} Fix those lines and select Submit again, or clear the field to go back to the built-in list.", "invalid_digit_delay": "The digit delay must be between 0.05 and 1.0 seconds.", "invalid_app_row": "A row needs a package id with no spaces and a non-empty name — for example ar.tvplayer.tv=TiviMate. An icon, if given, cannot contain spaces.", "unknown_app_row": "That app is no longer in the list. Pick one of the rows offered, or add it again." diff --git a/tests/test_now_playing_publication.py b/tests/test_now_playing_publication.py new file mode 100644 index 0000000..898cb92 --- /dev/null +++ b/tests/test_now_playing_publication.py @@ -0,0 +1,175 @@ +"""tests/test_now_playing_publication.py — which entity states may carry a reading (t17/O2). + +THE OBSERVATION THIS CLOSES. The batch review read `extra_state_attributes` and +argued the now-playing row publishes "the entity state verbatim", so a dashboard +could see `now_playing_state: on`, while the README documents only `playing`, +`paused`, `buffering` and `idle`. t10's author had reported the opposite (an +`on` or `off` device publishes no row at all). Both accounts were about a +different thing, and this file settles it BY EXECUTION: + +* the t10 account is true for the NATURAL ladder — `on` means no source reported + a power or playback fact, and a reading only exists when a tier answered, so + the two cannot meet in practice; +* the review was right about the RULE, which was "publish unless the state is + `off`" — an unenforced assumption, and the probe below shows it publishing + `now_playing_state: on` (and `standby`) the moment a reading exists beside that + state. `standby` is not even in the README, and nothing in the code stopped it. + +WHAT WAS CHANGED. The publication rule now names the four values a reading may +carry (`NOW_PLAYING_STATE_VALUES`) and drops the reading when the entity state is +anything else, exactly as a state of `off` always did. The README says the same +thing in words. The two tests here walk EVERY ``MediaPlayerState`` member and +compare the README against the code constant, so neither can drift again. +""" + +from __future__ import annotations + +import re +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import entity_stublib # noqa: E402 +import pytest # noqa: E402 +from entity_stublib import MediaPlayerState, load_entity_modules # noqa: E402 + + +@pytest.fixture(autouse=True) +def scoped_entity_stubs(): + """Container for the HA stubs + the loaded entity modules (t7 discipline).""" + snapshot = entity_stublib.snapshot_modules() + entity_stublib.install() + yield load_entity_modules() + entity_stublib.restore_modules(snapshot) + + +README = Path(__file__).resolve().parent.parent / "README.md" + +#: The states the README documents, read from the file itself (never from the +#: constant under test) so a code change cannot silently rewrite the pin. +def _documented_states() -> set[str]: + text = README.read_text(encoding="utf-8") + row = re.search(r"^\| `now_playing_state` \|(.*)\|$", text, re.M) + assert row, "the README's now_playing_state table row is gone" + return {value for value in re.findall(r"`([a-z_]+)`", row.group(1)) if value != "on"} + + +def _player_for_state(modules, member): + """A player whose ladder answers `member` while a reading is available. + + The state is forced by patching the ``state`` property, which is the only way + to ask "what would be published IF the ladder ever answered this?" — the + question the review raised. A reading is genuine: the foreground tier answers + it through the same code path production uses (``_now_playing``). + """ + from test_command_routing import TIVIMATE_PACKAGE, _now_playing_attrs, _state_player + + player, _manager = _state_player(modules, foreground=TIVIMATE_PACKAGE) + player._last_channel = "101" + media_player_cls = type(player) + original = media_player_cls.__dict__.get("state") + media_player_cls.state = property(lambda self, forced=member: forced) + try: + reading = player._now_playing() + attrs = _now_playing_attrs(player) + finally: + if original is None: # pragma: no cover - the class always declares it + del media_player_cls.state + else: + media_player_cls.state = original + return reading, attrs + + +class TestThePublicationRule: + """One reading in, one row out — for EVERY member the enum has.""" + + def test_the_documented_four_publish_the_reading(self, scoped_entity_stubs): + modules = scoped_entity_stubs + documented = { + MediaPlayerState.PLAYING: "playing", + MediaPlayerState.PAUSED: "paused", + MediaPlayerState.BUFFERING: "buffering", + MediaPlayerState.IDLE: "idle", + } + + for member, value in documented.items(): + reading, attrs = _player_for_state(modules, member) + + assert reading, f"{value}: the fixture must have a reading to publish" + assert attrs["now_playing_state"] == value + assert attrs["now_playing_title"] == "101" + + def test_a_state_outside_the_documented_four_publishes_nothing(self, scoped_entity_stubs): + """The review's case, executed: `on` publishes no row — by construction now.""" + modules = scoped_entity_stubs + reading_available = { + MediaPlayerState.ON: "on", + MediaPlayerState.OFF: "off", + MediaPlayerState.STANDBY: "standby", + } + + for member, value in reading_available.items(): + reading, attrs = _player_for_state(modules, member) + + assert reading, f"{value}: the fixture still HAS a reading (that is the point)" + assert attrs == {}, f"{value} must publish no now-playing row, got {attrs}" + + def test_every_enum_member_is_accounted_for(self, scoped_entity_stubs): + """The matrix is exhaustive: no state is left untested as the enum grows.""" + modules = scoped_entity_stubs + published = {} + for member in MediaPlayerState: + reading, attrs = _player_for_state(modules, member) + published[member.value] = attrs.get("now_playing_state") + + assert set(published) == {member.value for member in MediaPlayerState} + for value in ("playing", "paused", "buffering", "idle"): + assert published[value] == value + for value in ("on", "off", "standby"): + assert published[value] is None, f"{value} published a state: {published[value]}" + + def test_the_gate_reads_the_named_constant(self, scoped_entity_stubs): + """The rule is the constant, not a hard-coded pair of states.""" + from nsh.media_player import NOW_PLAYING_STATE_VALUES + + assert NOW_PLAYING_STATE_VALUES == ("playing", "paused", "buffering", "idle") + assert self._published_for(scoped_entity_stubs, MediaPlayerState.ON) == {} + + @staticmethod + def _published_for(modules, member) -> dict: + _reading, attrs = _player_for_state(modules, member) + return attrs + + +class TestTheReadmeAgrees: + """The docs and the code say the same four values (t17/O2's anti-drift pin).""" + + def test_the_table_row_lists_exactly_the_constant(self, scoped_entity_stubs): + from nsh.media_player import NOW_PLAYING_STATE_VALUES + + assert _documented_states() == set(NOW_PLAYING_STATE_VALUES) + + def test_the_prose_names_the_same_set_and_excludes_on(self, scoped_entity_stubs): + text = README.read_text(encoding="utf-8") + from nsh.media_player import NOW_PLAYING_STATE_VALUES + + paragraph = re.search( + r"\*\*`now_playing_state` is one of FOUR values[^*]*\*\*(.*?)\n-", text, re.S + ) + assert paragraph, "the README's now_playing_state paragraph is gone" + body = paragraph.group(1) + # the sentence that names the set, read apart from the sentence that + # excludes `on`/`off` — otherwise the exclusion would look like a member + sentence = re.search(r"full set is ([^.]+)\.", body) + assert sentence, "the README no longer names the full set" + named = set(re.findall(r"`([a-z_]+)`", sentence.group(1))) + assert named == set(NOW_PLAYING_STATE_VALUES), named + assert "`on`" in body and "not one of them" in text, ( + "the paragraph must say `on` is excluded" + ) + + def test_the_off_and_on_prose_still_says_no_rows(self, scoped_entity_stubs): + text = README.read_text(encoding="utf-8") + + assert "**A device that reads `off` publishes none of them.**" in text + assert "publishes none either" in text diff --git a/tests/test_standalone_setup.py b/tests/test_standalone_setup.py index 1fc8389..8cfd229 100644 --- a/tests/test_standalone_setup.py +++ b/tests/test_standalone_setup.py @@ -706,6 +706,160 @@ class TestAppIconColumn: assert "app_name_map" not in flow._options # nothing half-stored +class TestAppPasteRejectionNamesTheLine: + """t17/O3: a rejected paste says WHICH line is wrong, not just that one is. + + The all-or-nothing rule stays (nothing is half-stored), but the message now + quotes each offending line with its number and the reason, so a 30-row paste + with one typo is fixed instead of bisected by hand. The row editor's own + single-row behaviour is pinned alongside, because neither half may regress. + """ + + def _flow(self, tmp_path, options=None): + module = _form_module() + entry = SimpleNamespace(options=dict(options or {}), title="Shield") + flow = module.NvidiaShieldOptionsFlow() + flow.config_entry = entry # t29 G3b: HA assigns config_entry + flow.hass = FakeHass(str(tmp_path)) + flow.async_show_form = lambda **kwargs: {"type": "form", **kwargs} + flow.async_create_entry = lambda **kwargs: {"type": "create_entry", **kwargs} + return module, flow + + async def test_the_message_names_the_numbered_bad_line(self, tmp_path): + module, flow = self._flow(tmp_path) + + result = await _options_submit( + flow, + { + "app_name_map": ( + "ar.tvplayer.tv=TiviMate\n" + "com.no.equals.sign\n" + "com.netflix.ninja=Netflix" + ) + }, + ) + + assert result["errors"] == {"base": "invalid_app_map"} + detail = result["description_placeholders"]["invalid_lines"] + # the SECOND line is the bad one, and it is quoted as the user wrote it + assert "line 2" in detail + assert "com.no.equals.sign" in detail + assert "no '='" in detail + assert "line 1" not in detail and "line 3" not in detail + assert "app_name_map" not in flow._options # still all-or-nothing + + async def test_every_bad_line_is_named_up_to_the_cap(self, tmp_path): + module, flow = self._flow(tmp_path) + + result = await _options_submit( + flow, + { + "app_name_map": ( + "com.good.one=Name\n" + "com.bad.space in id=Name\n" + "com.bad.blank=\n" + "com.bad.icon=Name|http://x y/z.png" + ) + }, + ) + + detail = result["description_placeholders"]["invalid_lines"] + assert "line 2" in detail and "package id" in detail + assert "line 3" in detail and "name after the '='" in detail + assert "line 4" in detail and "icon after the '|'" in detail + assert "line 1" not in detail # no false accusation + + async def test_a_long_bad_paste_is_capped_and_counts_the_rest(self, tmp_path): + module, flow = self._flow(tmp_path) + + result = await _options_submit( + flow, + {"app_name_map": "\n".join(f"no-equals-{n}" for n in range(1, 8))}, + ) + + detail = result["description_placeholders"]["invalid_lines"] + assert module.APP_MAP_PROBLEM_LIMIT == 4 + assert "line 4" in detail + assert "line 5" not in detail + assert "3 more line(s)" in detail, detail + + async def test_a_blank_line_is_not_a_row_and_never_a_problem(self, tmp_path): + module, flow = self._flow(tmp_path) + + await _options_submit( + flow, {"app_name_map": "\n\nar.tvplayer.tv=TiviMate\n\n"} + ) + + assert flow._options["app_name_map"] == {"ar.tvplayer.tv": "TiviMate"} + + async def test_a_good_paste_clears_the_previous_rejection(self, tmp_path): + module, flow = self._flow(tmp_path) + await _options_submit(flow, {"app_name_map": "com.bad row=Name"}) + assert flow._app_map_problems + + result = await _options_submit(flow, {"app_name_map": "com.good.one=Name"}) + + assert result["type"] == "menu" # back at the Apps screen, nothing left over + assert flow._app_map_problems == "" + assert flow._options_dict["app_name_map"] == {"com.good.one": "Name"} + + async def test_the_parser_reports_problems_in_a_pure_call(self): + module = _form_module() + + app_map, problems = module._app_map_parse("com.a=One\nbroken\ncom.b=", {}) + + assert app_map is None + assert [number for number, _line, _why in problems] == [2, 3] + assert problems[0][1] == "broken" + assert "no '='" in problems[0][2][0] + assert "name after the '='" in problems[1][2][0] + # and a valid paste reports nothing at all + assert module._app_map_parse("com.a=One", {}) == ({"com.a": "One"}, []) + + async def test_the_report_and_the_wrapper_cannot_disagree(self): + module = _form_module() + icons: dict[str, str] = {} + + assert module._app_map_from_text("com.a=One|http://x/i.png", icons) == { + "com.a": "One" + } + assert icons == {"com.a": "http://x/i.png"} + assert module._app_map_from_text("com.bad row=One", {}) is None + + def test_the_paste_report_and_the_row_editor_apply_the_same_rule(self): + """t17/O3 rider: the one-row path is unchanged, and both paths agree. + + The row editor is driven in tests/test_config_flow_ux.py + (``test_an_invalid_row_is_refused_and_stores_nothing``); what matters HERE + is that the new per-line report cannot reject a row the editor accepts (or + wave one through that the editor refuses) — otherwise a paste and a typed + row would disagree about the same text. + """ + module = _form_module() + rows = [ + ("com.a", "Alpha", "", True), + ("com.a", "Alpha", "http://x/i.png", True), + ("com.bad row", "Alpha", "", False), + ("", "Alpha", "", False), + ("com.a", "", "", False), + ("com.a", "Alpha", "http://x y/i.png", False), + ] + + for package, name, icon, valid in rows: + assert module._valid_app_row(package, name, icon) is valid, (package, name) + assert bool(module._app_row_problems(package, name, icon)) is not valid, ( + package, + name, + icon, + ) + + # the paste path STRIPS before checking, which is why a whitespace-only + # name cannot get in through it either (t49: nothing blank is stored) + app_map, problems = module._app_map_parse("com.a= ", {}) + assert app_map is None + assert problems[0][2] == ["the name after the '=' is empty"] + + class TestChannelEntryOptions: """P5-2: digit-delay + optional send-enter on the options page."""