Fill displayText in DialogflowResponse body

Carleslc commented 6 years ago

Using the following code:

@PostMapping("/")
fun intentMapper(req: HttpServletRequest, res: HttpServletResponse) {
    DialogflowAction(req, res).handleRequest(mapOf("testAction" to ::test))
}

fun test(action: DialogflowApp) {
    action.tell(speech="Text to speech", displayText="Text to display")
}

the response body fullfilment looks like this:

"fulfillment": {
      "speech": "Text to speech",
      "source": "",
      "displayText": "",   <-------------------------------- SHOULD NOT BE EMPTY
      "messages": [
        {
          "type": 0,
          "speech": "Text to speech"
        }
      ],
      "data": {
        "google": {
          "isSsml": false,
          "noInputPrompts": [],
          "expectUserResponse": false,
          "richResponse": {
            "items": [
              {
                "simpleResponse": {
                  "textToSpeech": "Text to speech",
                  "displayText": "Text to display"
                }
              }
            ],
            "suggestions": []
          }
        }
      }
    }

We can access to the displayText of fulfillment.data.google.richResponse.items[0].simpleResponse.displayText.

However, I was expecting the same text in displayText at fulfillment.displayText like fulfillment.speech, but it is empty.

patjackson52 commented 6 years ago

This matches the behavior of the official SDK, which is the goal of this project. The official Nodejs library will output this:

{
    "speech":"Text to speech",
    "contextOut":[
    ],
        "data":{
        "google":{
        "expect_user_response":false,
        "no_input_prompts":[
    ],
    "rich_response":{
    "items":[
        {
            "simple_response":{
                "text_to_speech":"Text to speech",
                "display_text":"Text to display"
            }
        }
    ],
    "suggestions":[
        ]
    }
    }
 }
}

Why do you need the displayText field to be set? Unlikely that we would make a change to differ from the behavior of the official library, but would like to learn why you would need this.

Carleslc commented 6 years ago

It is a matter of consistency and readability. In your official Nodejs library output there is not a display_text outside simple_response, whereas the current behavior shows an empty displayText in fulfillment, no matter the value set. If I set DialogflowApp displayText I expect to have that value in all the occurrencies of displayText in the output. Moreover, it is clearer to access to fulfillment.speech and fulfillment.displayText instead fulfillment.data.google.richResponse.items[0].simpleResponse.displayText if I do not need to fill a rich response. So, I think it is more consistent to fill in that field, or delete it from the output, rather than having an always-empty value.

patjackson52 commented 6 years ago

Makes total sense. I've updated master with a fix that does not have the empty field. Version 1.6.3 of the artifact has the changes. See PR here: https://github.com/Ticketmaster/actions-on-google-kotlin/pull/46

ticketmaster / actions-on-google-kotlin

Fill displayText in DialogflowResponse body #45