actions-on-google / actions-on-google-java

Java/Kotlin library for Actions on Google
Apache License 2.0
286 stars 39 forks source link

Improve @ForIntent Annotation #31

Open cbeaujoin opened 5 years ago

cbeaujoin commented 5 years ago

Hi,

for the moment we can easily handle Intent by their name: @ForIntent("Intent Name")

As we often use webhook call for slot filling, it could be interesting to add a second parameter to the annotation that would handle the corresponding slot filling. For example : @ForIntent("Intent Name", "Parameter Name")

For the moment as a workaround I do like this :

 // Choose between slot filling
List<Context> outputContexts = request.getWebhookRequest().getQueryResult().getOutputContexts();
Optional<Context> param = outputContexts.stream().filter(context -> context.getName().contains("intent_name_dialog_params")).findFirst();
if (param.isPresent()) {
     if (param.get().getName().contains("parameter_name")) {
          // do this ..
      } else {
          ...
      }
}
taycaldwell commented 5 years ago

Hi @cbeaujoin,

Thank you for your feature request! We are always looking on ways to improve our client libraries, so we appreciate feedback and suggestions for the community.

spenc53 commented 5 years ago

Should it only check for 1 parameter? Or should it also be able to take in a list of parameters?

spenc53 commented 5 years ago

Also, as a follow up for this, maybe at some point, add another annotation for @Param("paramName") paramName: string for functions to pass in data? Probably won't be part of this issue, might be out of scope.

Fleker commented 5 years ago

If you do support a list of parameters, would your annotation support a comma-separated list of args or an array primitive?

@ForIntent("Intent Name", "Param Name 1", "Param Name 2", ...)

or

@ForIntent("Intent Name", ["Param Name 1", "Param Name 2"])

spenc53 commented 5 years ago

It could probably go either way on the array formatting. I think that an actual array would be more explicit. Also, to get which parameters to check,how should we pick which name from the outputContext to choose? Or do we look at the parameters for all of them? I provided the snippet below to show the output contexts and their names.


      {
        "name": "projects/projectId/agent/sessions/sessionId/contexts/choose_fact-followup",
        "lifespanCount": 5,
        "parameters": {
          "category.original": "",
          "category": "history"
        }
      },
      {
        "name": "projects/projectId/agent/sessions/sessionId/contexts/_actions_on_google",
        "lifespanCount": 98,
        "parameters": {
          "data": "{\"headquarters\":[\"google_headquarters_fact_1\",\"google_headquarters_fact_2\",\"google_headquarters_fact_3\"],\"cats\":[\"cat_fact_1\",\"cat_fact_2\",\"cat_fact_3\"],\"history\":[],\"test\":\"hello\"}",
          "category.original": "",
          "category": "history"
        }
      }
    ]```
Fleker commented 5 years ago

You mean between "category" and "category.original"?

spenc53 commented 5 years ago

Oh, sorry, I should've explained better.

The @ForIntent("Intent Name") matches the top level intent name. For the specific outputContexts, they also have names that are specific, will the parameters be the same for those?

Fleker commented 5 years ago

You mean the parameter names, or the names of the context?

spenc53 commented 5 years ago

Name of the context

On Mon, Oct 14, 2019 at 11:33 AM Nick notifications@github.com wrote:

You mean the parameter names, or the names of the context?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/actions-on-google/actions-on-google-java/issues/31?email_source=notifications&email_token=AF7GLLLDTRHXWP2UVUZATTLQOSNMDA5CNFSM4IUY6AWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBFPEQY#issuecomment-541782595, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF7GLLM725JVBW5ZTU7IPYTQOSNMDANCNFSM4IUY6AWA .

Fleker commented 5 years ago

I guess choosing either the first (exit early) or the last (replace) would be the primary expected way to do it. Or maybe throw an exception if not specific enough.

There are many different ways to do it which could all be potentially valid.

spenc53 commented 5 years ago

Would the user also need to specify the context type name then (just the last part)? And if they didn't care which it hit, leave it empty and it will default to match the first one that has all the parameters, if any?

Fleker commented 5 years ago

Well I don't know whether they'd need to specify the context always. Certainly it's something you may need. But what do you think the expected behavior should be if it's empty?

cbeaujoin commented 5 years ago

Hi,

I don't think we need to support a list of parameters. A second parameter (a String) is enough.

First let me add my understanding of the contexts names :

With actions we have a context at conversation level :

For a given intent "intent" whatever the number of filled params there is parent context :

For a given intent "intent" with a required parameter "cellphone" not filled and in first position in the required parameters sorted list. There is a context :

If there is no context that contains "_dialogparams" it means we have reach the intent's end.

If we apply this to the annotation :

@ForIntent("Intent Name", "cellphone") If a context that contains "intent_dialog_params_cellphone" exists :

@ForIntent("Intent Name") => with an empty/null second parameters If there is no context that contains "_dialogparams":

or there is a _dialogparams but it does not match any @ForIntent("Intent Name", "param")