Fixing show typing and delay #121
Conversation
…cause there is no flush method.
|
I don't like the type casting to Activity. I found another way of creating the Activity by setting the ServiceUrl, From, and Conversation if that is preferred. |
… the activity types.
There was a problem hiding this comment.
Unfortunatly, this won't work quite how you think it will.
The ActivityTypes.cs is auto-generated from a Swagger file via AutoRest. That means that the moment we re-gen the code, we'll lose the "Delay" value you added.
The real issue is that "Delay" isn't actually part of the protocol yet, which is why it's not being auto-generated via the Swagger file.
If you look at the adapters, there's a special case that looks for delay and effectivly just sleep's for a small period of time.
I know we're hoping to actually add this into the procol, so this client-side hack can be removed, but it's pretty deeply backlogged right now.
We could add this in an ActivityTypesEx.cs, which is where/how we add other similar extensions to the Swagger generated code.
| Activity activity = ((Activity)context.Request).CreateReply(); | ||
| activity.Type = "delay"; | ||
| activity.Value = duration; | ||
| await context.Bot.SendActivity(context, new List<IActivity>() { activity }); |
There was a problem hiding this comment.
I quite like that you've added these - I think they're very helpful.
IF you can seperate them from the constant change in the Swagger layer, I think these are good to take.
Now, to complicate things, I think we're going to change away from the queuing of responses, which are then run through a seperate adapter. It seems simpler all-the-way-around to simply have the Send methods just immediatly do a send.
Doing that will make the framework work more how people expect, meaning we don't have to educate folks that "Send is not really send". That would be a big win.
Until we make that change, however, (this week? next week?) these changes seem totally reasonable.
Thanks~!!
There was a problem hiding this comment.
By separating from swagger do you mean by removing the ActivityTypes.Delay?
There was a problem hiding this comment.
Yes. The way you added the ActivityTypes.Delay just won't work given our auto-generation of the .Schema package and related code.
This code, except for that, looks very reasonable at first glance.
There was a problem hiding this comment.
Okay cool. I have removed the Delay!
|
Okay @cleemullins I will undo the changes from the AcitivityTypes.cs. |
|
To add one more thing, as of earlier today, all outbound activities now automatically have their serviceURL and other properties set. These are applied during the Send processing. You can see the code I added here that's doing this here. |
…he top comment that said auto generated.
… back because there is no flush method." This reverts commit 9cde00e.
|
In case I didn't mention it, thank you. Your help here is really appreciated! 👍 |
|
I'd like to link this and #97 together. |
|
@cleemullins Other than the ActivityTypes.Delay, is there anything else you are wanting me to change? |
|
It's worth noting these changes removed the fluent-API capability of context.Reply("msg 1").ShowTyping().Delay(2000).Reply("msg 2");and must now context.Reply("msg 1");
await context.ShowTyping();
await context.Delay(2000);
context.Reply("msg 2"); |
|
@brandonh-msft that was by design as the feedback was they're confusing and further more they currently complicate the overall lifetime of the context object from a middleware perspective. And Chris may not of applied all of the changes yet but the end goal for the base context object to only have: await context.sendActivities([
{ type: 'message', text: 'msg 1' },
{ type: 'typing' },
{ type: 'delay', value: 2000 },
{ type: 'message', text: 'msg 2' }
]);Does that suck? yes it kind of does... But it's what you get with a unopinionated context object that's been stripped down to its bare basics. The queue based approach can be added back in using middleware and a builder that looks something like this: Batch.add(context).reply(`msg 1`).typing().delay(2000).reply(`msg 2`);I personally think it would be cleaner for the middleware to extend the context object like this: context.batch.reply(`msg 1`).typing().delay(2000).reply(`msg 2`);But that would violate the immutable context object @billba has been pushing for. |
|
not sure how you can have what you've outlined above, in any fashion, in line w/ what this PR did; this PR made |
|
So these were actually direct methods added to the With that said... Yes these methods go direct and I think you'd want one approach or the other (either batch based or direct.) There's a couple of advantages to the batch based approach over the direct approach (fluent style being one and potentially the ability to guarantee order on channels like kick) but I think having both models on the same context object will cause general confusion. We've classically supported the batch based approach as it's the most flexible (or more accurately has the least issues) but I can understand why people hate having to manually call flush(). |
|
I should be clear that moving away from the batched based approach does introduce some issues in certain edge cases like Cortana skills but it's nothing the dev can't work around. We were just able to work around these issues for them before and now we can't. |
I'm not sure if you mean "direct communication" or "direct-on-the-interface" … if the former, yes. if the latter, no - they're extension methods.
100% agree. |
|
@brandonh-msft I meant "direct communication" |
-Add documentation for classes and methods. -Remove multiple blank lines.
When the IActivitie's were being processed ServiceUrl and Conversation were not set. This was preventing the Typing from being sent.
I added the SendActivity method back because there is no flush method in order to force the Typing Activity to show in the chat client.