-
Notifications
You must be signed in to change notification settings - Fork 158
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
Voice Model Dashboard #996
base: feature/voicecommand
Are you sure you want to change the base?
Voice Model Dashboard #996
Conversation
…into fix/wakeword
I'd really like to merge the Dashboard to the app. However, the navigation path is still missing, so the dashboard can not be reached by users. Are there any updates on this pending task? |
Hey @SebaDro , I've added the navigation and other necessary logic for the Dashboard. Glad if you could make a review. Thanks |
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.
Thanks for finishing the Model Download Dashboard. The implementation looks good to me. Just some small adjustments accroding to the inline comments and your PR is ready for being merged.
binding.phaseDescriptionTextview.text = "Downloading model..." | ||
binding.multipurposeButton.text = "CANCEL" |
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'd better use String resources here and all other places where you used hard coded Strings. In particular, that applies to the model download ProgressDialog. With hard coded Strings we can not support multi languages
|
||
|
||
private val retrofit = Retrofit.Builder() | ||
.baseUrl("http://ec2-15-206-186-192.ap-south-1.compute.amazonaws.com:8000/") |
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 should make this URL configurable, since it may change in the future.
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.
Sure thing, I will just do that.
Should I store it as an environment variable (I would prefer this, as keeping the address public would have security concerns) or a string resource?
Or maybe just a BASE_URL variable as is the enviroCar API base address?
Let me know your thoughts.
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.
Sorry for the late response. I'd prefer to introduce an environment variable for it. I noticed, you have already commited the BASE_URL
approach. If you are fine with having your own AWS service linked here, I will merge the request immediately. Otherwise, I would also wait if you want to implement the environment variable solution e.g. via gradle.properties
.
Hey, @devAyushDubey! Any idea when this PR will be merged? I wanted to start working on the Voice feature, and since this is an important PR, I think it would be better if I start looking for tasks after it gets merged. Also, @SebaDro let me know if any other tasks that I can work on in parallel to this PR. |
I committed the required changes, @SebaDro let me know if I am missing something. Thanks. |
Hey guys. I will merge the request as soon as possible. Just have a quick look on my last comment . |
Introduction of Voice Model Dashboard in the VoiceCommands module
Expanding the usability of the voice command feature, we introduced Voice Model Dashboard with the primary objective of reducing the app size by not packaging the bulky model files with the app but rather remotely delivering them while also giving more model options and control to the user.
Here are some screen captures of the UI implemented in the app:
Progress:
Model Deletetion
Navigation
A diagrammatic representation of the proposed working of the dashboard: