Skip to content
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

[Facebook Adapter] Add typing indicator support #3130

Merged
merged 3 commits into from
Jan 23, 2020

Conversation

matiasroldan6
Copy link
Contributor

Description

The changes address the issue raised in bug #3071 where the typing indicator is not supported in the .NET version of the adapter yet.

Proposed changes

To display a typing_on sender_action every time the bot prepares a message, we added the SendMessage method in FacebookHelper which creates an extra activity with the required format for the Sender Action (note that FacebookMessage.Message needs to be null when Sender Actions are present) and then sends it along with the actual message activity using TurnContext SendActivitiesAsync method.
Sample bots have been modified accordingly, displaying typing indicators for messages, attachments, and templates.

@fuselabs
Copy link
Collaborator

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.Luis.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.QnA.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.ApplicationInsights.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Azure.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Dialogs.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Integration.ApplicationInsights.Core.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Integration.AspNet.Core.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.TemplateManager.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Testing.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Configuration.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Connector.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Schema.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Streaming.dll compared against version 4.6.3

@garypretty
Copy link
Contributor

@benbrown I can review this, but would be great if you could take a quick look as well with regards to resolving #3071

Copy link
Contributor

@benbrown benbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tests should cover different scenarios.

Typing indicators will most often be used when an operation may take some time. IE, the typing indicator is sent, then the long running operation is undertaken, then the final message is sent.

  • send a typing activity on its own, not as part of a special sendWithTypingIndicator method. This is most often how this will be used.
  • include a typing indicator in a dialog.

public static async Task SendActivityWithTypingIndicatorAsync(IActivity activity, ITurnContext turnContext, CancellationToken cancellationToken)
{
var typingActivity = GenerateTypingActivity(turnContext.Activity.Conversation.Id);
await turnContext.SendActivitiesAsync(new[] { typingActivity, activity }, cancellationToken).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be extraneous since the messages will both go out very quickly in sequence meaning there won't be much time for the typing indicator to be displayed before the next message arrives...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Ben. Then we'll change it to let the user manually create the typing indicator activity and then send it before the message activity (basically what the method SendActivityWithTypingIndicatorAsync is doing. We would then remove the method of course).
We will also include an example with a dialog.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds right to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok now SendActivityWithTypingIndicatorAsync has been removed. The bot needs to create a typing indicator activity by calling FacebookHelper.GenerateTypingActivity.
There are examples for sending messages with and without typing indicator in the bot, as well as a trigger phrase (display typing indicator) for displaying a typing indicator alone.
I also made some other minor corrections I found.

@benbrown
Copy link
Contributor

Also, the generated documentation should make clear how to send a typing indicator on its own.

@fuselabs
Copy link
Collaborator

✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.Luis.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.AI.QnA.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.ApplicationInsights.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Azure.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Dialogs.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Integration.ApplicationInsights.Core.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Integration.AspNet.Core.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.TemplateManager.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.Testing.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Configuration.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Connector.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Schema.dll compared against version 4.6.3
✔️ No Binary Compatibility issues for Microsoft.Bot.Streaming.dll compared against version 4.6.3

@garypretty
Copy link
Contributor

@benbrown are you happy with this now? If so, please approve and I will get it merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants