Better Culture Recognition in Choice/Confirm Prompts #2463
Conversation
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
|
DO NOT MERGE Please hold off until decision made in this issue. |
mdrichardson
changed the title
|
@cleemullins, @sgellock (CC @mutanttech) Regarding this comment I had some time, so went ahead and de-coupled Basically, it amounts to needing to maintain: This: public static string MapToNearestLanguage(string cultureCode)
{
cultureCode = cultureCode.ToLowerInvariant();
if (SupportedCultureCodes.All(o => o != cultureCode))
{
// Handle cases like EnglishOthers with cultureCode "en-*"
var fallbackCultureCodes = SupportedCultureCodes
.Where(o => o.EndsWith("*", StringComparison.Ordinal) &&
cultureCode.StartsWith(o.Split('-').First(), StringComparison.Ordinal)).ToList();
if (fallbackCultureCodes.Count == 1)
{
return fallbackCultureCodes.First();
}
// If there is no cultureCode like "-*", map only the prefix
// For example, "es-mx" will be mapped to "es-es"
fallbackCultureCodes = SupportedCultureCodes
.Where(o => cultureCode.StartsWith(o.Split('-').First(), StringComparison.Ordinal)).ToList();
if (fallbackCultureCodes.Any())
{
return fallbackCultureCodes.First();
}
}
return cultureCode;
}...which is more or less a copy/paste from This will allow us to easily add additional language support for prompts, without needing to depend on |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
|
@cleemullins, @sgellock (CC @mdrichardson) And also inputs on how long do you think this is gonna take to make into master. As I am running on kinda deadline for a Danish Bot I am working on, so this is something I am depending on to design the whole bot workflow. If it sounds like a week or more, then kindly suggest me a short term solution for the same while getting this longer term in. @mrichardson, please update the translation for "or" as below: @mdrichardson, With the newer files I was able to get the Confirm Prompt with then Danish Text Buttons as given below - Although, when I click "Ja" i.e. "Yes", then confirmation was posted back for execution through AfterConversationContinueAnswerAsync, but break points dont hit, instead it returns the prompt again and with English Buttons.
I think the logic needs to be fixed there. |
|
@mutanttech I made the updates to the Danish "or". Thanks again for the translation. I didn't experience the same issue that you had and the confirm prompt returned the appropriate
(ignore some of the other stuff...this is just my Quick Test bot). I'm 90% sure the issue is that you're manually setting the locale with: stepContext.Context.Activity.Locale = "da-DK";I believe what's happening is that you're only setting the locale on the activity that creates the prompt, but not the reply. You should actually be setting it for the entire conversation for the user. In Emulator, you do that in Settings:
In WebChat, you do that like: window.WebChat.renderWebChat(
{
directLine: window.WebChat.createDirectLine({
token: 'YOUR_DIRECT_LINE_TOKEN'
}),
userID: 'YOUR_USER_ID',
username: 'Web Chat User',
locale: 'da-DK', // HERE
botAvatarInitials: 'WC',
userAvatarInitials: 'WW'
},
document.getElementById('webchat')
);I can't speak for the dev team, but if accepted, I'd guess this would go in our 4.6 release, which we're targeting to have out by ~November. There may be some nightly releases you could get from our MyGet feed if/when the PR is accepted. The alternative would be downloading the SDK and building it into your bot until this is released officially, at which time you'd replace your local build with the official release. |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
mdrichardson
changed the title
libraries/Microsoft.Bot.Builder.Dialogs/Prompts/IPromptCultureModel.cs
Outdated
Show resolved
Hide resolved
|
@mdrichardson , And instead of getting the whole SDK or event Dialogs package, I have picked the three files that you have created in culture recognizer branch with same namespace, just renamed them to ChoicePromptCustom and ConfirmPromptCustom. And in Bot Dialog add them. It seems to work as expected. I will upgrade to 4.6 when it arrives and get rid of these additional files. Let me know if you think any other thing might be required in addition to those files. |
mdrichardson
dismissed
cleemullins’s stale review
Implemented requested changes
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
mdrichardson
changed the title
|
@cleemullins This is a long thread, so I'll re-summarize the PR. Changes are made in both Confirm and Choice prompts.
Pending approval, I'll work up the same thing for JS/Python. |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
libraries/Microsoft.Bot.Builder.Dialogs/Prompts/ChoicePrompt.cs
Outdated
Show resolved
Hide resolved
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
libraries/Microsoft.Bot.Builder.Dialogs/Prompts/PromptCultureModels.cs
Outdated
Show resolved
Hide resolved
libraries/Microsoft.Bot.Builder.Dialogs/Prompts/PromptCultureModel.cs
Outdated
Show resolved
Hide resolved
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.5.3 |
mdrichardson
force-pushed
the
cultureRecognizer
branch
from
d6bf4e4
to
d986977
Compare
|
@cleemullins I don't think I'll be able to code review/sync in person until next week (out sick). Happy to then, if you want. |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.5.3 |
|
This looks good to me, but I would like
|
mdrichardson
changed the title
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.5.3 |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.5.3 |
mdrichardson
changed the title
|
I believe this gives us partiy in both C# and JS at this point. I think what's here is reasonable. I'll get one more set of eyes on it, and then we can merge. |
| /// </summary> | ||
| public class ChoicePrompt : Prompt<FoundChoice> | ||
| { | ||
| private static readonly Dictionary<string, ChoiceFactoryOptions> DefaultChoiceOptions = new Dictionary<string, ChoiceFactoryOptions>(StringComparer.OrdinalIgnoreCase) |
There was a problem hiding this comment.
This is a breaking change...
Fixes: #2458
Summary
Mostly, this brings the dotnet repo to parity with JS due to this PR: microsoft/botbuilder-js#1121
However, I also noticed that we duplicate some code and hardcode some strings like:
ConfirmPrompt:
ChoicePrompt:
Instead, I made an interface and class to house all of the languages Confirm and Choice Prompts support. This reduces the number of places needed up update or add a language and will also make it so the prompts dynamically support new languages that we add.
I considered using the PromptCultureModels class more in the tests, but I thought the tests might be better at catching errors if they kept hard-coded strings. Let me know if you disagree and I should modify the tests to use the -CultureModels
Like the JS PR, this tests variations of locale strings
Let me know what you think of the additional Interface and Class and I can duplicate it in JS or remove it from this PR
Changes
Changed
Added
Testing
Working in Bot: