Add connectivity status check to SpeechPlayer provider#1901
Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1901 +/- ##
===========================================
+ Coverage 36.23% 36.33% +0.1%
- Complexity 1085 1087 +2
===========================================
Files 261 261
Lines 8860 8874 +14
Branches 668 668
===========================================
+ Hits 3210 3224 +14
+ Misses 5360 5358 -2
- Partials 290 292 +2 |
Contributor
Guardiola31337
left a comment
Guardiola31337
left a comment
There was a problem hiding this comment.
This is looking good so far @danesfeder
I've left some comments to 👀 before merging. Great test coverage ❤️
| SpeechPlayerProvider(@NonNull Context context, String language, | ||
| boolean voiceLanguageSupported, VoiceInstructionLoader voiceInstructionLoader, | ||
| ConnectivityStatusProvider connectivityStatus) { | ||
| initialize(context, language, voiceLanguageSupported, voiceInstructionLoader); |
Contributor
There was a problem hiding this comment.
Could this be this(context, language, voiceLanguageSupported, voiceInstructionLoader); instead?
|
|
||
| private void initMapboxSpeechPlayer(Context context, String language, boolean voiceLanguageSupported, | ||
| SpeechListener listener, VoiceInstructionLoader voiceInstructionLoader) { | ||
| private void initializeMapboxSpeechPlayer(Context context, String language, boolean voiceLanguageSupported, |
Contributor
There was a problem hiding this comment.
Thanks for adding naming consistency here ❤️
|
|
||
| private void initAndroidSpeechPlayer(Context context, String language, | ||
| SpeechListener listener) { | ||
| private void initializeAndroidSpeechPlayer(Context context, String language, |
Contributor
There was a problem hiding this comment.
Thanks for adding naming consistency here ❤️
| } | ||
|
|
||
| public void cacheInstructions(List<String> instructions) { | ||
| if (!connectivityStatus.isConnected()) { |
Contributor
There was a problem hiding this comment.
Wondering if we could move this check up in the stack
VoiceInstructionLoader#cacheInstructions 🤔
danesfeder
force-pushed
the
dan-voice-connectivity
branch
2 times, most recently
from
d58898d to
b43eece
Compare
Guardiola31337
approved these changes
Apr 25, 2019
Contributor
Guardiola31337
left a comment
Guardiola31337
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback @danesfeder
🚢 as soon as CI is ✅
danesfeder
force-pushed
the
dan-voice-connectivity
branch
from
b43eece to
bf3d067
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #1842
bug,feature,new API(s),SEMVER, etc.)Goal
As described in #1842, we rely on failed Voice API requests for our fallback to the
AndroidSpeechPlayeror "offline voice". The goal of this PR is to check connection upfront so that we don't worry about the API request or have to deal with the latency of the failed request.Implementation
Make
ConnectivityStatusProviderpublicso we can share it between the router / voice instruction loader, etc. We should definitely make thisinternalas soon as the package work lands. I've added a javadoc noteTODOas a reminder.Testing
Please describe the manual tests that you ran to verify your changes
SNAPSHOTupstream dependencies if needed)Checklist