-
Notifications
You must be signed in to change notification settings - Fork 478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[en] Add stop, previous, clear playlist intents #2080
base: main
Are you sure you want to change the base?
[en] Add stop, previous, clear playlist intents #2080
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
slot_combinations: | ||
name_only: | ||
- "name" | ||
area_only: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure it's OK to "clear playlist in the living room"? Shouldn't this be name-only? You haven't even added sentences for any other scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be my lack of understanding at the moment. I would think we do want the ability to in the future just be able to say "Clear the queue" and have the area of the satellite used to work out what player should be targeted. What should I do in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree about the area awareness, that most intents should act immediately on one/all of the targeted entities in the satellite's area.
However, this may be a bit more tricky, as clearing/deleting is a destructive action, with potentially frustrating implications. Imagine the scenario where you have 2 media_player
s and a voice satellite in the office
area. You say clear the playlist
but forget you have 2 players. The first media_player
matches and gets its playlist cleared, but you actually wanted to perform the action on the second. That sounds annoying.
What I'd do is that for this intent (and any other destructive/irreversible ones) if that I'd only allow targeting by device name so as to prevent accidental triggerings. Maybe in the future we will have a short conversation for confirmation (Are you sure you want to clear the playlist on <media player name>?
Yes
Cleared
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK so to fix this can you confirm I should remove all reference to area in the clear playlist intent and then I need to add sentences which include in <area>
against the other intents?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tetele I see in the latest beta area support. Are there changes I should make to this PR now as a result of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to make sure you match the behavior there. Good or bad, at least it will be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looking at it now. Looking at Mike's recent PRs it seems I should remove slot combinations throughout intents.yaml and add similar sentence variations to what he has done for each of my commands.
intents: | ||
HassMediaPrevious: | ||
data: | ||
- sentences: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also use "play"/"replay" as verbs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think just previous track on <name>
should work, without the verb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look at what I have changed here and see if I have captured what you were thinking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about omitting the verb, that wouldn't be correct grammar and I thought that was desired....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@synesthesiam any thoughts on this?
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
@OzGav I think you forgot to push the fixes, I can't see anything new, so I can't review anything new :D |
I am still a github noob and I was working on the wrong branch. Fixed now (I think!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the comments that are still open, I'm good with this.
Being a set of new intents, we will wait for Mike's input.
Just gently checking in as to whether there is any news? |
@synesthesiam in the context of 2024.6, will you consider this PR too? |
I'm torn on this. Maybe @jlpouffier will have more thoughts? First, I'm hesitant to have both "pause" and "stop" intents for media players since it's not always clear how these are different. Sometimes, you can resume from a stopped state; sometimes, you can't. It's confusing, and so I've avoided it by only including "pause" 🙈 Second, we need to be careful about introducing too many intents since this puts a translation burden on language leaders, as well as a maintenance burden in HA. Our original goal was to cover the "most common" voice assistant use cases, but there's no formal definition of what this means. However we define it, I think we have a problem right now with intents requiring code inside HA and the sentences not being sharable in a blueprint style. So we can't have a set of community-supported intents that could get promoted to "official" status given enough use 🙁 What do you think? |
I wasn't across the desire to only have the "most common" intents so I approached this by creating intents for all the media player service calls. I help out with the Music Assistant project and we have shared custom sentences that cover all of these (including a form of area awareness). I then thought to bring these across to core and so here we are. If HA will remain only supporting this subset of calls that is OK we will just amend our docs to advise everyone of this fact and that they should use the supplied sentences or create their own. As for pause and stop you are correct. In MA "stop" will stop playback and clear the queue whereas pause just stops playback |
WalkthroughWalkthroughThe changes introduce new intents for controlling media players within a home automation system, specifically for stopping playback, clearing playlists, and skipping to the previous item. These updates encompass the addition of new intents, response templates, language definitions, and test cases to support these media control actions. Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (10)
Additional comments not posted (10)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
These have been removed from here #2063 to make it more managable.
Summary by CodeRabbit
New Features
Tests