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

Voice Model Dashboard #996

Open
wants to merge 16 commits into
base: feature/voicecommand
Choose a base branch
from

Conversation

devAyushDubey
Copy link

@devAyushDubey devAyushDubey commented Aug 27, 2023

Under GSoC'23

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:

Model Dashboard

Progress:

  • Settings UI & SharedPreferences Changes
  • Model Dashboard Activity UI
  • Model Dashboard Functional Logic
  • Download Model Dialog UI
  • Download Model Dialog Functional Logic
  • VoiceModelNetworkService
  • VoiceViewModel
  • VoiceModelHandler
  • File(Model) Download Logic (Retrofit)

Model Deletetion

  • Delete Model Dialog UI
  • Delete Model Dialog Functionality
  • Model Deletion Logic

Navigation

  • Navigation

A diagrammatic representation of the proposed working of the dashboard:


@SebaDro SebaDro self-requested a review August 30, 2023 11:56
@SebaDro
Copy link
Contributor

SebaDro commented Sep 15, 2023

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?

@devAyushDubey
Copy link
Author

Hey @SebaDro , I've added the navigation and other necessary logic for the Dashboard. Glad if you could make a review. Thanks

Copy link
Contributor

@SebaDro SebaDro left a 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.

Comment on lines 35 to 36
binding.phaseDescriptionTextview.text = "Downloading model..."
binding.multipurposeButton.text = "CANCEL"
Copy link
Contributor

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/")
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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.

@prathamVaidya
Copy link

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.

@devAyushDubey
Copy link
Author

I committed the required changes, @SebaDro let me know if I am missing something. Thanks.

@SebaDro
Copy link
Contributor

SebaDro commented Nov 20, 2023

Hey guys. I will merge the request as soon as possible. Just have a quick look on my last comment .

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

Successfully merging this pull request may close these issues.

None yet

3 participants