-
-
Notifications
You must be signed in to change notification settings - Fork 117
Cleanup of VoiceSearchWidget #1886
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: main
Are you sure you want to change the base?
Conversation
4cf600b
to
cd288fd
Compare
Additional checks to ensure that the voice recognition system is always in a consistent state.
Simplify the code of VoiceSearchWidget and make it respond better to state changes. The main change is to add a new State class that represents the state of the view and its associated message for the user. This allows us to simplify the layout while preserving the existing functionality.
cd288fd
to
dfff9e1
Compare
@@ -219,16 +231,17 @@ public void onCanceled() { | |||
@Override | |||
public void onError(@ErrorType int errorType, @Nullable String error) { | |||
Log.e(LOGTAG, "===> ERROR: " + error); | |||
setResultState(errorType); | |||
stopVoiceSearch(); |
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.
Would it make sense to call this in setState() instead? We could easily check if the new state is an error condition and stop the voice search there. Or are there cases in which setting an error state does not imply stopping the voice search?
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.
Currently, all error states stop the voice search.
Some errors are recoverable (e.g. the user remained silent) and some others are actual problems (e.g. the network is unavailable). If we continue evolving the UI, maybe in some cases we could try restarting the voice search.
In any case, the reason for having these two calls here is that setStateforError()
only encapsulates UI changes.
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.
Just a few comments
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.
LGTM. Thanks!
Simplify the code of VoiceSearchWidget and make it respond better to state changes.
The main change is to add a new State class that
represents the state of the view and its associated message for the user. This allows us to simplify the layout while preserving the existing functionality.