NavigationViewEventDispatcher remove navigation listeners in onDestroy #1013
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devotaaabel
reviewed
Jun 11, 2018
|
@devotaaabel none of the other listeners are added to |
danesfeder
force-pushed
the
dan-progress-leak
branch
from
fa4ad77 to
e7e4b25
Compare
| @@ -42,6 +46,13 @@ void initializeListeners(NavigationViewOptions navigationViewOptions, MapboxNavi | |||
| assignInstructionListListener(navigationViewOptions.instructionListListener()); | |||
| } | |||
|
|
|||
| void onStop(MapboxNavigation navigation) { | |||
There was a problem hiding this comment.
Could you add the @Nullable annotation?
| navigation.addProgressChangeListener(navigationViewOptions.progressChangeListener()); | ||
| ProgressChangeListener progressChangeListener = navigationViewOptions.progressChangeListener(); | ||
| if (progressChangeListener != null) { | ||
| this.progressChangeListener = progressChangeListener; |
There was a problem hiding this comment.
We can move this assignment to L#69 like
this.progressChangeListener = navigationViewOptions.progressChangeListener();and remove this line. It's safe because then we're checking if the listener is null in removeProgressChangeListener 👇
| navigation.addMilestoneEventListener(navigationViewOptions.milestoneEventListener()); | ||
| MilestoneEventListener milestoneEventListener = navigationViewOptions.milestoneEventListener(); | ||
| if (milestoneEventListener != null) { | ||
| this.milestoneEventListener = milestoneEventListener; |
| @@ -290,4 +294,69 @@ public void onInstructionListShown() { | |||
|
|
|||
| verify(instructionListListener, times(1)).onInstructionListVisibilityChanged(true); | |||
| } | |||
|
|
|||
| @Test | |||
| public void onInstructionListHidden_listenerReturnsFalse() { | |||
There was a problem hiding this comment.
We can extract shared code between these new tests.
danesfeder
force-pushed
the
dan-progress-leak
branch
2 times, most recently
from
b5c9df4 to
c059603
Compare
danesfeder
changed the title
danesfeder
force-pushed
the
dan-progress-leak
branch
from
c059603 to
bc204dc
Compare
Guardiola31337
approved these changes
Jun 19, 2018
| @Test | ||
| public void onProgressChangeListenerAddedInOptions_isAddedToNavigation() { | ||
| NavigationViewEventDispatcher eventDispatcher = new NavigationViewEventDispatcher(); | ||
| ProgressChangeListener progressChangeListener = mock(ProgressChangeListener.class); |
There was a problem hiding this comment.
Minor comment - what about 👇?
private ProgressChangeListener setupProgressChangeListener(NavigationViewOptions options) {
ProgressChangeListener progressChangeListener = mock(ProgressChangeListener.class);
when(options.progressChangeListener()).thenReturn(progressChangeListener);
return progressChangeListener;
}
danesfeder
force-pushed
the
dan-progress-leak
branch
from
bc204dc to
b670619
Compare
This was referenced Jun 21, 2018
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #981
Because
MapboxNavigationsurvives rotation, any listeners from the view must be removed before rotation or shutdown. This was not the case for theMilestoneEventListenerandProgressChangeListenerbeing added throughNavigationViewOptions. A memory leak would then result from the old listener instances.