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.

Sign up for GitHub

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

Jump to bottom

Fix voice instructions cache #1481

Merged
merged 1 commit into from
Nov 13, 2018
Merged

Fix voice instructions cache #1481

merged 1 commit into from
Nov 13, 2018

Conversation

Guardiola31337
Copy link
Contributor

@Guardiola31337 Guardiola31337 commented Oct 30, 2018 •
edited
Loading

  • Fixes VoiceInstructionLoader not caching ahead voice instructions

TODOs:

  • Clean up code / refactor
    • Remove unnecessary DEBUG log calls
    • Get the voice code out of core and get all that code out of the processor
      • Listening the milestones from ui? Custom progress change listener?
    • Break some logic out to make it testable
      • Add tests
  • Remove snapshot repository from root project's build.gradle before merging
  • Bump MAS version when 4.1.0 gets released 👀 Add interceptor and network interceptor support to Mapbox Speech mapbox-java#910
  • Implement a better cache management strategy from our side?
    • Right now, caches at the beginning and then on every 5 instructions announced + removing the previous 4 instructions cached. We're requesting voice instructions only once, we should consider increasing API calls though.

@Guardiola31337 Guardiola31337 added bug Defect to be fixed. ⚠️ DO NOT MERGE PR should not be merged! blocked: upstream backwards incompatible Requires a SEMVER major version change. labels Oct 30, 2018
@Guardiola31337 Guardiola31337 self-assigned this Oct 30, 2018
danesfeder
danesfeder reviewed Oct 31, 2018
View reviewed changes
Copy link
Contributor

@danesfeder danesfeder left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Guardiola31337 great digging here - as a first pass, most of my comments are already noted by your checklist 👍 please follow up once they are addressed, thanks!

@Guardiola31337 Guardiola31337 force-pushed the pg-fix-voice-instructions-cache branch 4 times, most recently from 11c5486 to 31a8c00 Compare November 7, 2018 22:11
@Guardiola31337
Copy link
Contributor Author

This is updated addressing almost all the TODOs from the checklist and ready for a first round of 👀 @danesfeder @devotaaabel

@Guardiola31337 Guardiola31337 changed the title [DO NOT MERGE] Fix voice instructions cache Fix voice instructions cache Nov 7, 2018
@Guardiola31337 Guardiola31337 added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Nov 7, 2018
danesfeder
danesfeder reviewed Nov 8, 2018
View reviewed changes
Copy link
Contributor

@danesfeder danesfeder left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Guardiola31337 this is looking great, just a minor javadoc update for now.

I'll take a second pass once tests are added for VoiceInstructionCache. Are we able to test VoiceInstructionLoader as well? I think the logic in evictVoiceInstructions() is pretty critical.

...ion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigation.java
@@ -716,6 +716,10 @@ public void toggleHistory(boolean isEnabled) {
mapboxNavigator.toggleHistory(isEnabled);
}

public String retrieveSsmlAnnouncementInstruction(int index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add some javadoc for this new API

@Guardiola31337 Guardiola31337 force-pushed the pg-fix-voice-instructions-cache branch 4 times, most recently from 37cc1b1 to 4f0cdcf Compare November 9, 2018 18:14
@Guardiola31337
Copy link
Contributor Author

@danesfeder this is updated and ready for another round of 👀

danesfeder
danesfeder approved these changes Nov 12, 2018
View reviewed changes
Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Guardiola31337 this is great, thanks for adding tests for the cache logic you updated. Going to go ahead and ✅ but let's hold on merging until the java library is released. 👍

@Guardiola31337
Copy link
Contributor Author

Going ahead and merging here as MAS 4.1.0 was released 🎉

Thanks for the great feedback @danesfeder

@Guardiola31337 Guardiola31337 merged commit ec262af into master Nov 13, 2018
@Guardiola31337 Guardiola31337 deleted the pg-fix-voice-instructions-cache branch November 13, 2018 14:16
@Guardiola31337 Guardiola31337 mentioned this pull request Nov 15, 2018
Closed
@Guardiola31337 Guardiola31337 mentioned this pull request Nov 30, 2018
Closed
12 tasks
@danesfeder danesfeder mentioned this pull request Mar 12, 2019
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danesfeder danesfeder danesfeder approved these changes

@devotaaabel devotaaabel Awaiting requested review from devotaaabel

Assignees

@Guardiola31337 Guardiola31337

Labels
backwards incompatible Requires a SEMVER major version change. bug Defect to be fixed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Guardiola31337 @danesfeder