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

Fix backwards instructions in left-side driving scenarios #2044

Merged
merged 3 commits into from
Sep 16, 2019

Conversation

Guardiola31337
Copy link
Contributor

@Guardiola31337 Guardiola31337 commented Sep 5, 2019

Description

Fixes backwards instructions in left-side driving scenarios.

Fixes #396 and a regression from #1946

  • I have added any issue links
  • I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
  • I have added the appropriate milestone and project boards

Goal

Maneuvers images (both the maneuver view and the notification maneuver icon) should be displayed accordingly in left-side driving countries.

Implementation

Fixed ManeuverView regression and added new notification maneuver icons and logic for left driving side 👀 ManeuverUtils#getManeuverResource

Testing

Please describe the manual tests that you ran to verify your changes

  • I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
  • I have tested via a test drive, or a simulation/mock location app
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have updated the CHANGELOG including this PR

cc @1ec5

@Guardiola31337 Guardiola31337 added bug Defect to be fixed. topic: instructions labels Sep 5, 2019
@Guardiola31337 Guardiola31337 self-assigned this Sep 5, 2019
@codecov-io
Copy link

codecov-io commented Sep 5, 2019

Codecov Report

Merging #2044 into master will decrease coverage by 0.51%.
The diff coverage is 90.32%.

@@             Coverage Diff             @@
##             master   #2044      +/-   ##
===========================================
- Coverage     42.01%   41.5%   -0.52%     
- Complexity     1303    1348      +45     
===========================================
  Files           288     286       -2     
  Lines          9215    9127      -88     
  Branches        682     684       +2     
===========================================
- Hits           3872    3788      -84     
+ Misses         5022    5020       -2     
+ Partials        321     319       -2

if (!STEP_MANEUVER_MODIFIER_LEFT.equals(drivingSide)) {
drivingSide = EMPTY;
if (STEP_MANEUVER_MODIFIER_LEFT.equals(drivingSide)) {
if (STEP_MANEUVER_MODIFIER_UTURN.equals(maneuverModifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if we could reduce number (5) of nested if conditions to improve readability of this method.

@@ -141,6 +141,21 @@
R.drawable.ic_maneuver_roundabout_straight);
maneuverMap.put(STEP_MANEUVER_TYPE_ROUNDABOUT, R.drawable.ic_maneuver_roundabout);

maneuverMap.put(STEP_MANEUVER_TYPE_ROUNDABOUT + STEP_MANEUVER_MODIFIER_LEFT + STEP_MANEUVER_MODIFIER_LEFT,
Copy link
Contributor

@andrlee andrlee Sep 5, 2019

Choose a reason for hiding this comment

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

what's the benefit of maintaining hashmap of the constants vs. simple switch-case? also note is that ManeuverMap constructor is not private, which may potentially result in multiple instances of the ManeuverMapobject created accidentally.

Copy link
Contributor Author

@Guardiola31337 Guardiola31337 Sep 9, 2019

Choose a reason for hiding this comment

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

what's the benefit of maintaining hashmap of the constants vs. simple switch-case?

In general, switch statements are considered a bad smell 👀 https://refactoring.guru/smells/switch-statements

Most of the times a switch statement can be easily replaced with map / dictionary.

And yeah, it's true there’s probably a bit more overhead in the dictionary than in the switch statement because you have to store the dictionary somewhere so it’s obviously using some memory. However IMO, in return you’re getting clean, concise, less error-prone and well-organized code that helps maintainability in the long term.

if (drivingSide.equals(STEP_MANEUVER_MODIFIER_RIGHT)) {
boolean flip = SHOULD_FLIP_MODIFIERS.contains(maneuverModifier);
setScaleX(flip ? -1 : 1);
boolean flip = SHOULD_FLIP_MODIFIERS.contains(maneuverModifier);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this equivalent to:

Suggested change
boolean flip = SHOULD_FLIP_MODIFIERS.contains(maneuverModifier);
boolean flip = SHOULD_FLIP_MODIFIERS.contains(maneuverModifier);
if (STEP_MANEUVER_MODIFIER_LEFT.equals(drivingSide) && STEP_MANEUVER_MODIFIER_UTURN.contains(maneuverModifier)) {
setScaleX(flip ? 1 : -1);
} else {
setScaleX(flip ? -1 : 1);
}

@Guardiola31337
Copy link
Contributor Author

I'm still testing multiple scenarios in different countries, I'll report back here and let you know when this is ready for review.

@Guardiola31337
Copy link
Contributor Author

Tested some of the incorrect cases reported in #396 and #1946 and so far it’s working 👌

Borsdane Ave Banner

borsdane_ave_banner

Borsdane Ave Notification

borsdane_ave_notification

East Lancashire Rd Banner

east_lancashire_rd_banner

East Lancashire Rd Notification

east_lancashire_rd_notification

Flamingo Rd Banner

flamingo_rd_banner

Flamingo Rd Notification

flamingo_rd_notification

Old Kent Rd Banner

old_kent_rd_banner

Old Kent Rd Notification

old_kent_rd_notification

@Guardiola31337
Copy link
Contributor Author

One thing that I noticed during testing is that the source of the data that we're using for the banner and the notification is different 👀

BannerInstructions for ManeuverView

private void updateBannerInstruction(RouteProgress routeProgress, Milestone milestone) {
if (milestone instanceof BannerInstructionMilestone) {
BannerInstructions instructions = ((BannerInstructionMilestone) milestone).getBannerInstructions();
instructions = retrieveInstructionsFromBannerEvent(instructions);
if (instructions != null) {
BannerInstructionModel model = new BannerInstructionModel(distanceFormatter, routeProgress, instructions);
bannerInstructionModel.setValue(model);
}
}
}

and LegStep for MapboxNavigationNotification

private void updateManeuverImage(LegStep step) {
if (newManeuverId(step)) {
int maneuverResource = ManeuverUtils.getManeuverResource(step);
currentManeuverId = maneuverResource;
collapsedNotificationRemoteViews.setImageViewResource(R.id.maneuverImage, maneuverResource);
expandedNotificationRemoteViews.setImageViewResource(R.id.maneuverImage, maneuverResource);
}
}

For consistency, we should revisit / consolidate this using the banner information in a feature PR. Refs. #1946 (comment)

@Guardiola31337 Guardiola31337 force-pushed the pg-396-left-driving-side-backwards branch from de02f59 to f572154 Compare September 9, 2019 18:45
@Guardiola31337 Guardiola31337 added the backwards incompatible Requires a SEMVER major version change. label Sep 9, 2019
@Guardiola31337
Copy link
Contributor Author

IMO, objects and methods should live / be where their context is. From my experience utility classes become a drawer easily and we should treat them carefully. So I went ahead and removed ManeuverUtils and moved getManeuverResource utility method into MapboxNavigationNotification. Also I don't believe this class needs to be public as it's used only internally (breaking change / SemVer).

This is now ready for review @andrlee @abhishek1508 @olegzil

@Guardiola31337 Guardiola31337 force-pushed the pg-396-left-driving-side-backwards branch 3 times, most recently from 105bfc1 to fa0bfb1 Compare September 15, 2019 00:03
@Guardiola31337 Guardiola31337 force-pushed the pg-396-left-driving-side-backwards branch from fa0bfb1 to 9d01b99 Compare September 15, 2019 00:07
Copy link
Contributor

@andrlee andrlee left a comment

Choose a reason for hiding this comment

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

LGTM!

@andrlee andrlee merged commit 97e967e into master Sep 16, 2019
@Guardiola31337 Guardiola31337 deleted the pg-396-left-driving-side-backwards branch September 16, 2019 16:11
@Christophe668
Copy link

IMO, objects and methods should live / be where their context is. From my experience utility classes become a drawer easily and we should treat them carefully. So I went ahead and removed ManeuverUtils and moved getManeuverResource utility method into MapboxNavigationNotification. Also I don't believe this class needs to be public as it's used only internally (breaking change / SemVer).

This is now ready for review @andrlee @abhishek1508 @olegzil

@Guardiola31337, To be honest, I was using this class to retrieve to the icon resource in order to implement our upcoming/follow up leg design.
Is this something you want to restrict/avoid?

@Guardiola31337
Copy link
Contributor Author

Hey @Christophe668 thanks for your feedback.

To be honest, I was using this class to retrieve to the icon resource in order to implement our upcoming/follow up leg design.
Is this something you want to restrict/avoid?

As commented in #2044 (comment) we should consolidate the data source for the banner and the notification, in favor of using the banner information. Also mentioning that the logic changed is used for generating the MapboxNavigationNotification icons (which still has some known issues 👀 #1952).

As you may know, all this data can be retrieved from the RouteProgress and used to implement your upcoming / follow up leg design. Would that work for you? Are you using these resources directly in your UI? If so, I wouldn't recommend you doing that because of the reasons above-mentioned and because it could break your UI if we decide to change the resources internally. As mentioned we don't believe this class / method needs to be public as it's used only internally. On a side note, if you still want to use this logic you can grab the code and implement it on your side, that's the beauty of open source 😅

In any case, could you talk more about your specific use case? We'd love to make sure we're not missing anything. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible Requires a SEMVER major version change. bug Defect to be fixed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: Incorrect Direction Images
4 participants