Added exit signs to the instruction banner and refactored instruction loader

[devotaaabel]

Left to do:

• Add everything to landscape layouts and deal with rotation
• Deal with issues with sub-text
• Test that abbreviations are still working
• Make InstructionLoader public-friendly by replacing InstructionTextView with TextView
• Add support for left/right exits

Moved style attributes into separate branch. Will make a PR when this one lands. Issue is here: #1705

I wanted to add more tests but it is difficult with the nature of the image spans, and I'm assuming that's why there weren't any tests for the highway signs. If anyone has any advice on how to unit test, it would be helpful. Otherwise I'm going to leave it out.

Closes #972

[akitchen]

Related to #972

[danesfeder]

@devotaaabel are the tests updated here? I see some were removed from InstructionLoaderTest. Please tag when ready and remove the DO NOT MERGE > Ready for Review thanks!

[codecov-io]

Codecov Report

Merging #1195 into master will decrease coverage by 0.46%.
The diff coverage is 28.57%.

@@             Coverage Diff              @@
##             master    #1195      +/-   ##
============================================
- Coverage     29.32%   28.86%   -0.47%     
+ Complexity      820      815       -5     
============================================
  Files           203      212       +9     
  Lines          7924     7993      +69     
  Branches        619      622       +3     
============================================
- Hits           2324     2307      -17     
- Misses         5379     5468      +89     
+ Partials        221      218       -3

[devotaaabel]

@Guardiola31337 @danesfeder This is ready for review. I still want to do some more thorough testing. I cleaned up the unit tests and made them a little more useful, but I need help understanding the code cov report, I'm not sure how to see the changes that I made that it's complaining about. I could add tests to the Verifiers and Nodes, but that doesn't seem very useful, but I can do it if we need it for code coverage.

[danesfeder]

@devotaaabel thanks for addressing the feedback, I have no further comments. We should mark this as SEMVER due to the renaming of ImageCoordinator > ImageCreator. Also can we cut a ticket to track the styling of the exits if that's considered out-of-scope for this PR?

[devotaaabel]

Created an issue here #1705 will open another PR after this PR lands. Thanks!

[Guardiola31337]

This is looking great @devotaaabel

While testing this, I've noticed some weird issues that we need to 👀 before landing this 👇

This seems not abbreviated properly

Left exit icon not drawn

I also left some minor comments / questions.

[Guardiola31337]

As discussed internally, should we remove this constructor? IMO it's not adding any value.

[devotaaabel]

@Guardiola31337 What discussion? Are you talking about the discussion about Dan's PR? If you are, you said that you didn't feel strongly about that either way. I think this is adding the value of not making the InstructionLoader instantiate the hashmap and text utils while still making this testable. It's basically acting as a dependency injection utility method

[Guardiola31337]

Have we thought about extracting these magic numbers i.e. "exit", "exit-number" and "left" into constants?

Going to hold on the approval until we do some more field testing.

[Guardiola31337]

I kept testing this locally and I found the following 👀

Pixel XL Android 9

Exit sign loaded intermittently

Abbreviation issue and hidden secondary banner

Primary text empty

Shield not loaded

Saw the following log trace when ☝️ happened

2019-01-29 15:01:28.654 24575-24575/com.mapbox.services.android.navigation.app E/InstructionTarget: com.squareup.picasso.NetworkRequestHandler$ResponseException: HTTP 403
        at com.squareup.picasso.NetworkRequestHandler.load(NetworkRequestHandler.java:51)
        at com.squareup.picasso.BitmapHunter.hunt(BitmapHunter.java:219)
        at com.squareup.picasso.BitmapHunter.run(BitmapHunter.java:175)
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:458)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:764)
        at com.squareup.picasso.Utils$PicassoThread.run(Utils.java:354)

cc @devotaaabel

[devotaaabel]

Maybe to keep the APIs tightly scoped, we can accept an array of enum that defines the types we want to create for the dev? NodeCreator.TEXT, NodeCreater.SHIELD, etc. - idk just a thought.

@danesfeder So I like this idea, but I'm thinking, let's not do that until there's a need for it, either on our side or a customer request, unless there is one that I'm not aware of. Just to keep it simple for now until there's a need. Let me know what you think.

[Guardiola31337]

After rolling back abbreviations fixes 7be09d2 I wasn't able to reproduce the issues found in #1195 (comment) so it seems that was the reason. Per internal discussion we're going to keep discussing on a solution for landscape issues and will follow up in a different PR.

I was able to reproduce the Left exit icon not drawn #1195 (review) though. Apparently, there's some issue with Android 6.0 and rotating the vector to the left. We've tested and we're going to generate a new left asset to workaround this.

[devotaaabel]

@Guardiola31337 The left exit android 6.0 fix is pushed

[devotaaabel]

@Guardiola31337 @danesfeder This is ready for re-review, I've addressed all relevant issues

[danesfeder]

@devotaaabel great work here and thanks for addressing the feedback 👍