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

Added onOffRoute call and removed queue from NavigationInstructionPlayer #986

Merged
merged 1 commit into from
Jun 5, 2018

Conversation

devotaaabel
Copy link
Contributor

Removed the queue from NavigationInstructionPlayer after realizing that every VoiceInstructionMilestone was the same instance. Added the onOffRoute call to NavigationViewModel.

@devotaaabel devotaaabel added this to the 0.15.0 milestone Jun 1, 2018
@devotaaabel devotaaabel self-assigned this Jun 1, 2018
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.

@devotaaabel so the queue in NavigationInstructionPlayer wasn't necessary? This shouldn't affect any queueing / caching right?

public class NavigationInstructionPlayer implements InstructionListener {

private AudioManager instructionAudioManager;
private AudioFocusRequest instructionFocusRequest;
private MapboxSpeechPlayer mapboxSpeechPlayer;
private AndroidSpeechPlayer androidSpeechPlayer;
private Queue<VoiceInstructionMilestone> instructionQueue;
VoiceInstructionMilestone voiceInstructionMilestone;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to store the String voiceInstructionMilestone.getAnnouncement() here instead of the milestone? Also this field can be private rather than package-private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danesfeder no because we need both the announcement and the ssmlAnnouncement

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, wasn't sure because ssmlAnnouncement is immediately passed to:
mapboxSpeechPlayer.play(voiceInstructionMilestone.getSsmlAnnouncement());

@devotaaabel devotaaabel force-pushed the devota-add-on-off-route-call branch from a0d778c to 5d85b1c Compare June 5, 2018 16:00
@devotaaabel
Copy link
Contributor Author

@danesfeder no it shouldn't affect the caching, that happens in MapboxSpeechPlayer. This queue was for retrying instructions if we were unable to contact Polly

@devotaaabel devotaaabel force-pushed the devota-add-on-off-route-call branch from 5d85b1c to 7c2d676 Compare June 5, 2018 18:14
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.

@devotaaabel thanks for clarifying 🚢

@devotaaabel devotaaabel merged commit c88aff0 into master Jun 5, 2018
@devotaaabel devotaaabel deleted the devota-add-on-off-route-call branch June 5, 2018 18:40
@danesfeder danesfeder mentioned this pull request Jun 21, 2018
11 tasks
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.

2 participants