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

Feature: Change path of the resources cache db (CP #13707) #13947

Merged
merged 3 commits into from Mar 13, 2019

Conversation

LukasPaczos
Copy link
Member

CPs #13707 and changes the reinitialization flow to reset the database instead of the whole FileSource.

Again, thanks a lot for the contribution @arnekaiser!

@LukasPaczos LukasPaczos added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels Feb 19, 2019
@LukasPaczos LukasPaczos added this to the release-kombucha milestone Feb 19, 2019
@LukasPaczos LukasPaczos force-pushed the lp-cp-13707-origin branch 2 times, most recently from 51dfa45 to e5fa928 Compare February 19, 2019 13:17
Copy link
Member

@tmpsantos tmpsantos left a comment

Choose a reason for hiding this comment

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

mbgl::Core part looks good in general, minor comment. We need unit tests for this.

void OfflineDatabase::changePath(const std::string& path_) {
Log::Info(Event::Database, "Changing the database path.");

statements.clear();
Copy link
Member

Choose a reason for hiding this comment

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

This is what we call on the destructor. This functions might throw and we should catch. To avoid duplicating the code, make both the destructor and this function call something like OfflineDatabase::cleanup

@tmpsantos tmpsantos removed their assignment Mar 12, 2019
@LukasPaczos LukasPaczos force-pushed the lp-cp-13707-origin branch 2 times, most recently from fa9701e to 82d4aab Compare March 13, 2019 13:34
@LukasPaczos
Copy link
Member Author

Ready for another round.

Copy link
Member

@tmpsantos tmpsantos left a comment

Choose a reason for hiding this comment

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

LGTM from the core side.

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.

Can we add some kind of instrumented unit test to this?
Have we tested updating the cachepath when a mapview is in use or while offlinedownload is ongoing?

@LukasPaczos
Copy link
Member Author

Have we tested updating the cachepath when a mapview is in use or while offlinedownload is ongoing?

The idea is that https://github.com/mapbox/mapbox-gl-native/pull/13947/files#diff-bfa4af72183147adf9f916e65d5e64c3R278 guards against those, do you see a better way?

@LukasPaczos LukasPaczos merged commit b770299 into master Mar 13, 2019
@LukasPaczos LukasPaczos deleted the lp-cp-13707-origin branch March 13, 2019 17:28
@arnekaiser
Copy link
Contributor

Thank you for the refactoring and merging @LukasPaczos ! Unfortunately I found a bug: #14297

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants