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

Remove WeakRefernce from MapboxMap and Style #1193

Merged
merged 7 commits into from
Mar 3, 2022
Merged

Remove WeakRefernce from MapboxMap and Style #1193

merged 7 commits into from
Mar 3, 2022

Conversation

Chaoba
Copy link
Contributor

@Chaoba Chaoba commented Feb 28, 2022 •
edited
Loading

Summary of changes

This pr removes the WeakRefernce form mapbox ans style, and throws MapboxMapMemoryLeakException when the nativeMap is null to notify the memory leak.

User impact (optional)

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Optimize code for java consumption (@JvmOverloads, @file:JvmName, etc).
  • Add example if relevant.
  • Document any changes to public APIs.
  • Run make update-api to update generated api files, if there's public API changes, otherwise the verify-kotlin-binary-compatibility CI will fail.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog></changelog>.
  • If this PR is a v10.[version] release branch fix / enhancement, merge it to main firstly and then port to v10.[version] release branch.

Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

PRs must be submitted under the terms of our Contributor License Agreement CLA.

@Chaoba Chaoba self-assigned this Feb 28, 2022
@Chaoba Chaoba requested a review from a team as a code owner February 28, 2022 07:50
@Chaoba Chaoba added the bug 🪲 Something isn't working label Feb 28, 2022
@Chaoba Chaoba changed the title Remove WeakRefernce from mapbox and style Remove WeakRefernce from MapboxMap and Style Feb 28, 2022
@Chaoba Chaoba force-pushed the kl-remove-weak branch 3 times, most recently from 60ad38f to 699f174 Compare February 28, 2022 13:01
yunikkk
yunikkk reviewed Feb 28, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@Chaoba Chaoba added skip changelog and removed bug 🪲 Something isn't working labels Mar 1, 2022
@pengdev pengdev requested a review from tobrun March 1, 2022 13:21
@pengdev
Copy link
Member

pengdev commented Mar 1, 2022

@Chaoba would it make sense to have a change log entry for this fix? as it introduced a new MapboxMapMemoryLeakException, and would be good to get 👁️ from @tobrun as well.

tobrun
tobrun approved these changes Mar 2, 2022
View reviewed changes
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Thank you for picking this up!

sdk-base/src/main/java/com/mapbox/maps/MapboxExceptions.kt Outdated Show resolved Hide resolved
sdk/src/main/java/com/mapbox/maps/MapboxMap.kt Outdated Show resolved Hide resolved
@Chaoba Chaoba merged commit bbaff3b into main Mar 3, 2022
@Chaoba Chaoba deleted the kl-remove-weak branch March 3, 2022 00:57
Chaoba pushed a commit that referenced this pull request Mar 3, 2022
Remove WeakRefernce from MapboxMap and Style (#1193)
63f8127 
* Remove weakReference from MapboxMap

* Remove weakReference from Style

* Update readme

* Update api files

* Remove changelog

* Revert Utils.kt

* Fix review comments
@yunikkk yunikkk mentioned this pull request Mar 21, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobrun tobrun tobrun approved these changes

@yunikkk yunikkk yunikkk approved these changes

@pengdev pengdev pengdev approved these changes

Assignees

@Chaoba Chaoba

Labels
skip changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Chaoba @pengdev @tobrun @yunikkk