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

JSON answer : fixed key instead of hotword? #564

Closed
Oliv4945 opened this issue Apr 26, 2017 · 13 comments
Closed

JSON answer : fixed key instead of hotword? #564

Oliv4945 opened this issue Apr 26, 2017 · 13 comments
Assignees

Comments

@Oliv4945
Copy link
Collaborator

Oliv4945 commented Apr 26, 2017

@alexylem, I just understood an issue with the app: I thought that the generated JSON always have the same key (Jarvis) for the answer but after digging into the code I see that the key is the hotword.

I think it is not a good practice to have a variable key in a JSON file: only the content should change, not the key. So I propose to set the key to Jarvis or answer instead.

Obviously if you do not agree I will update the app to query the hotword and use it as the key :-).

@alexylem
Copy link
Owner

Good point, the reason I used the trigger is to display properly the answer in Jarvis-UI plugin. Here is how I parse the answer in javascript:

my.post({
        url: ractive.get ('server_url'),
        data: JSON.stringify (data),
        success: function (messages) {
            $.each (messages, function () {
                $.each (this, function (key, text) {
                    ractive.addMessage (key, text);
                });
            });
            $(".panel-body").animate({ scrollTop: 9999 });
        }
    });

So for each key value pair, I write the key, then : then the value. This way I can also output troubleshooting information:
image
Now for the coloring, in the addmessage I test the key=

addMessage: function (key, text) {
        switch (key) {
            case 'debug':
            case 'warning':
            case 'error':
                type=key;
                break;
            case 'You':
                type='you';
                break;
            default:
                type="jarvis";
        }
        this.push ('messages', {
            type: type,
            key: key,
            text: text
        });

So if not debug, warning or error, I assume it's Jarvis' name so I color it this way (whatever Jarvis name is).
I know could be better by retrieving the trigger name upfront, it was a lazy approach to avoid a call but I can change if needed.

@Oliv4945
Copy link
Collaborator Author

So if not debug, warning or error, I assume it's Jarvis' name so I color it this way (whatever Jarvis name is).

Ok

I know could be better by retrieving the trigger name upfront, it was a lazy approach to avoid a call but I can change if needed.

I will not blame you for being lazy, I am too :)
But, as you already get action=get_config when the page is loaded, it should be really fast to change

Nevertheless I implemented a call to the config and it works: the app will be compatible in next release, so now this issue is just to improve jarvis-core/api code, I let you close the issue if you choose not to do it :-)

@alexylem
Copy link
Owner

Shit you are right, I'm already doing the call anyway...
https://github.com/alexylem/jarvis-ui/blob/master/js/my.js#L255
then I'll change it as suggested 👍

@alexylem alexylem self-assigned this Apr 26, 2017
@Oliv4945
Copy link
Collaborator Author

Oliv4945 commented Apr 26, 2017 via email

@alexylem
Copy link
Owner

Let me just check quickly if this will not break the integration with android wear / tasker that is parsing this JSON already...

@alexylem
Copy link
Owner

@Jean-Bernard-Hallez si tu passes par la on envisage avec @Oliv4945 de changer le retour de Jarvis API de:

[{"Jarvis":"Bonjour"}]

par

[{"answer":"Bonjour"}]

J'imagine que ca va impacter ta procédure pour avoir le retour en synthèse vocale sur une montre android wear. Mais ca pourrait aussi la simplifier dans le sens ou la clé sera toujours "answer" et non pas le hotword de l'utilisateur qui peut varier.
Qu'en penses-tu? Serais-tu prêt à faire la modif dans ta procédure?

@Oliv4945
Copy link
Collaborator Author

Oliv4945 commented Apr 26, 2017 via email

Oliv4945 added a commit to Oliv4945/jarvis-android-app that referenced this issue Apr 26, 2017
Wait for jarvis-core issue [#564](alexylem/jarvis#564) to be closed
@Oliv4945
Copy link
Collaborator Author

Ok, c'est fait: dans la v0.1.4, il y a une sélection du hotword ou "answer" pour avec une bascule transparente lors de ta mise à jour.

@alexylem
Copy link
Owner

Ok je te tiens au courant quand j'ai fait la modif suite à la confirmation de @Jean-Bernard-Hallez

@Jean-Bernard-Hallez
Copy link
Collaborator

Jean-Bernard-Hallez commented Apr 28, 2017 via email

@wikijm wikijm changed the title JSON anwer : fixed key instead of hotword ? JSON answer : fixed key instead of hotword ? Apr 29, 2017
@wikijm wikijm changed the title JSON answer : fixed key instead of hotword ? JSON answer : fixed key instead of hotword? Apr 29, 2017
alexylem added a commit to alexylem/jarvis-ui that referenced this issue Apr 29, 2017
alexylem added a commit to alexylem/jarvis-api that referenced this issue Apr 29, 2017
@Oliv4945
Copy link
Collaborator Author

@alexylem It works well in Jarvis-ui and Android app, thanks ! 👍

@Jean-Bernard-Hallez
Copy link
Collaborator

Jean-Bernard-Hallez commented May 2, 2017

@Oliv4945 et @alexylem Bonjour... un peu absent en ce moment mais je viens de mettre à jour le project Jarvis+Tasker... ;-)

@alexylem
Copy link
Owner

alexylem commented May 2, 2017

@Jean-Bernard-Hallez merci!!

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

No branches or pull requests

3 participants