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

Added InstructionViewCallback to allow views to be alerted when the in… #988

Merged
merged 1 commit into from
Jun 8, 2018
Merged

Added InstructionViewCallback to allow views to be alerted when the in… #988

merged 1 commit into from
Jun 8, 2018

Conversation

devotaaabel
Copy link
Contributor

…struction list is shown or hidden. This fixes the bugs where the re-center button and other widgets are shown on top of the instruction list when it's expanded.

@devotaaabel devotaaabel added this to the 0.15.0 milestone Jun 1, 2018
@devotaaabel devotaaabel self-assigned this Jun 1, 2018
danesfeder
danesfeder reviewed Jun 5, 2018
View reviewed changes
...ox/services/android/navigation/testapp/activity/navigationui/EmbeddedNavigationActivity.java
@@ -78,7 +80,8 @@ private void startNavigation(DirectionsRoute directionsRoute) {
.navigationListener(this)
.directionsRoute(directionsRoute)
.shouldSimulateRoute(true)
.progressChangeListener(this);
.progressChangeListener(this)
Copy link
Contributor

Choose a reason for hiding this comment

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

@devotaaabel did you notice EmbeddedNavigationActivity being affected by #981?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danesfeder yea I think it was

danesfeder
danesfeder suggested changes Jun 5, 2018
View reviewed changes
...rc/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewEventDispatcher.java Outdated
@@ -152,4 +159,10 @@ private void assignMilestoneEventListener(NavigationViewOptions navigationViewOp
navigation.addMilestoneEventListener(navigationViewOptions.milestoneEventListener());
}
}

void onInstructionListShown(boolean shown) {
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 add a test in https://github.com/mapbox/mapbox-navigation-android/blob/master/libandroid-navigation-ui/src/test/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewEventDispatcherTest.java for this?

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java Outdated
@@ -401,12 +402,29 @@ public MapboxMap getMapboxMap() {
return map;
}

public void setInstructionViewCallback() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method can be private and we can be more specific in the naming initializeInstructionListListener. I think aligning callback and listener here makes sense even though they are essentially the same.

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java Outdated
@@ -401,12 +402,29 @@ public MapboxMap getMapboxMap() {
return map;
}

public void setInstructionViewCallback() {
if (instructionView != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the instructionView ever be null here? It's being bound right before this in NavigationView#bind()

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java Outdated
instructionView.setInstructionListListener(new InstructionListListener() {
@Override
public void onInstructionListShown(boolean shown) {
if (shown) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This business logic can be extracted into NavigationPresenter so we can call navigationPresenter.onInstructionListVisibilityChanged(isShowing). The show / hide recenter button are already view methods that the presenter has access to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danesfeder hmm maybe the dispatcher should reside in the presenter

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java Outdated
if (instructionView != null) {
instructionView.setInstructionListListener(new InstructionListListener() {
@Override
public void onInstructionListShown(boolean shown) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It may make more sense to update the naming here to something like onInstructionListVisibilityChanged(boolean isShowing) because the listener is fired for both the list showing and hiding and not just showing as the current naming may imply

@devotaaabel devotaaabel force-pushed the devota-add-instruction-view-callback branch 2 times, most recently from ef33a5b to b513aa6 Compare June 5, 2018 18:44
@devotaaabel
Copy link
Contributor Author

@danesfeder updated

@Guardiola31337

This comment has been minimized.

Sign in to view
@devotaaabel devotaaabel force-pushed the devota-add-instruction-view-callback branch 2 times, most recently from 374a6ee to 5c973dc Compare June 6, 2018 14:57
@danesfeder danesfeder added bug Defect to be fixed. navigation-ui labels Jun 6, 2018
@devotaaabel devotaaabel force-pushed the devota-add-instruction-view-callback branch 2 times, most recently from 1e17bd1 to 7042e4d Compare June 6, 2018 17:56
danesfeder
danesfeder reviewed Jun 7, 2018
View reviewed changes
Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

@devotaaabel looking great thank you, much cleaner with that logic in the presenter - two minor nitpicks before we 🚢

...navigation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationView.java Outdated
@@ -378,6 +379,7 @@ private void initializeView() {
bind();
initializeNavigationViewModel();
initializeSummaryBottomSheet();
initializeInstructionListListener();
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it would be incredibly hard to cause this crash, but let's initialize this listener after the presenter/event dispatcher because they are being used within the listener

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java Outdated
@@ -299,6 +305,9 @@ public void hideInstructionList() {
updateLandscapeConstraintsTo(R.layout.instruction_layout);
}
instructionListLayout.setVisibility(GONE);
if (instructionListListener != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove duplicate code here by creating a function that takes a boolean (duplicate code: L320-322)

@devotaaabel devotaaabel force-pushed the devota-add-instruction-view-callback branch from 7042e4d to 874f7fe Compare June 7, 2018 16:21
danesfeder
danesfeder approved these changes Jun 7, 2018
View reviewed changes
Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

Once CI is ✅, this looks good to go 🚢

@devotaaabel devotaaabel closed this Jun 7, 2018
@devotaaabel devotaaabel reopened this Jun 7, 2018
Added InstructionViewCallback to allow views to be alerted when the i…
d9e0809 
…nstruction list is shown or hidden. This fixes the bugs where the re-center button and other widgets are shown on top of the instruction list when it's expanded
@devotaaabel devotaaabel force-pushed the devota-add-instruction-view-callback branch from 874f7fe to d9e0809 Compare June 8, 2018 12:58
@devotaaabel devotaaabel merged commit 69e69e8 into master Jun 8, 2018
@Guardiola31337 Guardiola31337 deleted the devota-add-instruction-view-callback branch June 8, 2018 13:17
@danesfeder danesfeder mentioned this pull request Jun 21, 2018
Closed
11 tasks
@Guardiola31337 Guardiola31337 mentioned this pull request Jul 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danesfeder danesfeder danesfeder approved these changes

@Guardiola31337 Guardiola31337 Awaiting requested review from Guardiola31337

Assignees

@devotaaabel devotaaabel

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

Successfully merging this pull request may close these issues.
3 participants
@devotaaabel @Guardiola31337 @danesfeder