Dynamic abbreviations in banner instructions #887

devotaaabel · 2018-04-25T21:42:30Z

#807 I moved the image loading logic from InstructionLoader into the new class InstructionImageLoader to separate concerns, and added AbbreviationCoordinator. Both new classes are accessed by InstructionLoader, which now coordinates the use of both new classes.

…ng shield and abbreviation logic

…abbreviations by abbreviationPriority while also keeping track of index

danesfeder

Added a couple nit picks - overall this looks great. Nice work @devotaaabel

...n/java/com/mapbox/services/android/navigation/ui/v5/instruction/AbbreviationCoordinator.java

+    this.abbreviations = new HashMap<>();
+    this.textViewUtils = textViewUtils;
+  }
+


danesfeder · 2018-05-15T20:30:39Z

...n/java/com/mapbox/services/android/navigation/ui/v5/instruction/AbbreviationCoordinator.java

+
+    int currAbbreviationPriority = 0;
+    int maxAbbreviationPriority = Collections.max(abbreviations.keySet());
+    while (!textViewUtils.textFits(textView, bannerText) && (currAbbreviationPriority <= maxAbbreviationPriority)) {


Can we pull this while out into a private method to make abbreviateBannerText more readable?

We can also extract !textViewUtils.textFits(textView, bannerText) && (currAbbreviationPriority <= maxAbbreviationPriority) into two booleans that better explain what is being checked by the loop.

Guardiola31337

Overall this is looking great. Well done @devotaaabel

Some comments to discuss before merging the PR.

Guardiola31337 · 2018-05-17T08:38:35Z

...n/java/com/mapbox/services/android/navigation/ui/v5/instruction/AbbreviationCoordinator.java

+ * BannerComponents containing abbreviation information and given a list of BannerComponentNodes,
+ * constructed by InstructionLoader.
+ */
+public class AbbreviationCoordinator {


Do we want to expose this class and its methods publicly?

Classes are private until the opposite is proved.

😂
It's only consumed by InstructionLoader (+ tests) which is in the same package so I guess we could make it package-private. Also the IDE hints you about doing that (constructors).

Guardiola31337 · 2018-05-17T08:40:34Z

...n/java/com/mapbox/services/android/navigation/ui/v5/instruction/AbbreviationCoordinator.java

+  private Map<Integer, List<Integer>> abbreviations;
+  private TextViewUtils textViewUtils;
+
+  public AbbreviationCoordinator(TextViewUtils textViewUtils) {


Access can be package-private

Guardiola31337 · 2018-05-17T08:54:49Z

...n/java/com/mapbox/services/android/navigation/ui/v5/instruction/AbbreviationCoordinator.java

+
+  /**
+   * Using the abbreviations HashMap which should already be populated, abbreviates the text in the
+   * bannerConmponentNodes until the text fits the given TextView.


NIT Typo *bannerConmponentNodes

Guardiola31337 · 2018-05-17T08:59:28Z

...n/java/com/mapbox/services/android/navigation/ui/v5/instruction/AbbreviationCoordinator.java

+   */
+  public void addPriorityInfo(BannerComponents bannerComponents, int index) {
+    int abbreviationPriority = bannerComponents.abbreviationPriority();
+    if (abbreviations.get(Integer.valueOf(abbreviationPriority)) == null) {


Unnecessary boxing 'Integer.valueOf(abbreviationPriority)'

Guardiola31337 · 2018-05-17T09:01:20Z

...n/java/com/mapbox/services/android/navigation/ui/v5/instruction/AbbreviationCoordinator.java

+    int maxAbbreviationPriority = Collections.max(abbreviations.keySet());
+
+    while (shouldKeepAbbreviating(textView, bannerText, currAbbreviationPriority, maxAbbreviationPriority)) {
+      List<Integer> indices = abbreviations.get(new Integer(currAbbreviationPriority++));


Unnecessary boxing 'new Integer(currAbbreviationPriority++)'

...rc/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionTarget.java

+  private BannerShield shield;
  private InstructionLoadedCallback instructionLoadedCallback;

  InstructionTarget(TextView textView, Spannable instructionSpannable,


Guardiola31337 · 2018-05-17T09:55:29Z

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

+import android.graphics.Paint;
+import android.widget.TextView;
+
+public class TextViewUtils {


Do we want to expose this class and its method publicly?

Classes are private until the opposite is proved.

😂
It's only consumed by AbbreviationCoordinator (+ tests) so I guess this should live under com.mapbox.services.android.navigation.ui.v5.instruction package.

...est/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionLoaderTest.java

+public class InstructionLoaderTest {
+
+  @Test
+  public void testPriorityInfoIsAdded() {


Guardiola31337 · 2018-05-17T10:08:51Z

...va/com/mapbox/services/android/navigation/ui/v5/instruction/AbbreviationCoordinatorTest.java

+
+public class AbbreviationCoordinatorTest extends BaseTest {
+  @Test
+  public void textIsAbbreviatedWhenItDoesntFit() {


For consistency we should name the tests following the same style (e.g. onResetCamera_trackingIsResumed).
In any case, test prefix is not necessary because it's implicit with the usage of @Test annotation.

Normally is good to structure the tests following the triple A 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=40 http://wiki.c2.com/?ArrangeActAssert

Here we're including the Act inside the Assert. It'd be great to have them separated and take advantage of giving great names to variables so it's easy to understand what's going on.

AbbreviationCoordinator abbreviationCoordinator = new AbbreviationCoordinator(textViewUtils); abbreviationCoordinator.addPriorityInfo(bannerComponents, 0); List<InstructionLoader.BannerComponentNode> bannerComponentNodes = new ArrayList<>(); bannerComponentNodes.add(new AbbreviationCoordinator.AbbreviationNode(bannerComponents, 0));

Should be in the Arrange part.

This applies to other tests included in AbbreviationCoordinatorTest.

Guardiola31337 · 2018-05-17T10:11:49Z

...est/java/com/mapbox/services/android/navigation/ui/v5/instruction/BannerComponentsFaker.java

+
+import com.mapbox.api.directions.v5.models.BannerComponents;
+
+public class BannerComponentsFaker {


Why BannerComponentsFaker needs to be public?

Classes are private until the opposite is proved.

😂
I guess we could make it package-private, it's only consumed by the tests.

danesfeder

🚢 Awesome work @devotaaabel!

Devota Aabel added 20 commits March 26, 2018 17:19

First commit for abbreviations using MAS snapshot

9a60689

Updated by removing custom comparator

a64b521

Merge remote-tracking branch 'origin/master' into devota-abbreviations

7ff4fc9

Merge remote-tracking branch 'origin/master' into devota-abbreviations

4a39697

Merge remote-tracking branch 'origin/master' into devota-abbreviations

297ded2

Added InstructionBuilder without implementation

a6a86f4

Added text as filler for banner shields and continued work on combini…

da17d64

…ng shield and abbreviation logic

Continued abbreviation work

59f6556

Continued working on abbreviations but having issues sorting list of …

ffc8d81

…abbreviations by abbreviationPriority while also keeping track of index

Merge branch 'master' into devota-abbreviations

5f6db50

Merge branch 'master' into devota-abbreviations

9bd0911

Merge branch 'master' into devota-abbreviations

3c25ab7

First commit with InstructionBuilder fixed up

c1890f5

Cleanup

91bafc7

Refactored to separate concerns

9cf38b3

Merge branch 'master' into devota-abbreviations

5fff99c

Added startIndex to Node constructor for consistency

21a57b5

Cleanup

2c23887

Removed snapshots

183c2d9

Added private modifiers

01707cd

devotaaabel added the ⚠️ DO NOT MERGE PR should not be merged! label Apr 25, 2018

devotaaabel requested review from Guardiola31337 and danesfeder April 25, 2018 21:42

Devota Aabel added 2 commits April 25, 2018 17:43

Removed logging

ef73dd9

Fixed checkstyle issue

8de18f2

devotaaabel changed the title ~~Devota abbreviations~~ Dynamic abbreviations in banner instructions Apr 26, 2018

Guardiola31337 force-pushed the master branch from 96cd589 to 279c7b2 Compare April 27, 2018 18:34

Devota Aabel added 3 commits April 30, 2018 13:13

Merge branch 'master' into devota-abbreviations

eb97a3a

Refactored to separate concerns even further

95a724b

Fixed bug where navigation froze when text never fit TextView

73cc170

Cleanup

a14bf82

devotaaabel closed this May 9, 2018

devotaaabel reopened this May 9, 2018

devotaaabel self-assigned this May 10, 2018

Merge branch 'master' into devota-abbreviations

9ad6125

danesfeder reviewed May 15, 2018

View reviewed changes

Devota Aabel and others added 5 commits May 16, 2018 09:32

Merge branch 'master' into devota-abbreviations

ed3e959

Extracted booleans into variables

50e8adf

Refactored for clarity

bf34537

Fixed checkstyle issues

8858ac2

Merge branch 'master' into devota-abbreviations

387ee2d

Guardiola31337 reviewed May 17, 2018

View reviewed changes

Devota Aabel and others added 4 commits May 17, 2018 10:33

Merge branch 'master' into devota-abbreviations

49d7da3

Made changes according to PR comments

1edda00

Merge branch 'master' into devota-abbreviations

a45401b

Updated test names

b7d53a5

devotaaabel force-pushed the devota-abbreviations branch from 7e0e2c8 to f2b0dd3 Compare May 18, 2018 14:44

Cleanup from PR comments

4a8427b

devotaaabel force-pushed the devota-abbreviations branch from f2b0dd3 to 4a8427b Compare May 18, 2018 14:45

Devota Aabel and others added 4 commits May 18, 2018 10:50

Refactored name

13dd699

Renamed ImageLoader to ImageCoordinator

2698e05

Fixed bug

7de31c7

Merge branch 'master' into devota-abbreviations

c288365

danesfeder approved these changes May 18, 2018

View reviewed changes

devotaaabel merged commit ab2b7e3 into master May 18, 2018

devotaaabel deleted the devota-abbreviations branch May 18, 2018 19:43

This was referenced May 21, 2018

Add abbreviations from the server (street names, cardinal directions, and subbanners) #807

Closed

Use BannerComponent text as placeholder for shield before loading #715

Closed

danesfeder mentioned this pull request May 31, 2018

Release v0.14.0 #982

Closed

13 tasks

danesfeder mentioned this pull request Oct 18, 2018

Update UrlDensityMap for more screen densities #1436

Merged


		import com.mapbox.api.directions.v5.models.BannerComponents;

		public class BannerComponentsFaker {

Dynamic abbreviations in banner instructions #887

Dynamic abbreviations in banner instructions #887

Uh oh!

Conversation

devotaaabel commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danesfeder left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danesfeder left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

devotaaabel commented Apr 25, 2018 •

edited

Loading