fix(media_player,config_flow): close the batch review's O2 and O3 (t17)

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.
This commit is contained in:
James Mackay committed 2026-10-12 12:36:15 +01:00
1 parent 91d9fb3883
commit 2819ee5a49
7 files changed
+489 -33

No files matched your search

+128 -30
View File
@@ -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):
@@ -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
+1 -1
View File
@@ -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."
@@ -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."