Kyv fix leak navigation map route option keep lifecycle observer #3012
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
|
Here's no documentation for new methods just because it's another option how to fix leak. If we'd approve this approach I'll add docs 🙈 |
| @NonNull MapboxMap mapboxMap) { | ||
| this(mapView, mapboxMap, null); | ||
| @NonNull MapboxMap mapboxMap, | ||
| @NonNull LifecycleOwner lifecycleOwner) { |
There was a problem hiding this comment.
Noting that this would break SemVer cc @langsmith
| @@ -115,8 +125,9 @@ public NavigationMapboxMap(@NonNull MapView mapView, | |||
| */ | |||
| public NavigationMapboxMap(@NonNull MapView mapView, | |||
| @NonNull MapboxMap mapboxMap, | |||
| @NonNull LifecycleOwner lifecycleOwner, | |||
There was a problem hiding this comment.
Noting that this would break SemVer cc @langsmith
| @@ -129,8 +140,9 @@ public NavigationMapboxMap(@NonNull MapView mapView, | |||
| */ | |||
| public NavigationMapboxMap(@NonNull MapView mapView, | |||
| @NonNull MapboxMap mapboxMap, | |||
| @NonNull LifecycleOwner lifecycleOwner, | |||
There was a problem hiding this comment.
Noting that this would break SemVer cc @langsmith
| @@ -144,11 +156,13 @@ public NavigationMapboxMap(@NonNull MapView mapView, | |||
| */ | |||
| public NavigationMapboxMap(@NonNull MapView mapView, | |||
| @NonNull MapboxMap mapboxMap, | |||
| @NonNull LifecycleOwner lifecycleOwner, | |||
There was a problem hiding this comment.
Noting that this would break SemVer cc @langsmith
Comment on lines
-60
to
-160
| * Construct an instance of {@link NavigationMapRoute}. | ||
| * | ||
| * @param mapView the MapView to apply the route to | ||
| * @param mapboxMap the MapboxMap to apply route with | ||
| * @since 0.4.0 | ||
| */ | ||
| public NavigationMapRoute(@NonNull MapView mapView, @NonNull MapboxMap mapboxMap) { | ||
| this(null, mapView, mapboxMap, R.style.NavigationMapRoute); | ||
| } | ||
|
|
||
| /** | ||
| * Construct an instance of {@link NavigationMapRoute}. | ||
| * | ||
| * @param mapView the MapView to apply the route to | ||
| * @param mapboxMap the MapboxMap to apply route with | ||
| * @param belowLayer optionally pass in a layer id to place the route line below | ||
| * @since 0.4.0 | ||
| */ | ||
| public NavigationMapRoute(@NonNull MapView mapView, @NonNull MapboxMap mapboxMap, | ||
| @Nullable String belowLayer) { | ||
| this(null, mapView, mapboxMap, R.style.NavigationMapRoute, belowLayer); | ||
| } | ||
|
|
||
| /** | ||
| * Construct an instance of {@link NavigationMapRoute}. | ||
| * | ||
| * @param navigation an instance of the {@link MapboxNavigation} object. Passing in null means | ||
| * your route won't consider rerouting during a navigation session. | ||
| * @param mapView the MapView to apply the route to | ||
| * @param mapboxMap the MapboxMap to apply route with | ||
| * @since 0.4.0 | ||
| */ | ||
| public NavigationMapRoute(@Nullable MapboxNavigation navigation, @NonNull MapView mapView, | ||
| @NonNull MapboxMap mapboxMap) { | ||
| this(navigation, mapView, mapboxMap, R.style.NavigationMapRoute); | ||
| } | ||
|
|
||
| /** | ||
| * Construct an instance of {@link NavigationMapRoute}. | ||
| * | ||
| * @param navigation an instance of the {@link MapboxNavigation} object. Passing in null means | ||
| * your route won't consider rerouting during a navigation session. | ||
| * @param mapView the MapView to apply the route to | ||
| * @param mapboxMap the MapboxMap to apply route with | ||
| * @param belowLayer optionally pass in a layer id to place the route line below | ||
| * @since 0.4.0 | ||
| */ | ||
| public NavigationMapRoute(@Nullable MapboxNavigation navigation, @NonNull MapView mapView, | ||
| @NonNull MapboxMap mapboxMap, @Nullable String belowLayer) { | ||
| this(navigation, mapView, mapboxMap, R.style.NavigationMapRoute, belowLayer); | ||
| } | ||
|
|
||
| /** | ||
| * Construct an instance of {@link NavigationMapRoute}. | ||
| * | ||
| * @param navigation an instance of the {@link MapboxNavigation} object. Passing in null means | ||
| * your route won't consider rerouting during a navigation session. | ||
| * @param mapView the MapView to apply the route to | ||
| * @param mapboxMap the MapboxMap to apply route with | ||
| * @param styleRes a style resource with custom route colors, scale, etc. | ||
| */ | ||
| public NavigationMapRoute(@Nullable MapboxNavigation navigation, @NonNull MapView mapView, | ||
| @NonNull MapboxMap mapboxMap, @StyleRes int styleRes) { | ||
| this(navigation, mapView, mapboxMap, styleRes, null); | ||
| } | ||
|
|
||
| /** | ||
| * Construct an instance of {@link NavigationMapRoute}. | ||
| * | ||
| * @param navigation an instance of the {@link MapboxNavigation} object. Passing in null means | ||
| * your route won't consider rerouting during a navigation session. | ||
| * @param mapView the MapView to apply the route to | ||
| * @param mapboxMap the MapboxMap to apply route with | ||
| * @param styleRes a style resource with custom route colors, scale, etc. | ||
| * @param belowLayer optionally pass in a layer id to place the route line below | ||
| */ | ||
| public NavigationMapRoute(@Nullable MapboxNavigation navigation, @NonNull MapView mapView, | ||
| @NonNull MapboxMap mapboxMap, @StyleRes int styleRes, | ||
| @Nullable String belowLayer) { | ||
| this(navigation, mapView, mapboxMap, styleRes, belowLayer, false, null); | ||
| } | ||
|
|
||
| /** | ||
| * Construct an instance of {@link NavigationMapRoute}. | ||
| * | ||
| * @param navigation an instance of the {@link MapboxNavigation} object. Passing in null means | ||
| * your route won't consider rerouting during a navigation session. | ||
| * @param mapView the MapView to apply the route to | ||
| * @param mapboxMap the MapboxMap to apply route with | ||
| * @param styleRes a style resource with custom route colors, scale, etc. | ||
| * @param belowLayer optionally pass in a layer id to place the route line below | ||
| * @param routeLineInitializedCallback called to indicate that the route line layer has been added | ||
| * to the current style | ||
| */ | ||
| public NavigationMapRoute(@Nullable MapboxNavigation navigation, @NonNull MapView mapView, | ||
| @NonNull MapboxMap mapboxMap, @StyleRes int styleRes, | ||
| @Nullable String belowLayer, | ||
| @Nullable MapRouteLineInitializedCallback routeLineInitializedCallback) { | ||
| this(navigation, mapView, mapboxMap, styleRes, belowLayer, false, routeLineInitializedCallback); | ||
| } |
There was a problem hiding this comment.
Noting that this would break SemVer cc @langsmith
Comment on lines
77
to
86
| private NavigationMapRoute(@Nullable MapboxNavigation navigation, | ||
| @NonNull MapView mapView, | ||
| @NonNull MapboxMap mapboxMap, | ||
| @NonNull LifecycleOwner lifecycleOwner, | ||
| @StyleRes int styleRes, | ||
| @Nullable String belowLayer, | ||
| Boolean vanishRouteLineEnabled, | ||
| @Nullable MapRouteLineInitializedCallback routeLineInitializedCallback) { |
There was a problem hiding this comment.
Noting that this would break SemVer cc @langsmith
Guardiola31337
added
backwards incompatible
libnavigation-ui/src/main/java/com/mapbox/navigation/ui/route/NavigationMapRoute.java
Outdated
Show resolved
Hide resolved
|
Converted to draft, because it's one of option, but still ready for review |
cafesilencio
approved these changes
May 21, 2020
cafesilencio
approved these changes
May 21, 2020
10 tasks
Codecov Report
@@ Coverage Diff @@
## master #3012 +/- ##
============================================
- Coverage 36.78% 36.76% -0.02%
Complexity 2215 2215
============================================
Files 553 553
Lines 19975 19982 +7
Branches 1885 1882 -3
============================================
- Hits 7348 7347 -1
- Misses 11789 11796 +7
- Partials 838 839 +1 |
@cafesilencio thanks, could you take a 👀 again. PS commits will be squashed before merge |
| @@ -223,9 +220,6 @@ public void onPause() { | |||
|
|
|||
| public void onStop() { | |||
| mapView.onStop(); | |||
| if (navigationMap != null) { | |||
| navigationMap.onStop(); | |||
| } | |||
There was a problem hiding this comment.
Does onStop need lifecycleRegistry.markState(Lifecycle.State.STOPPED); as I see in onStart?
There was a problem hiding this comment.
This may be outside of the scope of your PR so I don't think merging depends on this necessarily but it caught my attention so if we could at least create a ticket to investigate whether lifecycleRegistry.markState(Lifecycle.State.STOPPED) is needed that would be great so we don't lose track of it.
Besides that, I reviewed the changes again. I think this is the right way to go and I think the changes should be merged in.
There was a problem hiding this comment.
@cafesilencio definitely it's not a right way how NavigationView should work 👍
I've created separate issue #3024 for missing calls. Would be great investigate and fix it in separate PR because here might be more than one missing call(have a look on fun onPause(), missing call as well)
If everything is ok I need ✅
cafesilencio
approved these changes
May 22, 2020
|
I've approved but it looks like the PR needs an additional approval.
…On Fri, May 22, 2020 at 2:03 PM Yury Kanetski ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
libnavigation-ui/src/main/java/com/mapbox/navigation/ui/NavigationView.java
<#3012 (comment)>
:
> @@ -223,9 +220,6 @@ public void onPause() {
public void onStop() {
mapView.onStop();
- if (navigationMap != null) {
- navigationMap.onStop();
- }
@cafesilencio <https://github.com/cafesilencio> definitely it's not a
right way how NavigationView should work 👍
I've created separate issue #3024
<#3024> for
missing calls. Would be great investigate and fix it in separate PR because
here might be more than one missing call(have a look on fun onPause(),
missing call as well)
If everything is ok I need ✅
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3012 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARFIWR5G3GDLWBMABGP26LRS3SB5ANCNFSM4NGW25MA>
.
|
RingerJK
force-pushed
the
kyv-fix-leak-navigation-map-route-option-keep-lifecycle-observer
branch
from
f2edd3f
to
33aa427
Compare
|
@cafesilencio thanks! |
RingerJK
force-pushed
the
kyv-fix-leak-navigation-map-route-option-keep-lifecycle-observer
branch
2 times, most recently
from
81da820
to
645066d
Compare
|
Crashes because of #3024 |
RingerJK
force-pushed
the
kyv-fix-leak-navigation-map-route-option-keep-lifecycle-observer
branch
from
645066d
to
5a0acf0
Compare
…tionMapboxMap and NavigationMapRoute pass LifecycleOwner explicitly.
RingerJK
force-pushed
the
kyv-fix-leak-navigation-map-route-option-keep-lifecycle-observer
branch
from
5a0acf0
to
e537d5b
Compare
RingerJK
deleted the
kyv-fix-leak-navigation-map-route-option-keep-lifecycle-observer
branch
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.
Description
Another option how to solve memory leak
First is #3007
bug,feature,new API(s),SEMVER, etc.)Goal
Please describe the PR goals. Just the stuff needed to implement the fix / feature and a simple rationale. It could contain many check points if needed
Implementation
Please include all the relevant things implemented and also rationale, clarifications / disclaimers etc. related to the approach used. It could be as self code companion comments
Screenshots or Gifs
Please include all the media files to give some context about what's being implemented or fixed. It's not mandatory to upload screenshots or gifs, but for most of the cases it becomes really handy to get into the scope of the feature / bug being fixed and also it's REALLY useful for UI related PRs
Testing
Please describe the manual tests that you ran to verify your changes
SNAPSHOTupstream dependencies if needed) through testapp/demo app and run all activities to avoid regressionsChecklist
CHANGELOGincluding this PR