VoiceCommand trigger added to Microsoft.Toolkit.Uwp.UI.Behaviors (#3392) #3661
Conversation
|
Thanks sonnemaf for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
ghost
requested review from
|
Forgot to add the screenshot from the SampleApp. You can test it using the 'Add', 'Delete', 'Append', 'Move Available/Selected Up/Down/To First/To Last', 'What can I say' speech commands.
|
ghost
added
the
|
I have added an IsEnabled property on the VoiceCommand trigger. In the SamplePage I have added a ToggleSwitch 'Voice'. If it is Off the buttons and listbox commands are not working. See: |
| } | ||
| } | ||
|
|
||
| public string Text |
There was a problem hiding this comment.
Would it make sense to turn this into a list for better maintainability / readability?
So instead of:
<Behaviors:VoiceCommandTrigger Text="Move available to First|Move available too First">
something like:
Behaviors:VoiceCommandTrigger
Behaviors:VoiceCommandTrigger.Commands
</Behaviors:VoiceCommandTrigger.Commands>
</Behaviors:VoiceCommandTrigger>
I have to say that's way more verbose, which isn't great at all.. but might be easier to append a string/voice command to a list programmatically instead of re-formatting a string.
There was a problem hiding this comment.
I have thought about this too. I decided not to implement this. It would make localization much harder. With my | (pipe) separator you can easily add or remove alternatives for different languages. This will be very hard if you use a Commands collection.
There was a problem hiding this comment.
Should we still have the type be a list and use a custom converter to parse the text and split it?
https://timheuer.com/blog/implement-type-converter-uwp-winrt-windows-10-xaml
Would it also make sense to make the delimiter a property that can be overridden? Comma by default seems like a better choice?
| SpeechRecognizer sr = null; | ||
| foreach (var item in Windows.System.UserProfile.GlobalizationPreferences.Languages) | ||
| { | ||
| var language = new Language(item); |
There was a problem hiding this comment.
Should the language be set-able in the voice command as well (for multi-language use cases) and by default use this?
There was a problem hiding this comment.
Don't think Language should be a property on the VoiceCommandTrigger. Would love to know what others think though. It's all new for me too.
|
@sonnemaf seems like the app is breaking when I attempt to open it or try to select the Add button in the app. Attached is the screen-recording.
|
You are right. The app crashed when you didn't allow the use of the Microphone in this app.
I have now fixed this with a new push. To test the app you can give it permissions to the microphone in the Settings of your computer. Search the App in the 'Apps & Features'.
Click on the 'Advanced options' hyperlink. Turn the Microphone toggle switch ON.
|
|
Sorry about the build break, we had an issue with a stale config item. I've updated the base branches with the fix, so hopefully this will build again now and be easier for folks to test via NuGet? |
|
@sonnemaf tried various times but seems like it keeps breaking the moment I select VoiceCommand in the sample app. Before this update, it would still land to the page allowing me to Add items but now it just crashes directly with the same exception message. Can you try on your end and see if you are able to repro? |
|
@sonnemaf can you also ensure you've added the required headers and fixed any StyleCop issues (if any) so that the CI can pass? |
|
@michael-hawker I have just pushed a new version with Xml comments and fixed StyleCop issues. I hope (expect) it will now pass. |
| @@ -0,0 +1,95 @@ | |||
| using System; | |||
There was a problem hiding this comment.
You're missing the headers on the files here, so CI is still failing. You can run the UpdateHeaders command in the build directory to help, though if it somehow touches more files beyond yours, don't commit those.
The build script in the build directory can also do a local test build as if it were in the CI, so that can help to validate.
There was a problem hiding this comment.
Headers updated. I forgot that I had to do this. I'm a bit rusty, sorry.
|
@sonnemaf looks like you provided a '.bind' file for an interactive sample, but then it doesn't actually work because you have If we need to make the sample guidance clearer in any regard, let us know by opening an issue on the wiki repo. Thanks! It might be good to have a prompt text at the top of the sample to get folks started. I didn't see the microphone prompt pop-up though for me, so I don't think it's working? Is this going to be a problem for folks upgrading apps? |
|
Is there an API we can use to detect if we have Microphone access so we can help display/troubleshoot? Also, I just noticed this branch has the same name as the head vs. for the specific feature so it makes it a bit harder to manage. Don't think it'll cause problems, but not sure? |
I have removed the two .bind files. I have added a 'What can I say' button to the sample page. It will show you which commands you can speak.
|
|
@sonnemaf I am still not able to see this pop-up and checked everything on my end 🤔
|
|
@Kyaa-dost did you restart the app after turning the setting off? |
|
Sorry I accidentally hit the 'Close with comment' button. Have reopened it again. |
|
@sonnemaf Correct I have tried restarting multiple times but not seeing that at all. |
|
@Kyaa-dost I had the same result. It works if you manually uninstall the app. If you then deploy a new version you will get the MessageBox again.
|
|
@sonnemaf I have tried that multiple times in different ways and not seeing at all☹ I will wait for @michael-hawker to confirm and see if he is able to see this popup screen now. |
|
@Kyaa-dost Maybe you can try as a last resort a Reset in the settings. Find the apps in
|
|
I am also not seeing anything requesting the microphone, tried resting, uninstalling. Nothing worked. |
|
@sonnemaf I am getting the same error on the Reset button as well. I am not certain what is causing the issue and perhaps something will need more time to investigate 🤔 |
|
@RosarioPulella, @Kyaa-dost Can you verify if the
|
|
@sonnemaf Correct it's turned on
|
|
The option is also enabled for me too. |
|
Moving this to 7.1 for now since we can't understand why the proper permissions aren't being prompted for. There's something very odd going on as we can't even get the proper prompt on a fresh machine for us... |
Fixes #3392
This PR implements Issue #3392
PR Type
What kind of change does this PR introduce?
What is the new behavior?
A VoiceCommand triggers an Action using a SpeechRecognizer.
PR Checklist
Please check if your PR fulfills the following requirements:
Other information
The XML Comments are not written yet. This PR is there for retrieving initial feedback.