[Voice] Allow hli list #2906

GiviMAD · 2022-04-10T09:51:17Z

Signed-off-by: Miguel Álvarez Díez miguelwork92@gmail.com

This is a proposed solution for #2904 based on the feedback from @lolodomo.

Basically allows to pass a comma separate list of hli ids instead of a single one.

~~I have some doubts about renaming the hli vars, and the 'getHLI' methods, as there is already a getHLIs method that returns all the available interpreters, let me know if it's better to rename them.~~

GiviMAD · 2022-04-10T10:05:26Z

ups, adding WIP

lolodomo · 2022-04-10T10:28:18Z

You should keep getHLI unchanged and add a new method taking a comma separated list of IDs and returning a list of HLI. .

Note the typo: comma separated list

GiviMAD · 2022-04-10T10:53:03Z

You should keep getHLI unchanged and add a new method taking a comma separated list of IDs and returning a list of HLI

Ok, but there will be no uses for the 'getHLI(String id)' method other than inside the own VoiceManager, as all the places will call the new method 'getHLIByIds(String idList)'. Is this ok?

Edit: Done. I have added 2 new methods at the end because another one was needed to replace the 'getHLI()' method call in some places.

lolodomo · 2022-04-10T11:24:52Z

I think it is better, it avoids breaking the interface and it avoids having a method name not matching what it does.

GiviMAD · 2022-04-10T11:39:53Z

I think it is better, it avoids breaking the interface and it avoids having a method name not matching what it does.

Yes it makes sense, specially the last part, thanks for the explanation.

GiviMAD · 2022-04-10T11:43:50Z

I forgot to apply spotless last time.

GiviMAD · 2022-04-10T12:33:12Z

Now is ready for review.

....core.io.rest.voice/src/main/java/org/openhab/core/io/rest/voice/internal/VoiceResource.java

...s/org.openhab.core.voice/src/main/java/org/openhab/core/voice/internal/VoiceManagerImpl.java

GiviMAD · 2022-04-17T09:32:10Z

@lolodomo I have solve part of the review.
Can you explain me the reason why you think is better to support just one default interpreter while allowing to use multiples? For me it breaks a little the default functionality as if you want to always use multiple interpreters (this is my case) you can not rely on the default value anymore.

GiviMAD · 2022-04-26T13:31:38Z

@lolodomo I have finally followed your advice so I can complete the review.
I'll open an issue on the future to discuss there if it is a good idea to support multiple interpreters as default or not.

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

lolodomo

Still few comments.

....core.io.rest.voice/src/main/java/org/openhab/core/io/rest/voice/internal/VoiceResource.java

bundles/org.openhab.core.voice/src/main/java/org/openhab/core/voice/VoiceManager.java

...s/org.openhab.core.voice/src/main/java/org/openhab/core/voice/internal/VoiceManagerImpl.java

...es/org.openhab.core.voice/src/main/java/org/openhab/core/voice/internal/DialogProcessor.java

lolodomo · 2022-04-27T10:00:40Z

@GiviMAD : most of my remaining comments are about documentation, I think there is not too much work to finish.

J-N-K

Sorry for being late to the party. I have one general question (which is probably due to me missing knowledge on this part, I never used HLI): If there are multiple HLI and they are all listed, the first result is taken. Could it happen that the user is interested in the result of another HLI? Since Collection in general does not guarantee order, the user can't control which HLI is queried first.

....core.io.rest.voice/src/main/java/org/openhab/core/io/rest/voice/internal/VoiceResource.java

bundles/org.openhab.core.model.script/src/org/openhab/core/model/script/actions/Voice.java

bundles/org.openhab.core.voice/src/main/java/org/openhab/core/voice/VoiceManager.java

...es/org.openhab.core.voice/src/main/java/org/openhab/core/voice/internal/DialogProcessor.java

lolodomo · 2022-04-27T13:19:44Z

If there are multiple HLI and they are all listed, the first result is taken. Could it happen that the user is interested in the result of another HLI? Since Collection in general does not guarantee order, the user can't control which HLI is queried first.

The order was very important. The user just wants to get the result of the first interpreter able to interpret, considering an ordered list of interpreters.
So Collection was a bad choice. I have this question in mind and finally totally forgot it !

lolodomo · 2022-04-27T13:53:34Z

Considering @J-N-K comments, I think we should duplicate REST API and actions for "interpret", keeping the existing ones unchanged. That means, we forget the idea of a string containing a coma separated list of ids.

For "startDialog" and "listenAndAnswer", as this is recent features added, we can directly consider the list of interpreters as parameter without duplicating them.

For REST API, having a List as parameter is not a problem.
But for actions, will it be possible to pass a list of strings as parameter in a rule ? What would be the syntax in a DSL rule for example ?

