Allow multiple media directories for collection_image - #180337
Conversation
There was a problem hiding this comment.
Pull request overview
Extends Collection Image to select random images across multiple media directories while retaining legacy entry compatibility.
Changes:
- Enables multi-directory media selection.
- Aggregates images across configured directories.
- Adds runtime multi-directory coverage and updates existing tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
config_flow.py |
Accepts and validates multiple directories. |
image.py |
Aggregates images and selects one randomly. |
conftest.py |
Adds a second mocked media directory. |
test_config_flow.py |
Updates inputs to the new list format. |
test_image.py |
Tests multi-directory image selection. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if not images: | ||
| if not self._unavailable_logged: | ||
| _LOGGER.info( | ||
| "%s: Has become available again", | ||
| "%s: No valid images in %s", | ||
| self.entity_id, |
There was a problem hiding this comment.
I don't think it's worthwhile to add extra complexity to avoid just a single extra log line in a pathological case. Right now we get one log per unbrowseable directory, and an overall error if we can't find anything at all. That seems reasonable to me.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
tests/components/collection_image/test_config_flow.py:52
- Add a config-flow test that submits at least two directories. Every updated flow test still supplies a one-item list, so the new loop can stop processing additional selections without any test failing; verify that both directories are browsed and the complete list is stored.
"media": [
{
"media_content_id": "media-source://mymedia",
"media_content_type": "",
}
],
homeassistant/components/collection_image/image.py:101
- Add a multi-source test where one browse raises
BrowseErrorand another source provides images. The existing tests cover either all-success or a single failed source, leaving the newly introduced skip-and-continue behavior unverified.
for media_content_id in self.media_content_ids:
try:
media = await async_browse_media(self.hass, media_content_id)
except BrowseError as err:
if not self._unavailable_logged:
_LOGGER.info(
"%s: Unable to browse %s: %s",
self.entity_id,
media_content_id,
err,
)
continue
homeassistant/components/collection_image/config_flow.py:18
- Update the config-flow strings to describe selecting multiple directories.
strings.json:13andstrings.json:15still refer to one directory/folder, so the UI will misdescribe this new multi-select field; update the source strings and regenerate the English translation.
vol.Required(CONF_MEDIA): MediaSelector(
{"accept": ["directory"], "multiple": True}
),
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/components/collection_image/conftest.py:112
- Remove this unused fixture. It is never requested by the test suite and, despite its name and docstring, returns a successful browse result rather than raising
BrowseError, which makes the shared test setup misleading.
def browse_media_browse_error() -> BrowseMediaSource:
"""Throws a BrowseError."""
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… into multi-dir-colleciton
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tests/components/collection_image/conftest.py:112
- Remove this unused and misleading fixture. No test requests it—the actual error behavior is provided directly by
browse_side_effect—and this fixture returns normal media despite claiming to raiseBrowseError.
def browse_media_browse_error() -> BrowseMediaSource:
"""Throws a BrowseError."""
homeassistant/components/collection_image/image.py:101
- Add a runtime test where one configured directory raises
BrowseErrorand another still supplies an image. The new skip-and-continue behavior is currently only exercised when every browse fails; the config-flow mixed-source case does not verify that the entity remains available and resolves an image from the working directory.
continue
Proposed change
Allow multiple media directories to be specified, instead of just 1. We will then randomize between all images in all selected directories.
Once we support multiple items, we can in the future extend it to picking directories and/or single images as well.
Requires home-assistant/frontend#53815
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: