-
-
Notifications
You must be signed in to change notification settings - Fork 112
Adds in app search #323
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
base: master
Are you sure you want to change the base?
Adds in app search #323
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is not a thorough review as I don't have time for it right now, but will come back to it after I'm done with university exams.
One thing we might want to consider is to implement something like an OrConstruct that also acts as a capturing group and allows matching only known services (i.e. spotify, youtube, ...).
I would also suggest adding the "duckduckgo" service that just redirects the query to the standard search skill.
import org.stypox.dicio.sentences.Sentences.AppSearch | ||
import org.stypox.dicio.util.StringUtils | ||
|
||
class AppSearchSkill(correspondingSkillInfo: SkillInfo, data: StandardRecognizerData<AppSearch>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that we need another skill, maybe it's better to extend the search skill that already exists. The search skill however has a low specificity, while this one has a high specificity at the moment (although I would suggest a medium specificity would be better), so we need to see how it plays out, maybe it is a good idea to have two separate skills.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could condense these into "in|on .app. (search for?)|lookup|(look up|for)|find .query.". You could also add another sentence with "in|on .app." at the end, the sentence recognizer should still be able to distinguish it from searching in general: "(search for?)|lookup|(look up|for)|find .query. in|on .app.".
@@ -1,4 +1,14 @@ | |||
skills: | |||
- id: app_search | |||
specificity: high |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specificity: high | |
specificity: medium |
Thanks for reviewing this, I'll add the changes as requested over the next week or so 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hunterwilhelm I finally completed the review, thanks again!
One thing we might want to consider is to implement something like an OrConstruct that also acts as a capturing group and allows matching only known services (i.e. spotify, youtube, ...).
Here is how I think we should add support for this, in an even more general way. Imagine being able to assign a "label" to a part of the sentence, that gets "activated" when that part of the sentence matches with the user input. A sentence with "label"ed parts might look like "search for .query. on (YouTube:youtube)|(Spotify:spotify)", where youtube and spotify are the tags. The natural language processing module would then output a boolean value for each of the labels indicating whether it was observed or not in the user input while matching that sentence. This would enable us to parse YouTube, Spotify, and more without having to create different sentences and without having to use a generic capturing group. Futhermore, since it is a very general way to assign labels, it would possibly also be useful for other nuanced behaviors in other skills.
Here is a rough outline of how this would be implemented. If you want to do it, great, let me know if you have any questions! Otherwise let me know and I can do it myself, I understand it would be a quite deep dive into the project and would require making many choices along the way.
- modify the
sentences-compiler-plugin
submodule so that it can parse a new type ofcaptures:
(e.g. call ittype: bool
ortype: label
). This would mean your- id: app \ type: string
would turn into- id: youtube \ type: label \ - id: spotify \ type: label
(by \ I mean newline) - modify
dicio-sentences-compiler
so that it can parse labels inside sentences. I think the:abc
syntax to label the thing to the left with the "abc" label would make sense, however you would probably need to remove some old now-unused code that used : in another way (as a section name delimiter, but sections are not used bysentences-compiler-plugin
anymore, though they are still in the code ofdicio-sentences-compiler
). - modify the
skill
submodule to add a new type of construct (e.g. call itLabelledConstruct
) that just wraps another construct (any construct), and insidematchToEnd
first calls the wrapped construct'smatchToEnd
, then checks at which places thememToEnd
changed (those would be the places where the wrapped construct matched better than other stuff so far), and finally adds capture information to theStandardScore
objects at those places. - finally modify
sentences-compiler-plugin
to use the new version ofdicio-sentences-compiler
and to generate Kotlin code that uses the newly addedLabelledConstruct
class AppSearchOutput( | ||
private val appName: String?, | ||
private val packageName: String?, | ||
private val searchQuery: String?, | ||
private val success: Boolean | ||
) : SkillOutput { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having a class with nullable fields, you can create a sealed interface
with one class for each possible outcome. It's not so necessary, but it might make things a bit more clear. See how it's done e.g. for the Lyrics, there is just one failure mode there while here you would have three. Also, to avoid having to implement GraphicalOutput
multiple times, you can make the error classes override HeadlineSpeechSkillOutput
.
<string name="skill_name_app_search">App Search</string> | ||
<string name="skill_sentence_example_app_search">In YouTube search for cute cats</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<string name="skill_name_app_search">App Search</string> | |
<string name="skill_sentence_example_app_search">In YouTube search for cute cats</string> | |
<string name="skill_name_app_search">Search in apps</string> | |
<string name="skill_sentence_example_app_search">Search for cute cats on YouTube</string> |
@Stypox I'll let you take this over since it's more involved. Thank you as well for the time to genuinely look at this :) I only have one thought to share, I did run into issues with "Search for cute cats on YouTube" because it would conflict with the searching on the web instead of a regular search engine. So, being able to handle prefixes and suffixes (e.g. "search ... on Youtube" and "On youtube search for ..." for the target application would be nice. |
"In YouTube search for cute cats"
"In Spotify look up Discover Weekly"