And finally, is List the right class for an ordered list ?

....core.io.rest.voice/src/main/java/org/openhab/core/io/rest/voice/internal/VoiceResource.java

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

J-N-K

Please also adapt the documentation: /~https://github.com/openhab/openhab-docs/blob/main/configuration/multimedia.md

....core.io.rest.voice/src/main/java/org/openhab/core/io/rest/voice/internal/VoiceResource.java

bundles/org.openhab.core.voice/src/main/java/org/openhab/core/voice/VoiceManager.java

...es/org.openhab.core.voice/src/main/java/org/openhab/core/voice/internal/DialogProcessor.java

...s/org.openhab.core.voice/src/main/java/org/openhab/core/voice/internal/VoiceManagerImpl.java

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

bundles/org.openhab.core.voice/src/main/java/org/openhab/core/voice/VoiceManager.java

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

lolodomo

Final review

bundles/org.openhab.core.voice/src/main/java/org/openhab/core/voice/VoiceManager.java

....core.io.rest.voice/src/main/java/org/openhab/core/io/rest/voice/internal/VoiceResource.java

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

lolodomo

LGTM, thank you @GiviMAD
The parameters for REST API that have been marked as deprecated could be discussed as these REST API were added very recently and are only available in 3.3 snapshots and 3.3 milestones.

lolodomo · 2022-05-10T18:35:06Z

@GiviMAD ; is it clear for you what must be kept as deprecated and what must be removed?

GiviMAD · 2022-05-10T19:06:36Z

@GiviMAD ; is it clear for you what must be kept as deprecated and what must be removed?

@lolodomo I think so, I'll go for it.

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

lolodomo · 2022-05-10T20:18:46Z

Perfect IMHO

lolodomo · 2022-05-11T06:34:33Z

Once merged, I will propose a big update in the documentation to add/update all existing actions and console commands about voice management.

J-N-K

Thanks.

This fixes the null type errors in Eclipse which were introduced by openhab#2906. Signed-off-by: Wouter Born <github@maindrain.net>

This fixes the null type errors in Eclipse which were introduced by #2906. Signed-off-by: Wouter Born <github@maindrain.net>

* [Voice] Allow hli list Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com> GitOrigin-RevId: 5753627

This fixes the null type errors in Eclipse which were introduced by openhab#2906. Signed-off-by: Wouter Born <github@maindrain.net> GitOrigin-RevId: 86ee4dc

GiviMAD requested a review from a team as a code owner April 10, 2022 09:51

GiviMAD changed the title ~~[Voice] Allow hli list~~ [WIP][Voice] Allow hli list Apr 10, 2022

GiviMAD force-pushed the voice/hli_chain branch from 100b934 to d35048f Compare April 10, 2022 10:25

GiviMAD force-pushed the voice/hli_chain branch from d35048f to 3d91d97 Compare April 10, 2022 10:45

GiviMAD force-pushed the voice/hli_chain branch from 3d91d97 to b2fd89d Compare April 10, 2022 11:34

GiviMAD force-pushed the voice/hli_chain branch from b2fd89d to 50f9d3d Compare April 10, 2022 11:43

GiviMAD force-pushed the voice/hli_chain branch from 50f9d3d to 0908a81 Compare April 10, 2022 11:45

GiviMAD changed the title ~~[WIP][Voice] Allow hli list~~ [Voice] Allow hli list Apr 10, 2022

lolodomo suggested changes Apr 16, 2022

View reviewed changes

wborn added the enhancement An enhancement or new feature of the Core label Apr 22, 2022

GiviMAD force-pushed the voice/hli_chain branch from 43fb931 to f48e791 Compare April 26, 2022 13:27

GiviMAD force-pushed the voice/hli_chain branch 2 times, most recently from 48cc12f to 848eea0 Compare April 26, 2022 15:39

[Voice] Allow hli list

3ff5086

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

GiviMAD force-pushed the voice/hli_chain branch from 848eea0 to 3ff5086 Compare April 26, 2022 16:22

GiviMAD requested a review from lolodomo April 26, 2022 18:36

lolodomo suggested changes Apr 27, 2022

View reviewed changes

J-N-K requested changes Apr 27, 2022

View reviewed changes

....core.io.rest.voice/src/main/java/org/openhab/core/io/rest/voice/internal/VoiceResource.java Outdated Show resolved Hide resolved

....core.io.rest.voice/src/main/java/org/openhab/core/io/rest/voice/internal/VoiceResource.java Outdated Show resolved Hide resolved

apply pr review

009c8bf

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

GiviMAD requested a review from J-N-K April 27, 2022 16:21

J-N-K requested changes Apr 28, 2022

