Fix memory leak due to builder with Context in constructor #3023
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
kmadsen
force-pushed
the
km-fix-directions-format-leak
branch
from
3fd8e1f
to
5a2743f
Compare
kmadsen
changed the title
Codecov Report
@@ Coverage Diff @@
## master #3023 +/- ##
============================================
- Coverage 36.76% 36.75% -0.02%
Complexity 2216 2216
============================================
Files 554 554
Lines 19955 19955
Branches 1886 1886
============================================
- Hits 7337 7334 -3
- Misses 11782 11784 +2
- Partials 836 837 +1 |
Guardiola31337
approved these changes
May 22, 2020
kmadsen
force-pushed
the
km-fix-directions-format-leak
branch
from
0e05c82
to
1f396cf
Compare
libnavigation-core/src/main/java/com/mapbox/navigation/core/internal/MapboxDistanceFormatter.kt
Outdated
Show resolved
Hide resolved
| * @param locale the locale to use for localization of distance resources | ||
| * @param unitType to use, or UNDEFINED to use default for locale country | ||
| * @param roundingIncrement increment by which to round small distances | ||
| * @param builder used for updating options | ||
| */ | ||
| class MapboxDistanceFormatter private constructor( |
There was a problem hiding this comment.
From #2709 (comment)
For example, MapboxDistanceFormatter will be renamed to DistanceFormatOptions which can be passed to components requiring distance formatting.
Do we still want to do that? Maybe in a separate ticket?
There was a problem hiding this comment.
Yeah finding MapboxDistanceFormatter to be a larger project. So the progress can be tracked here #3016
There was a problem hiding this comment.
MapboxDistanceFormatter is a service, so renaming to DistanceFormatOptions actually doesn't make sense. updating the source ticket
kmadsen
force-pushed
the
km-fix-directions-format-leak
branch
from
816b150
to
4d9041f
Compare
kmadsen
force-pushed
the
km-fix-directions-format-leak
branch
from
4d9041f
to
619e9db
Compare
Guardiola31337
approved these changes
May 22, 2020
11 tasks
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
Removing the builder from the constructor. It's not a good idea to add the Context to the builder because it creates memory leaks.Taking @Guardiola31337 suggestion to throw it in the build(..) function. That function converts the context to an applicationContext to save memory leaks.This issue was introduced here #3015
There is an outstanding issue for refactoring the MapboxDistanceFormatter #3016
bug,feature,new API(s),SEMVER, etc.)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