Added media browsing request#50
Conversation
|
Please wait with enhancing the integration library: the Integration-API specification for media browsing and searching is not yet finalized in the core-api repository and might still change! |
|
Ok sorry, I was anticipating a little bit too much : I just want to have a working solution, then it won't be a problem to bring some changes. I can keep going with my simulator and raise questions on discord channel. |
zehnm
left a comment
There was a problem hiding this comment.
The Python integration library should be similar as the Node.js library.
Instead of a generic params: dict[str, Any] browse parameter, it is much more convenient for integration developers to know what they are getting, and what to send back.
Please check https://github.com/unfoldedcircle/integration-node-library/blob/main/lib/entities/media_player.ts for all definitions.
They can easily be converted into Python classes with your AI agent of choice, including Python docs :-)
|
I can offer to do the refactoring after the next release is out. |
|
I am on it |
|
All done, I have integrated it locally in Kodi integration and it works fine. Just the pagination "count" field to check vs NodeJS implementation |
| data: AssistantEventData | None = None | ||
|
|
||
|
|
||
| class MediaContentType(str, Enum): |
There was a problem hiding this comment.
strEnum is the preferred approach for defining enums. It also aligns with the behavior found when calling asdict() as it returns the value of the enum rather than the enum.
The one question would be if it is better to align with the previous declarations using (str, Enum) or if it is agreed that this is the better approach, using it going forward.
There was a problem hiding this comment.
Ok I changed it StrEnum
There was a problem hiding this comment.
I do want @zehnm to weigh in on this change. It does break with the other enum definitions. I think it's better, and is a natural break point, but since it isn't like the others we should see what he thinks.
There was a problem hiding this comment.
I'm traveling today but should get a chance to look at it on the train
There was a problem hiding this comment.
strEnum definitely makes sense with the minimal Python version 3.11.
I can change all other str, Enum definitions before the next library release.
|
OK I have made the fixes reported by @JackJPowell |
|
Ok this is done, you can review, thanks |
- move media-player related classes to media_player.py - create msg_definitions.py for internal WS msg data definitions - Replace MediaType with MediaContentType - Use StrEnum for all media-player enums
|
Since I don't call myself a Python developer, I've asked Junie Pro for some feedback about the enums. Using Why this works well:
I'll prepare the changes and also improve the Paging / Pagination classes. An integration driver should always receive a Paging class for consistent handling. |
- Rename `PagingOptions` to `Paging` - Replace `PagingOptions` with `Paging` for better consistency. - Add validation, default values, and utility methods to `Paging`. - Make Paging mandatory in `BrowseOptions` and `SearchOptions` - Update references across `media_player.py` and `api.py` to use the new `Paging` class.
|
Refactoring should be done now. I'll test it tomorrow. |
I suspect the trouble was before the change to strEnum. Glad your testing worked since the change above looks good and looks like it should work 😅 |
zehnm
left a comment
There was a problem hiding this comment.
I will fix the media_classes definition in SearchMediaFilter
Updated `media_classes` attribute in `SearchMediaFilter` to accept both `MediaClass` and custom string values. Added unit tests.
zehnm
left a comment
There was a problem hiding this comment.
I've fixed all issues from the last review, added documentation and made the browse and search error handling more robust.
@JackJPowell @albaintor please double check if I didn't introduce new issues :-)
The media-player module is refactored in PR #50
| if self.album is not None: | ||
| validate_str("album", self.album) | ||
| if isinstance(self.media_class, str): | ||
| validate_str("media_class", self.media_class, 0) |
There was a problem hiding this comment.
Just wanted to confirm this was intentional that it's valid to have an empty string for media_class and media_type since None is valid for these too.
There was a problem hiding this comment.
Yes, that's a special quirk for Home Assistant. None and empty are different things. And some HA media-player integrations hand out empty media_class and media_type values and expect them again for continue browsing or start playing media.
If you omit it, HA rejects it.
It's also documented in the media-player entity documentation.
There was a problem hiding this comment.
Pytest is the testing framework preference now, but these tests will still run under pytest if we wanted to move in that direction and can be easily converted later. AI can do it in a heartbeat as well.
| @@ -1,193 +1,659 @@ | |||
| """ | |||
There was a problem hiding this comment.
Entity specific classes are not exported in init.py so I assume we don't want to start by adding the new MediaPlayer classes either... but wanted to point it out
There was a problem hiding this comment.
I guess they should be exported :-) Completely forgot about it!
There was a problem hiding this comment.
I've added the unique media-player classes in init.
Python modules and exports will always feel weird to me...
Let me know if you see missing things or other improvements
JackJPowell
left a comment
There was a problem hiding this comment.
Just very minor things. Mostly questions with a couple of additions to init
|
Those last changes look great! I think everything is being exported now. |
|
Thanks everyone! |
Breaking Changes - Renamed `MediaType` to `MediaContentType` and changed enums to lowercase. See media-player entity documentation for more information (#50). - Changed `str, Enum` to new Python 3.11 `StrEnum` class (#54). - All entity constructors require named parameters for the optional fields (#49, #54).
Hi Markus @zehnm ,
To begin with the new media browsing in Kodi integration, I modified the Python integration library with new request/messages.
For now I simulate requests and it seems to work correctly.
Only
browse_mediarequest is handled at this time. I will add support forplay_mediaand then :