View reviewed changes

apply pr review

a575dd9

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

GiviMAD mentioned this pull request Apr 28, 2022

[interpret action] document use multiple interpreters openhab/openhab-docs#1804

Closed

GiviMAD requested a review from J-N-K April 28, 2022 19:22

J-N-K reviewed May 1, 2022

View reviewed changes

bundles/org.openhab.core.voice/src/main/java/org/openhab/core/voice/VoiceManager.java Outdated Show resolved Hide resolved

apply pr review

60fd793

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

GiviMAD force-pushed the voice/hli_chain branch from 0b08c2c to 60fd793 Compare May 5, 2022 19:40

GiviMAD requested a review from J-N-K May 5, 2022 19:41

lolodomo suggested changes May 7, 2022

View reviewed changes

apply pr review

44c0d21

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

lolodomo approved these changes May 7, 2022

View reviewed changes

apply pr review

67d2922

Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com>

GiviMAD requested a review from lolodomo May 10, 2022 19:38

J-N-K approved these changes May 11, 2022

View reviewed changes

J-N-K merged commit 5753627 into openhab:main May 11, 2022

This was referenced May 16, 2022

[HumanLanguageInterpreter] Allow HLI chaining #2904

Closed

[voice] Add voice parameter to startDialog and listenAndAnswer #2971

Merged

wborn added a commit to wborn/openhab-core that referenced this pull request May 23, 2022

Fix null type mismatch errors

980a7a7

This fixes the null type errors in Eclipse which were introduced by openhab#2906. Signed-off-by: Wouter Born <github@maindrain.net>

wborn mentioned this pull request May 23, 2022

Fix null type mismatch errors #2976

Merged

J-N-K pushed a commit that referenced this pull request May 23, 2022

Fix null type mismatch errors (#2976)

86ee4dc

This fixes the null type errors in Eclipse which were introduced by #2906. Signed-off-by: Wouter Born <github@maindrain.net>

wborn added this to the 3.3 milestone Jun 23, 2022

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023

[voice] Allow list of HLIs in dialog function (openhab#2906)

5516e59

* [Voice] Allow hli list Signed-off-by: Miguel Álvarez Díez <miguelwork92@gmail.com> GitOrigin-RevId: 5753627

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Voice] Allow hli list #2906

[Voice] Allow hli list #2906

GiviMAD commented Apr 10, 2022 •

edited

Loading

GiviMAD commented Apr 10, 2022

lolodomo commented Apr 10, 2022

GiviMAD commented Apr 10, 2022 •

edited

Loading

lolodomo commented Apr 10, 2022

GiviMAD commented Apr 10, 2022

GiviMAD commented Apr 10, 2022

GiviMAD commented Apr 10, 2022

GiviMAD commented Apr 17, 2022

GiviMAD commented Apr 26, 2022

lolodomo left a comment

lolodomo commented Apr 27, 2022

J-N-K left a comment •

edited

Loading

lolodomo commented Apr 27, 2022 •

edited

Loading

lolodomo commented Apr 27, 2022 •

edited

Loading

J-N-K left a comment

lolodomo left a comment

lolodomo left a comment

lolodomo commented May 10, 2022

GiviMAD commented May 10, 2022

lolodomo commented May 10, 2022

lolodomo commented May 11, 2022

J-N-K left a comment

[Voice] Allow hli list #2906

[Voice] Allow hli list #2906

Conversation

GiviMAD commented Apr 10, 2022 • edited Loading

GiviMAD commented Apr 10, 2022

lolodomo commented Apr 10, 2022

GiviMAD commented Apr 10, 2022 • edited Loading

lolodomo commented Apr 10, 2022

GiviMAD commented Apr 10, 2022

GiviMAD commented Apr 10, 2022

GiviMAD commented Apr 10, 2022

GiviMAD commented Apr 17, 2022

GiviMAD commented Apr 26, 2022

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo commented Apr 27, 2022

J-N-K left a comment • edited Loading

Choose a reason for hiding this comment

lolodomo commented Apr 27, 2022 • edited Loading

lolodomo commented Apr 27, 2022 • edited Loading

J-N-K left a comment

Choose a reason for hiding this comment

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo commented May 10, 2022

GiviMAD commented May 10, 2022

lolodomo commented May 10, 2022

lolodomo commented May 11, 2022

J-N-K left a comment

Choose a reason for hiding this comment

GiviMAD commented Apr 10, 2022 •

edited

Loading

GiviMAD commented Apr 10, 2022 •

edited

Loading

J-N-K left a comment •

edited

Loading

lolodomo commented Apr 27, 2022 •

edited

Loading

lolodomo commented Apr 27, 2022 •

edited

Loading