mirror of
https://github.com/home-assistant/core.git
synced 2026-09-26 01:11:51 -04:00
Resolve review issues
This commit is contained in:
Michael Hansen
@@ -1,68 +0,0 @@
|
||||
# Timer list POC — known trade-offs
|
||||
|
||||
This branch is a proof of concept that replaces `intent`'s hand-rolled,
|
||||
in-memory voice-timer storage with real `timer_list` entities (one
|
||||
auto-created per `assist_satellite` device). It intentionally cuts scope in
|
||||
several places so the PR stays reviewable. These are called out here so
|
||||
reviewers and whoever picks up follow-on work don't have to rediscover them.
|
||||
|
||||
## Dropped / deferred features
|
||||
|
||||
- **`conversation_command` timers are gone.** The old system supported
|
||||
device-less timers that ran a delayed conversation command instead of
|
||||
notifying a device. This doesn't fit the entity-per-device model and needs
|
||||
its own design; it was removed rather than shimmed in.
|
||||
- **`mobile_app` timer support is broken.** `mobile_app` registers a device
|
||||
for timer push-notifications but has no `AssistSatelliteEntity`, so it
|
||||
never gets an auto-created `timer_list` entity. Starting a timer for a
|
||||
mobile_app device now raises `TimersNotSupportedError`. Its tests are
|
||||
marked `xfail` with the reason recorded inline
|
||||
(`tests/components/mobile_app/test_timers.py`). Fix: give mobile_app
|
||||
devices a `timer_list` entity the same way `assist_satellite` does.
|
||||
- **No `cancel_all_timers` / `clear_finished_timers` services.** Removed from
|
||||
`timer_list`/`local_timer_list` earlier in this branch. Archived timers can
|
||||
only be removed individually or via the automatic retention limit below.
|
||||
|
||||
## Behavior changes from the old voice-timer system
|
||||
|
||||
- **Finish action is always "archive."** Timers no longer support
|
||||
remove-on-finish or auto-restart; every finished/cancelled timer is kept
|
||||
(as `finished`/`cancelled`) until removed.
|
||||
- **Archived timers are capped at 10 per entity**, oldest evicted first, with
|
||||
no way to configure the limit.
|
||||
- **`created_seconds` no longer grows.** In the old system, adding time to a
|
||||
timer past its original length grew the value reported to satellites as
|
||||
the timer's nominal duration. The new `TimerItem.duration` is fixed at
|
||||
creation, so `TimerInfo.created_seconds` (and the `total_seconds` field
|
||||
sent to esphome/wyoming satellites) reflects the *original* duration even
|
||||
after time is added.
|
||||
- **Removing more time than remains skips the "updated" event.** The timer
|
||||
transitions straight to `finished`, matching `LocalTimerListEntity`'s
|
||||
existing behavior, rather than emitting an intermediate zero-duration
|
||||
update before finishing (as the old system did).
|
||||
|
||||
## Design shortcuts
|
||||
|
||||
- **Auto-created timer list entities aren't linked to their device.** They
|
||||
show up as standalone entities named `"{device name} Timers"` rather than
|
||||
nested under the satellite's device in the UI (no `DeviceInfo` set). Purely
|
||||
cosmetic; the lookup logic doesn't depend on device linkage and this can be
|
||||
added later.
|
||||
- **Auto-created vs. user-created entities are told apart implicitly.**
|
||||
Both live in the `timer_list` domain; the code distinguishes "this is a
|
||||
satellite's auto-created list" from "this is a user's `local_timer_list`
|
||||
helper" purely by the entity registry's `platform` field (`"timer_list"`
|
||||
vs. `"local_timer_list"`). There's no explicit flag — if that convention
|
||||
ever changes, voice matching (`_all_timer_infos` in
|
||||
`homeassistant/components/intent/timers.py`) silently stops seeing the
|
||||
right entities.
|
||||
- **No cleanup of orphaned timer list entities.** If a satellite device is
|
||||
removed and re-added with a new device ID, its old `timer_list` entity
|
||||
(keyed by the old device ID) is never cleaned up.
|
||||
- **New always-on dependency edges.** `intent` (a `system`-type integration,
|
||||
always loaded) now depends on `timer_list`, so `timer_list.async_setup`
|
||||
(services + websocket commands, no entities) runs on every Home Assistant
|
||||
install regardless of whether voice timers are used. `assist_satellite`
|
||||
now depends on `local_timer_list`.
|
||||
- **No user-facing control over the auto-created lists** — no config entry,
|
||||
options flow, or way to opt a satellite out of getting one.
|
||||
@@ -34,9 +34,14 @@ from homeassistant.helpers.typing import ConfigType
|
||||
from homeassistant.util import dt as dt_util
|
||||
|
||||
from .const import (
|
||||
ATTR_CREATED_AT,
|
||||
ATTR_DURATION,
|
||||
ATTR_FINISHED_AT,
|
||||
ATTR_FINISHES_AT,
|
||||
ATTR_REMAINING,
|
||||
ATTR_STATUS,
|
||||
ATTR_TIMER_ID,
|
||||
ATTR_TIMERS,
|
||||
DATA_COMPONENT,
|
||||
DOMAIN,
|
||||
TimerListEntityFeature,
|
||||
@@ -104,11 +109,11 @@ def timer_to_dict(item: TimerItem, now: datetime) -> dict[str, Any]:
|
||||
ATTR_TIMER_ID: item.timer_id,
|
||||
ATTR_NAME: item.name,
|
||||
ATTR_STATUS: item.status.value,
|
||||
"duration": item.duration.total_seconds(),
|
||||
"created_at": item.created_at.isoformat(),
|
||||
"finishes_at": item.finishes_at.isoformat() if item.finishes_at else None,
|
||||
"finished_at": item.finished_at.isoformat() if item.finished_at else None,
|
||||
"remaining": item.remaining_at(now).total_seconds(),
|
||||
ATTR_DURATION: item.duration.total_seconds(),
|
||||
ATTR_CREATED_AT: item.created_at.isoformat(),
|
||||
ATTR_FINISHES_AT: item.finishes_at.isoformat() if item.finishes_at else None,
|
||||
ATTR_FINISHED_AT: item.finished_at.isoformat() if item.finished_at else None,
|
||||
ATTR_REMAINING: item.remaining_at(now).total_seconds(),
|
||||
}
|
||||
|
||||
|
||||
@@ -332,7 +337,7 @@ async def _async_get_timers(
|
||||
now = dt_util.utcnow()
|
||||
statuses: list[TimerStatus] | None = call.data.get(ATTR_STATUS)
|
||||
return {
|
||||
"timers": [
|
||||
ATTR_TIMERS: [
|
||||
timer_to_dict(timer, now)
|
||||
for timer in entity.timers
|
||||
if not statuses or timer.status in statuses
|
||||
@@ -383,7 +388,7 @@ async def websocket_handle_subscribe(
|
||||
msg["id"],
|
||||
{
|
||||
"type": "timers",
|
||||
"timers": [timer_to_dict(timer, now) for timer in entity.timers],
|
||||
ATTR_TIMERS: [timer_to_dict(timer, now) for timer in entity.timers],
|
||||
},
|
||||
)
|
||||
)
|
||||
@@ -412,5 +417,5 @@ async def websocket_handle_list(
|
||||
now = dt_util.utcnow()
|
||||
connection.send_result(
|
||||
msg["id"],
|
||||
{"timers": [timer_to_dict(timer, now) for timer in entity.timers]},
|
||||
{ATTR_TIMERS: [timer_to_dict(timer, now) for timer in entity.timers]},
|
||||
)
|
||||
|
||||
@@ -125,7 +125,6 @@ NO_IOT_CLASS = [
|
||||
"tag",
|
||||
"temperature",
|
||||
"timer",
|
||||
"timer_list",
|
||||
"trace",
|
||||
"vibration",
|
||||
"web_rtc",
|
||||
|
||||
@@ -2074,7 +2074,6 @@ NO_QUALITY_SCALE = [
|
||||
"tag",
|
||||
"temperature",
|
||||
"timer",
|
||||
"timer_list",
|
||||
"local_timer_list",
|
||||
"trace",
|
||||
"usage_prediction",
|
||||
|
||||
@@ -1,212 +1,36 @@
|
||||
"""Tests for the Timer list integration."""
|
||||
|
||||
from datetime import datetime, timedelta
|
||||
from functools import partial
|
||||
from typing import override
|
||||
|
||||
from homeassistant.components.timer_list import (
|
||||
DOMAIN,
|
||||
TimerItem,
|
||||
InMemoryTimerListEntity,
|
||||
TimerListEntity,
|
||||
TimerListEntityFeature,
|
||||
TimerListEventType,
|
||||
TimerStatus,
|
||||
)
|
||||
from homeassistant.config_entries import ConfigEntry, ConfigFlow
|
||||
from homeassistant.core import CALLBACK_TYPE, HomeAssistant, callback
|
||||
from homeassistant.exceptions import ServiceValidationError
|
||||
from homeassistant.core import HomeAssistant
|
||||
from homeassistant.helpers.entity_platform import AddConfigEntryEntitiesCallback
|
||||
from homeassistant.helpers.event import async_track_point_in_utc_time
|
||||
from homeassistant.util import dt as dt_util, ulid as ulid_util
|
||||
from homeassistant.util import ulid as ulid_util
|
||||
|
||||
from tests.common import MockConfigEntry, MockPlatform, mock_platform
|
||||
|
||||
TEST_DOMAIN = "test"
|
||||
|
||||
ALL_FEATURES = (
|
||||
TimerListEntityFeature.START_TIMER
|
||||
| TimerListEntityFeature.PAUSE_TIMER
|
||||
| TimerListEntityFeature.CANCEL_TIMER
|
||||
| TimerListEntityFeature.ADD_TIME
|
||||
)
|
||||
|
||||
_FINISHED_STATUSES = (TimerStatus.FINISHED, TimerStatus.CANCELLED)
|
||||
MAX_ARCHIVED_TIMERS = 10
|
||||
|
||||
|
||||
class MockFlow(ConfigFlow):
|
||||
"""Test flow."""
|
||||
|
||||
|
||||
class MockTimerListEntity(TimerListEntity):
|
||||
class MockTimerListEntity(InMemoryTimerListEntity):
|
||||
"""Test timer list entity.
|
||||
|
||||
Reimplements the same in-memory storage and scheduling as the
|
||||
``local_timer_list`` platform, so the generic services, websocket API,
|
||||
and triggers can be exercised without depending on that integration.
|
||||
Subclasses the reference ``InMemoryTimerListEntity`` so the generic
|
||||
services, websocket API, and triggers are exercised against the real
|
||||
storage/scheduling logic without depending on the ``local_timer_list``
|
||||
integration.
|
||||
"""
|
||||
|
||||
_attr_supported_features = ALL_FEATURES
|
||||
|
||||
def __init__(self, name: str = "Timers") -> None:
|
||||
"""Initialize entity."""
|
||||
super().__init__()
|
||||
self._attr_name = name
|
||||
self._timers: dict[str, TimerItem] = {}
|
||||
self._cancel_callbacks: dict[str, CALLBACK_TYPE] = {}
|
||||
|
||||
@property
|
||||
@override
|
||||
def timers(self) -> list[TimerItem]:
|
||||
"""Return the timers in the list."""
|
||||
return list(self._timers.values())
|
||||
|
||||
@override
|
||||
async def async_start_timer(self, *, name: str | None, duration: timedelta) -> str:
|
||||
"""Create and start a new timer, returning its id."""
|
||||
now = dt_util.utcnow()
|
||||
timer_id = ulid_util.ulid_now()
|
||||
timer = TimerItem(
|
||||
timer_id=timer_id,
|
||||
name=name,
|
||||
status=TimerStatus.ACTIVE,
|
||||
duration=duration,
|
||||
created_at=now,
|
||||
finishes_at=now + duration,
|
||||
)
|
||||
self._timers[timer_id] = timer
|
||||
self._schedule(timer)
|
||||
self._notify(TimerListEventType.STARTED, timer)
|
||||
return timer_id
|
||||
|
||||
@override
|
||||
async def async_pause_timer(self, timer_id: str) -> None:
|
||||
"""Pause an active timer."""
|
||||
timer = self._get_timer(timer_id)
|
||||
if timer.status != TimerStatus.ACTIVE or timer.finishes_at is None:
|
||||
return
|
||||
timer.remaining = max(timedelta(0), timer.finishes_at - dt_util.utcnow())
|
||||
timer.finishes_at = None
|
||||
timer.status = TimerStatus.PAUSED
|
||||
self._unschedule(timer_id)
|
||||
self._notify(TimerListEventType.UPDATED, timer)
|
||||
|
||||
@override
|
||||
async def async_unpause_timer(self, timer_id: str) -> None:
|
||||
"""Resume a paused timer."""
|
||||
timer = self._get_timer(timer_id)
|
||||
if timer.status != TimerStatus.PAUSED or timer.remaining is None:
|
||||
return
|
||||
timer.finishes_at = dt_util.utcnow() + timer.remaining
|
||||
timer.remaining = None
|
||||
timer.status = TimerStatus.ACTIVE
|
||||
self._schedule(timer)
|
||||
self._notify(TimerListEventType.UPDATED, timer)
|
||||
|
||||
@override
|
||||
async def async_cancel_timer(self, timer_id: str) -> None:
|
||||
"""Cancel a timer, archiving it in the ``cancelled`` state."""
|
||||
timer = self._get_timer(timer_id)
|
||||
self._unschedule(timer_id)
|
||||
timer.status = TimerStatus.CANCELLED
|
||||
timer.finishes_at = None
|
||||
timer.remaining = None
|
||||
timer.finished_at = dt_util.utcnow()
|
||||
self._notify(TimerListEventType.CANCELLED, timer)
|
||||
self._enforce_archive_limit()
|
||||
|
||||
@override
|
||||
async def async_add_time(self, timer_id: str, duration: timedelta) -> None:
|
||||
"""Add (or, with a negative duration, remove) time on a timer."""
|
||||
timer = self._get_timer(timer_id)
|
||||
if timer.status == TimerStatus.ACTIVE and timer.finishes_at is not None:
|
||||
now = dt_util.utcnow()
|
||||
finishes_at = timer.finishes_at + duration
|
||||
if finishes_at <= now:
|
||||
self._unschedule(timer_id)
|
||||
self._async_timer_finished(timer_id, now)
|
||||
return
|
||||
timer.finishes_at = finishes_at
|
||||
self._schedule(timer)
|
||||
elif timer.status == TimerStatus.PAUSED and timer.remaining is not None:
|
||||
timer.remaining = max(timedelta(0), timer.remaining + duration)
|
||||
else:
|
||||
return
|
||||
self._notify(TimerListEventType.UPDATED, timer)
|
||||
|
||||
@override
|
||||
async def async_remove_timer(self, timer_id: str) -> None:
|
||||
"""Remove a timer from the list regardless of its status."""
|
||||
timer = self._get_timer(timer_id)
|
||||
self._unschedule(timer_id)
|
||||
del self._timers[timer_id]
|
||||
self._notify(TimerListEventType.REMOVED, timer)
|
||||
|
||||
def _get_timer(self, timer_id: str) -> TimerItem:
|
||||
"""Return a timer by id or raise if it does not exist."""
|
||||
if (timer := self._timers.get(timer_id)) is None:
|
||||
raise ServiceValidationError(
|
||||
translation_domain=DOMAIN,
|
||||
translation_key="timer_not_found",
|
||||
translation_placeholders={"timer_id": timer_id},
|
||||
)
|
||||
return timer
|
||||
|
||||
@callback
|
||||
def _schedule(self, timer: TimerItem) -> None:
|
||||
"""Schedule (or reschedule) the finish callback for a timer."""
|
||||
self._unschedule(timer.timer_id)
|
||||
assert timer.finishes_at is not None
|
||||
self._cancel_callbacks[timer.timer_id] = async_track_point_in_utc_time(
|
||||
self.hass,
|
||||
partial(self._async_timer_finished, timer.timer_id),
|
||||
timer.finishes_at,
|
||||
)
|
||||
|
||||
@callback
|
||||
def _unschedule(self, timer_id: str) -> None:
|
||||
"""Cancel a pending finish callback, if any."""
|
||||
if cancel := self._cancel_callbacks.pop(timer_id, None):
|
||||
cancel()
|
||||
|
||||
@callback
|
||||
def _async_timer_finished(self, timer_id: str, now: datetime) -> None:
|
||||
"""Handle a timer reaching its finish time, archiving it."""
|
||||
self._cancel_callbacks.pop(timer_id, None)
|
||||
if (timer := self._timers.get(timer_id)) is None:
|
||||
return
|
||||
|
||||
timer.status = TimerStatus.FINISHED
|
||||
timer.finishes_at = None
|
||||
timer.remaining = None
|
||||
timer.finished_at = dt_util.utcnow()
|
||||
self._notify(TimerListEventType.FINISHED, timer)
|
||||
self._enforce_archive_limit()
|
||||
|
||||
@callback
|
||||
def _enforce_archive_limit(self) -> None:
|
||||
"""Evict the oldest archived timers beyond ``MAX_ARCHIVED_TIMERS``."""
|
||||
archived = sorted(
|
||||
(
|
||||
timer
|
||||
for timer in self._timers.values()
|
||||
if timer.status in _FINISHED_STATUSES
|
||||
),
|
||||
key=lambda timer: timer.finished_at or dt_util.utcnow(),
|
||||
)
|
||||
excess = len(archived) - MAX_ARCHIVED_TIMERS
|
||||
if excess <= 0:
|
||||
return
|
||||
for timer in archived[:excess]:
|
||||
del self._timers[timer.timer_id]
|
||||
self._notify(TimerListEventType.REMOVED, timer)
|
||||
|
||||
@override
|
||||
async def async_will_remove_from_hass(self) -> None:
|
||||
"""Cancel all pending finish callbacks."""
|
||||
for cancel in self._cancel_callbacks.values():
|
||||
cancel()
|
||||
self._cancel_callbacks.clear()
|
||||
super().__init__(name=name, unique_id=ulid_util.ulid_now())
|
||||
|
||||
|
||||
async def create_mock_platform(
|
||||
|
||||
@@ -136,6 +136,28 @@ async def test_timer_cancelled_trigger(
|
||||
assert service_calls[0].data["status"] == "cancelled"
|
||||
|
||||
|
||||
async def test_timer_updated_trigger(
|
||||
hass: HomeAssistant, service_calls: list[ServiceCall]
|
||||
) -> None:
|
||||
"""Test the timer_updated trigger fires when a timer is paused."""
|
||||
await _setup_automation(hass, "timer_updated")
|
||||
timer_id = await _start_timer(hass)
|
||||
await hass.async_block_till_done()
|
||||
assert len(service_calls) == 0
|
||||
|
||||
await hass.services.async_call(
|
||||
DOMAIN,
|
||||
"pause_timer",
|
||||
{"timer_id": timer_id},
|
||||
target={ATTR_ENTITY_ID: TEST_ENTITY_ID},
|
||||
blocking=True,
|
||||
)
|
||||
|
||||
assert len(service_calls) == 1
|
||||
assert service_calls[0].data["timer_id"] == timer_id
|
||||
assert service_calls[0].data["status"] == "paused"
|
||||
|
||||
|
||||
async def test_trigger_options_supported(hass: HomeAssistant) -> None:
|
||||
"""Test the timer list triggers do not advertise behavior or duration."""
|
||||
for trigger_type in (
|
||||
|
||||
Reference in New Issue
Block a user