Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

RecyclerView integration #13132

Merged
merged 3 commits into from
Nov 14, 2018
Merged

RecyclerView integration #13132

merged 3 commits into from
Nov 14, 2018

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Oct 18, 2018

This PR adds an POC integration with RecyclerView.

ezgif com-video-to-gif

Note that we advice against adding a MapView as part of RecyclerView. It's better for these use-cases to use a bitmap of a map instead (eg. generated with the MapSnapshotter).

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Oct 18, 2018
@tobrun tobrun added this to the android-v6.7.0 milestone Oct 18, 2018
@tobrun tobrun self-assigned this Oct 18, 2018
@tobrun tobrun requested review from zugaldia and langsmith October 18, 2018 11:42
@tobrun
Copy link
Member Author

tobrun commented Oct 18, 2018

The tests were failing due to the FileSource remained being active after an instrumentation test. When a map is visible to the user in a recyclerview and the activity is destroyed we need to call onPause and onStop. The issue however is that they can already have been stopped or paused by scrolling the MapView out of bounds. e50ee45 addresses this issue by maintaining state if the FileSource has been activated/deactivated.

@tobrun tobrun requested a review from LukasPaczos October 23, 2018 08:36
@@ -101,6 +101,7 @@
private MapZoomButtonController mapZoomButtonController;
@Nullable
private Bundle savedInstanceState;
private boolean isActivated;
Copy link
Contributor

Choose a reason for hiding this comment

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

isStarted might be better suited here.

* </p>
*/
@SuppressLint("ClickableViewAccessibility")
class RecyclerViewActivity : AppCompatActivity() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This activity leaks context when rotated. Also, when map element is out of view and the activity is recreated (rotated), the map is not loading anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looked into this with this PR and solution for this is #13133

override fun onDestroy() {
super.onDestroy()
// to perform cleanup, we need to call MapView#onDestroy
(recyclerView.adapter as ItemAdapter).onDestroy()
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like some of the problems mentioned above might stem from calling MapView#onDestroy only when activity is destroyed. We should call that whenever the MapView holder is destroyed? Maybe #onViewRecycled? Not sure here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue above was the view measurement from #13133 . View recycling still keeps the View in memory, it's detached but keeps it alive. OnDestroy doesn't match that step at that time completely. Destroying at view recycle time would also result in recreating it completely while scrolling up and down the list. This results in hick-up during scrolling and seeing the black surface for a second. I initially went with that approach but was not satisfied with the experience.

protected boolean canScroll(View v, boolean checkV, int dx, int x, int y) {
return v instanceof SurfaceView || v instanceof PagerTabStrip || (super.canScroll(v, checkV, dx, x, y));
}
// @Override
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to remove below lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad, left over from testing, these lines should remain

@tobrun tobrun force-pushed the tvn-recyclerview branch 5 times, most recently from 80e7cf5 to c68062d Compare October 29, 2018 08:28
@zugaldia zugaldia removed their request for review October 30, 2018 18:30
@LukasPaczos
Copy link
Contributor

LukasPaczos commented Oct 31, 2018

Looks good, but when testing the NestedViewPagerActivity I'm running into an issue, where fragments are not loading after orientation change unless they go out of cache bounds and are re-added:
ezgif com-video-to-gif 14

I believe this might be a recycler + view pager setup issue, rather than maps issue, but wanted to clear this up before we merge these examples.

@tobrun
Copy link
Member Author

tobrun commented Nov 13, 2018

@LukasPaczos that issue stemmed from using FragmentPagerAdapter instead of FragmentStatePagerAdapter. Fixed now, see gif below:

ezgif com-video-to-asdgif

Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

Thanks for verifying!

@tobrun tobrun merged commit 6ea511b into master Nov 14, 2018
@tobrun tobrun deleted the tvn-recyclerview branch November 14, 2018 15:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants