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.

Sign up for GitHub

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

Jump to bottom

NavigationView do not allow way name to show in overview mode #1676

Merged
merged 1 commit into from
Jan 15, 2019
Merged

NavigationView do not allow way name to show in overview mode #1676

merged 1 commit into from
Jan 15, 2019

Conversation

danesfeder
Copy link
Contributor

Closes #1674

Nice catch @Guardiola31337!

@danesfeder danesfeder added bug Defect to be fixed. ✓ ready for review labels Jan 15, 2019
@danesfeder danesfeder added this to the v0.27.0 milestone Jan 15, 2019
@danesfeder danesfeder self-assigned this Jan 15, 2019
@codecov-io
Copy link

codecov-io commented Jan 15, 2019 •
edited
Loading

Codecov Report

Merging #1676 into master will decrease coverage by <.01%.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##             master    #1676      +/-   ##
============================================
- Coverage     26.22%   26.21%   -0.01%     
- Complexity      786      787       +1     
============================================
  Files           196      196              
  Lines          8340     8342       +2     
  Branches        597      598       +1     
============================================
  Hits           2187     2187              
- Misses         5956     5958       +2     
  Partials        197      197

Guardiola31337
Guardiola31337 reviewed Jan 15, 2019
View reviewed changes
...ation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationPresenter.java
@@ -69,7 +69,7 @@ void onNavigationLocationUpdate(Location location) {
}

void onWayNameChanged(@NonNull String wayName) {
if (TextUtils.isEmpty(wayName)) {
if (TextUtils.isEmpty(wayName) || view.isSummaryBottomSheetHidden()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now, I believe the feature filter task is executed anyways (although we're not showing the way name for the cases in which shouldn't be shown). Is there any possibility to achieve this but also not execute FeatureFilterTask at all so it's not calculated in the cases in which the way name should be not visible?

@danesfeder danesfeder force-pushed the dan-wayname-overview branch from e1125e9 to 5072259 Compare January 15, 2019 17:22
Guardiola31337
Guardiola31337 reviewed Jan 15, 2019
View reviewed changes
...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationContract.java Outdated
@@ -41,6 +41,8 @@

void updateNavigationMap(Location location);

void updateWayNameEnabled(boolean isEnabled);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we disable without adding a new NavigationContract API? Maybe adding a check here

mapbox-navigation-android/libandroid-navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/map/MapWayName.java

Line 39 in 5072259

void updateWayNameWithPoint(PointF point) {
?

@danesfeder danesfeder force-pushed the dan-wayname-overview branch from 5072259 to a37df85 Compare January 15, 2019 18:37
@danesfeder
Copy link
Contributor Author

@Guardiola31337 updated to not create public NavigationContract API

@Guardiola31337
Copy link
Contributor

Guardiola31337 commented Jan 15, 2019 •
edited
Loading

@danesfeder

updated to not create public NavigationContract API

Nice! Way more clean ❤️

I've noticed a small issue though 👀

way_name_not_quickly_updated

After clicking on the RE-CENTER button the way name takes a bit to update. Note that when turning onto Calle Padre Oltra the way name shows Calle General Serrano Orive for a second (the way name previously calculated). Should we keep it invisible until a new way name is calculated?

@danesfeder
Copy link
Contributor Author

@Guardiola31337 yeah good catch there - I believe fixing this would definitely require a public API in NavigationContact. Let's holding on that update until we figure out a better way to contain the NavigationView public APIs 👍

Guardiola31337
Guardiola31337 approved these changes Jan 15, 2019
View reviewed changes
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Let's merge here then 🎉

Thanks @danesfeder

@danesfeder danesfeder force-pushed the dan-wayname-overview branch from a37df85 to 6a3a2e4 Compare January 15, 2019 19:53
@danesfeder danesfeder merged commit db28032 into master Jan 15, 2019
@danesfeder danesfeder deleted the dan-wayname-overview branch January 15, 2019 20:23
@danesfeder danesfeder mentioned this pull request Jan 16, 2019
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Guardiola31337 Guardiola31337 Guardiola31337 approved these changes

Assignees

@danesfeder danesfeder

Labels
bug Defect to be fixed.
Projects
None yet
Milestone
v0.27.0
Development

Successfully merging this pull request may close these issues.
3 participants
@danesfeder @codecov-io @Guardiola31337