Fix voiceLanguage NPE and add tests for NavigationSpeechPlayer #1054

danesfeder · 2018-06-22T16:05:15Z

Closes #1052 Closes #848 Closes #1060

Refactor speech classes to be more testable and add tests to core classes used. I aligned the naming "instruction" vs. "speech" because we were using both. Also created an immutable AutoValue class SpeechAnnouncement to create from the VoiceInstructionMilestone

TODO tests for:

NavigationSpeechPlayer
NavigationSpeechListener
SpeechPlayerProvider

danesfeder · 2018-06-25T18:21:27Z

@Guardiola31337 @devotaaabel this is ready for 👀

...ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/AndroidSpeechPlayer.java


  /**
   * Creates an instance of {@link AndroidSpeechPlayer}.
   *
-   * @param context used to create an instance of {@link TextToSpeech}
+   * @param context  used to create an instance of {@link TextToSpeech}


devotaaabel · 2018-06-25T18:35:30Z

...ation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewModel.java

@@ -267,7 +276,11 @@ public void userOffRoute(Location location) {
    @Override
    public void onMilestoneEvent(RouteProgress routeProgress, String instruction, Milestone milestone) {
      if (milestone instanceof VoiceInstructionMilestone) {
-        instructionPlayer.play((VoiceInstructionMilestone) milestone);
+        SpeechAnnouncement speechAnnouncement = SpeechAnnouncement.builder()


what if you pass in VoiceInstructionMilestone into a constructor in SpeechAnnouncement so this logic can be in SpeechAnnouncement? Instead of using the builder pattern

Was thinking here in terms of usability outside of the turn-by-turn UI. NavigationSpeechPlayer is a public API, so tying it completely to the VoiceInstructionMilestone in order to voice something may not make a lot of sense to someone that doesn't want to use the milestones to voice something.

I did update the builder though to also take a VoiceInstructionMilestone, does that work? Any reasons you don't like the builder pattern here?

That makes sense! And yea having a builder method that takes in a VoiceInstructionMilestone for convenience is 👍

devotaaabel · 2018-06-25T18:38:12Z

...ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/AndroidSpeechPlayer.java

   */
  @Override
