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

Introduce a setConnected method to manually set a connected flag #6618

Merged
merged 3 commits into from
Oct 11, 2016

Conversation

zugaldia
Copy link
Member

@zugaldia zugaldia commented Oct 6, 2016

Currently, the PR only covers the usecase where an app handles its connectivity state internally and bypasses ConnectivityManager.

Fixes #6617.

/cc: @danswick @mapbox/android

@zugaldia zugaldia added feature Android Mapbox Maps SDK for Android ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold offline labels Oct 6, 2016
@mention-bot
Copy link

@zugaldia, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bleege, @cammace and @tobrun to be potential reviewers.

@zugaldia
Copy link
Member Author

zugaldia commented Oct 6, 2016

For testing purposes, a new SNAPSHOT build has been triggered via https://www.bitrise.io/build/a66f375f4c7a5408 and it's now available in mapbox-android-sdk-4.2.0-20161006.204020-83.aar.

Copy link
Contributor

@ivovandongen ivovandongen left a comment

Choose a reason for hiding this comment

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

@zugaldia This covers the HTTPRequest case, but not the core connectivity. The receiver that is started here will still inform the core about connectivity, through NativeConnectivityListener

I should've consolidated both right away, bit of a mess.

@zugaldia
Copy link
Member Author

zugaldia commented Oct 7, 2016

This covers the HTTPRequest case, but not the core connectivity. The receiver that is started here will still inform the core about connectivity, through NativeConnectivityListener

Yes. Let's not merge this PR until we take care of the ConnectivityReceiver too (maybe instead of exposing a setConnected method, we can just simply have another constructor for MapboxAccountManager where start(Context context, String accessToken) accepts a flag?).

@ivovandongen
Copy link
Contributor

maybe instead of exposing a setConnected method, we can just simply have another constructor for MapboxAccountManager where start(Context context, String accessToken) accepts a flag

Definitely. It will resume less quickly probably and needs some testing to be sure. But this is by far the easiest way for now.

@zugaldia
Copy link
Member Author

zugaldia commented Oct 7, 2016

@ivovandongen Thinking about it, the problem with the constructor approach is that it can only be set once, and if the app connectivity state changes, there's no way to inform MapboxAccountManager of the update. I've added a quick check inside ConnectivityReceiver which helps with the original issue in OP. This still has good room for refactoring before merging.

@zugaldia
Copy link
Member Author

zugaldia commented Oct 7, 2016

Updated build happened via https://www.bitrise.io/build/5a1b3138c006b2e2 and it's now available here: mapbox-android-sdk-4.2.0-20161007.193849-85.aar.

@zugaldia
Copy link
Member Author

zugaldia commented Oct 10, 2016

The previous snapshot was accidentally built from the wrong branch (master), a correct build happened via https://www.bitrise.io/build/337c5e7fdb9d04a7 and it's now available here: mapbox-android-sdk-4.2.0-20161010.174709-89.aar.

@zugaldia zugaldia merged commit 4cc7e7d into master Oct 11, 2016
@zugaldia zugaldia deleted the 6617-connectivity branch October 11, 2016 13:17
@zugaldia zugaldia removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Oct 25, 2016
@zugaldia zugaldia mentioned this pull request Oct 25, 2016
9 tasks
@zugaldia zugaldia mentioned this pull request Nov 14, 2016
8 tasks
@zugaldia zugaldia modified the milestone: android-v4.2.0 Nov 15, 2016
@cammace cammace mentioned this pull request Dec 14, 2016
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android feature offline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants