-
Notifications
You must be signed in to change notification settings - Fork 84
Users/hossein/silence detection #1336
base: main
Are you sure you want to change the base?
Users/hossein/silence detection #1336
Conversation
# Conflicts: # packages/Telephony/Microsoft.Bot.Components.Telephony.csproj
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.
Had only one comment so far, more later
//private const string TimeoutId = "this.TimeoutId"; | ||
//private const string ActiveTimeoutId = "conversation.ActiveTimeoutId"; | ||
|
||
private static ConcurrentDictionary<string, (ConcurrentQueue<string> triggeredTimers, IStateMatrix timersState)> conversationStateMatrix = new ConcurrentDictionary<string, (ConcurrentQueue<string> triggeredTimers, IStateMatrix timersState)>(); |
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.
Will this work in a setup with multiple instances?
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.
By multiple instances means multiple nodes one per region?
I believe each instance is its own app and runs individually and shouldn't affect each other, am I right?
We need the readme to get updated. |
/// <summary> | ||
/// The middleware that handles all outgoing messages | ||
/// </summary> | ||
public class SetSpeakMiddleware : IMiddleware |
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.
Copy and paste error?
"$schema": "https://schemas.botframework.com/schemas/component/v1.0/component.schema", | ||
"$role": [ "implements(Microsoft.IDialog)", "extends(Microsoft.InputDialog)" ], | ||
"type": "object", | ||
"title": "(Preview)1 text input dialog with silence detection", |
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.
Style: Capitalize, use non-digit version of number.
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.
Fixed
"$role": [ "implements(Microsoft.IDialog)", "extends(Microsoft.InputDialog)" ], | ||
"type": "object", | ||
"title": "(Preview)1 text input dialog with silence detection", | ||
"description": "getting arbitrary text and detecting silence in case of no entry", |
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.
Grab the description from the existing TextInput
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.
Fixed.
"defaultValue": { | ||
"$ref": "schema:#/definitions/stringExpression", | ||
"title": "Default value", | ||
"description": "'Property' will be set to the value of this expression when max turn count is exceeded.", |
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.
Does TextInput have a max turn count? Is this default value only available on silence?
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.
MaxTurn is used only for invalid inputs
However in case of Detected Silence , as a hack, we set the max turn to one to force the component to continue and that will set the value to the default before proceeding
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.
Also I cloned this file from Microsoft.Bot.Builder.Dialogs.Adaptive\Schemas\Dialogs\Microsoft.TextInput.schema
@@ -18,6 +19,10 @@ public class TelephonyBotComponent : BotComponent | |||
public override void ConfigureServices(IServiceCollection services, IConfiguration configuration) | |||
{ | |||
// Conditionals | |||
var middleware = new Middleware.SetSpeakMiddleware(); | |||
services.AddSingleton<IMiddleware>(middleware); | |||
middleware.addEventReceiver(ActivityTypes.EndOfConversation, TimeoutInput.RemoveTimers); |
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.
Does this middleware actually get correctly registered? I believe this DI entry point is special and won't be able to register middleware.
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.
Seems working and get registered
@@ -51,6 +52,8 @@ | |||
<None Remove="Schemas\Microsoft.Telephony.ResumeRecording.uischema" /> | |||
<None Remove="Schemas\Microsoft.Telephony.TimeoutChoiceInput.schema" /> | |||
<None Remove="Schemas\Microsoft.Telephony.TimeoutChoiceInput.uischema" /> | |||
<None Remove="Schemas\Microsoft.Telephony.TimeoutTextInput.schema" /> |
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.
Style: spacing
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.
fixed
|
||
private static ConcurrentDictionary<string, (ConcurrentQueue<string> triggeredTimers, IStateMatrix timersState)> conversationStateMatrix = new ConcurrentDictionary<string, (ConcurrentQueue<string> triggeredTimers, IStateMatrix timersState)>(); | ||
|
||
public static async Task<DialogTurnResult> BeginDialogAsync<K>(K inputActivity, DialogContext dc, |
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.
Style: Prefer T over K
return await dc.EndDialogAsync(cancellationToken: cancellationToken).ConfigureAwait(false); | ||
} | ||
|
||
dc.State.SetValue(ITimeoutInput.SilenceDetected, false); |
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.
What do you think, is this property more discoverable here, or on the interface? I generally don't expect interfaces to know things.
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 but to reduce the redundancies and having it in both TimeoutChoiceInput and TimeoutTextInput (as they don't derived from a same base class) I thought we can use interface to carry it and reduce rewriting.
Also the main problem would be when we pass them into TimeoutInput as a single controller for both!
//-----------------------------Body--------------- | ||
var activity = dc.Context.Activity; | ||
|
||
//we have to manage our timer somehow. |
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 these comments still valid?
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.
Removed. Thanks
Also, could probably use some tests, at least to validate/explain what the middleware should do |
Is this still a thing or can it be closed? |
Adding two new components "TimeoutChoiceInput", and "TimeoutTextInput" to detect the silence while prompting the user.
It's using a timer to track elapsed time and fire in case of reaching threshold