-  public void play(String instruction) {
+  public void play(SpeechAnnouncement speechAnnouncement) {
+    if (speechAnnouncement == null) {


I would add this in the boolean canPlay. i.e., boolean canPlay = languageSupported && !isMuted && !TextUtils.isEmpty(instruction) && speechAnnouncement != null;

devotaaabel · 2018-06-25T18:40:24Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/MapboxSpeechPlayer.java


  /**
   * Construct an instance of {@link MapboxSpeechPlayer}
   *
-   * @param context to setup the caches
-   * @param language for which language
+   * @param context     to setup the caches


we should be consistent with javadoc param spacing

All of the javadoc spacing was moved to align to the length of the longest @param, in this case accessToken:

* @param context to setup the caches * @param language for which language * @param accessToken a valid Mapbox access token

This is how I had always done it, but if we'd like to agree on a set style here I'm happy to conform / adjust cc @Guardiola31337

That’s correct. But it’s true that we don’t have a checkstyle rule that enforces it. So if you reformat (as I do constantly 😅 ⌥⌘L) it gets formatted as @danesfeder mentioned.

I'm ok with sticking with that. We should add this to checkstyle, or at least going forward start to update non-conforming javadoc as we run into it.

We should add this to checkstyle, or at least going forward start to update non-conforming javadoc as we run into it.

Yeah as I mentioned we don't have one and it'd be awesome to add it. I briefly looked into it and it's a tricky one though (more difficult than I initially thought). I'll keep digging.

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/MapboxSpeechPlayer.java

  }

  /**
   * Plays the specified text instruction using MapboxSpeech API
   *
   * @param instruction voice instruction to be synthesized and played
-   * @param textType either "ssml" or "text"
+   * @param textType    either "ssml" or "text"


devotaaabel · 2018-06-25T18:51:22Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/MapboxSpeechPlayer.java

@@ -243,12 +253,12 @@ private void startNextInstruction() {

  private void clearInstructionUrls() {
    while (!instructionQueue.isEmpty()) {
-      instructionQueue.remove().delete();
+      instructionQueue.poll();


Why this change? you still need to delete the actual files, and you shouldn't have to worry about exceptions because of the while(!instructionQueue.isEmpty())

The poll() Javadoc states Retrieves and removes the head of this queue, or returns {@code null} if this queue is empty. Does that not actually delete the file? I changed it back in case I was missing context here.

nope, it removes the File object from the queue and we lose track of it in memory but remember the audio file is still in the file system.

devotaaabel · 2018-06-25T18:55:17Z

...src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/NavigationSpeechPlayer.java

+   */
+  public void setMuted(boolean isMuted) {
+    this.isMuted = isMuted;
+    retrieveSpeechPlayer().setMuted(isMuted);


so this is only going to set the currently retrieved speech player muted, not both if both are active

devotaaabel · 2018-06-25T18:55:31Z

...src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/NavigationSpeechPlayer.java

+   * @since 0.16.0
+   */
+  public void onOffRoute() {
+    retrieveSpeechPlayer().onOffRoute();


devotaaabel · 2018-06-25T18:55:38Z

...src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/NavigationSpeechPlayer.java

+   * @since 0.16.0
+   */
+  public void onDestroy() {
+    retrieveSpeechPlayer().onDestroy();


devotaaabel · 2018-06-25T18:59:23Z

...c/main/java/com/mapbox/services/android/navigation/ui/v5/voice/NavigationSpeechListener.java

+  @Override
+  public void onError(String errorText, SpeechAnnouncement speechAnnouncement) {
+    Timber.e(errorText);
+    speechPlayerProvider.retrieveAndroidSpeechPlayer().play(speechAnnouncement);


Isn't this going to go into an infinite loop if AndroidSpeechPlayer returns an error?

Currently not firing error listener for AndroidSpeechPlayer https://github.com/mapbox/mapbox-navigation-android/blob/master/libandroid-navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/UtteranceListener.java#L31

Hmm, should we maybe print an error there? For the case where there's an error initializing native TTS

It looks like we are logging this currently in master https://github.com/mapbox/mapbox-navigation-android/blob/master/libandroid-navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/AndroidSpeechPlayer.java#L44

Does sticking with that work okay?

danesfeder · 2018-06-26T13:59:11Z

@devotaaabel updated and addressed some of your comments - let me know if anything is unclear

Guardiola31337

Added some minor things/questions - overall this looks great.

Guardiola31337 · 2018-06-27T13:17:13Z

...ation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewModel.java

  private void initVoiceInstructions(NavigationViewOptions options) {
-    boolean languageSupported = options.directionsRoute().voiceLanguage() != null;
-    instructionPlayer = new NavigationInstructionPlayer(getApplication(), language, languageSupported, accessToken);
+    boolean voiceLanguageSupported = options.directionsRoute().voiceLanguage() != null;


Minor thing - Per consistency with other booleans in the code base, what about calling it isVoiceLanguageSupported instead?

Guardiola31337 · 2018-06-27T13:24:39Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/MapboxSpeechPlayer.java


  /**
   * Construct an instance of {@link MapboxSpeechPlayer}
   *
-   * @param context to setup the caches
-   * @param language for which language
+   * @param context     to setup the caches


That’s correct. But it’s true that we don’t have a checkstyle rule that enforces it. So if you reformat (as I do constantly 😅 ⌥⌘L) it gets formatted as @danesfeder mentioned.

Guardiola31337 · 2018-06-27T13:42:46Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/SpeechDownloadTask.java

  private static final String MP3_POSTFIX = ".mp3";
  private static final int END_OF_FILE_DENOTER = -1;
  private static int instructionNamingInt = 1;
+


NIT - unnecessary empty line

Guardiola31337 · 2018-06-27T13:50:33Z

...rc/main/java/com/mapbox/services/android/navigation/ui/v5/voice/SpeechAudioFocusManager.java

+  }
+
+  void requestAudioFocus() {
+    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {


Could we remove/abstract these kind of checks somehow? I'm thinking on implementing like a mix of Factory and Strategy patterns.

Guardiola31337 · 2018-06-27T13:52:45Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/MapboxSpeechPlayer.java

+  public void play(SpeechAnnouncement speechAnnouncement) {
+    this.announcement = speechAnnouncement;
+    String ssmlAnnouncement = announcement.ssmlAnnouncememnt();
+    if (ssmlAnnouncement != null) {


Could we refactor this if else somehow? Both branches are calling play with different parameters.

Guardiola31337 · 2018-06-27T13:55:05Z

...tion-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/SpeechListener.java


  void onStart();

  void onDone();

-  void onError(boolean isMapboxPlayer, String errorText);
+  void onError(String errorText, SpeechAnnouncement speechAnnouncement);


Question - Could we refactor the code so we don't need to pass the announcement in the error callback?

Let's stick with the current setup here - this is an internal listener. No excuse, though, I know. We can look into re-wiring in the future.

devotaaabel · 2018-06-29T16:51:11Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/SpeechAnnouncement.java

+   * @since 0.16.0
+   */
+  @Nullable
+  public abstract String ssmlAnnouncememnt();


Guardiola31337

After fixing @devotaaabel's commented typo, let's

Great job @danesfeder 🙇

danesfeder added bug Defect to be fixed. navigation-ui labels Jun 22, 2018

danesfeder self-assigned this Jun 22, 2018

danesfeder requested review from Guardiola31337 and devotaaabel June 22, 2018 16:05

danesfeder force-pushed the dan-null-instruction-player branch 2 times, most recently from 1441d10 to 6045f5e Compare June 22, 2018 16:08

This was referenced Jun 25, 2018

Null Reference in NavigationInstructionPlayer #1052

Closed

No voice in 0.13 #922

Closed

danesfeder force-pushed the dan-null-instruction-player branch from 6045f5e to 9fb9d47 Compare June 25, 2018 18:00

danesfeder added the ✓ ready for review label Jun 25, 2018

danesfeder added this to the 0.16.0 milestone Jun 25, 2018

danesfeder force-pushed the dan-null-instruction-player branch from 9fb9d47 to 536abf8 Compare June 25, 2018 18:21

devotaaabel suggested changes Jun 25, 2018

View reviewed changes

danesfeder mentioned this pull request Jun 25, 2018

Unable to destroy activity #1060

Closed

danesfeder force-pushed the dan-null-instruction-player branch 2 times, most recently from 00ac03b to 4736e74 Compare June 26, 2018 13:58

Guardiola31337 reviewed Jun 27, 2018

View reviewed changes

danesfeder force-pushed the dan-null-instruction-player branch 2 times, most recently from 2ff2383 to be96983 Compare June 28, 2018 19:29

devotaaabel approved these changes Jun 29, 2018

View reviewed changes

Guardiola31337 approved these changes Jun 29, 2018

View reviewed changes

danesfeder force-pushed the dan-null-instruction-player branch from be96983 to 99d5a66 Compare June 29, 2018 18:15

Fix voiceLanguage NPE and add tests for NavigationSpeechPlayer

e5b5583

danesfeder force-pushed the dan-null-instruction-player branch from 99d5a66 to e5b5583 Compare June 29, 2018 18:17

danesfeder merged commit 6e8fc57 into master Jun 29, 2018

danesfeder deleted the dan-null-instruction-player branch June 29, 2018 18:49

danesfeder mentioned this pull request Jul 20, 2018

Release v0.16.0 #1132

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix voiceLanguage NPE and add tests for NavigationSpeechPlayer #1054

Fix voiceLanguage NPE and add tests for NavigationSpeechPlayer #1054

danesfeder commented Jun 22, 2018 •

edited

Loading

danesfeder commented Jun 25, 2018

This comment was marked as resolved.

devotaaabel Jun 25, 2018

danesfeder Jun 26, 2018

devotaaabel Jun 28, 2018

devotaaabel Jun 25, 2018

devotaaabel Jun 25, 2018

danesfeder Jun 26, 2018

Guardiola31337 Jun 27, 2018

devotaaabel Jun 28, 2018

Guardiola31337 Jun 29, 2018

This comment was marked as resolved.

devotaaabel Jun 25, 2018

danesfeder Jun 26, 2018

devotaaabel Jun 28, 2018

devotaaabel Jun 25, 2018

devotaaabel Jun 25, 2018

devotaaabel Jun 25, 2018

devotaaabel Jun 25, 2018

danesfeder Jun 26, 2018

devotaaabel Jun 28, 2018

danesfeder Jun 28, 2018

danesfeder commented Jun 26, 2018

Guardiola31337 left a comment

Guardiola31337 Jun 27, 2018

Guardiola31337 Jun 27, 2018

Guardiola31337 Jun 27, 2018

Guardiola31337 Jun 27, 2018

Guardiola31337 Jun 27, 2018

Guardiola31337 Jun 27, 2018

danesfeder Jun 28, 2018

devotaaabel Jun 29, 2018

Guardiola31337 left a comment

Fix voiceLanguage NPE and add tests for NavigationSpeechPlayer #1054

Fix voiceLanguage NPE and add tests for NavigationSpeechPlayer #1054

Conversation

danesfeder commented Jun 22, 2018 • edited Loading

danesfeder commented Jun 25, 2018

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danesfeder commented Jun 26, 2018

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

danesfeder commented Jun 22, 2018 •

edited

Loading