[ios] Add a flag to pause network requests. #15650

fabian-guerra · 2019-09-17T21:40:08Z

Adds the iOS equivalent of Android's setConnected flag. Unlike Android iOS don't have a ConnectivityManager to turn on/off the network requests. The network implementation checks if the stopsRequests flag is set to YES the core network library will stop making request. This enforce the usage of any previously cached data.

julianrex · 2019-09-18T14:51:02Z

FYI @tobrun

julianrex · 2019-09-18T22:20:10Z

I see mbgl::OnlineFileSource::Impl has an online instance variable - though it looks from code comments that this is really for testing only.

I'm wondering if in addition to setOnlineStatus we could add a getOnlineStatus, that uses the proposed MGLNetworkConfiguration.stopsRequests property?

@tmpsantos would you mind adding your 2 cents here?

tmpsantos · 2019-09-19T15:56:59Z

platform/darwin/src/http_file_source.mm

@@ -229,6 +229,10 @@ void cancel() {
 std::unique_ptr<AsyncRequest> HTTPFileSource::request(const Resource& resource, Callback callback) {
    auto request = std::make_unique<HTTPRequest>(callback);
    auto shared = request->shared; // Explicit copy so that it also gets copied into the completion handler block below.
+
+    if ([MGLNetworkConfiguration sharedManager].stopsRequests) {
+        return std::move(request);


The problem with this implementation is when you resume, it will not re-request the previously requested (but not fulfilled) network requests.

We have an API for this: mbgl::NetworkStatus::Set(NetworkStatus::Status::Offline).

tobrun · 2019-09-19T16:06:06Z

The Mapbox#setConnected(true) was introduced for other reasons. We had a client that had a device without an internet connection and we had to force an a connected state for that device (normally we rely on the connectivity state change listener that Android OS exposes).

The api that @tmpsantos mentiones we expose on the Map object (NativeMapView#setReachable(boolean). Imo we should update the Mapbox#setConnected to align with the API on the map object.

friedbunny

We have to add tests for this before it can be accepted.

julianrex · 2019-09-19T20:34:20Z

platform/darwin/src/MGLNetworkConfiguration.mm

+
+- (BOOL)stopsRequests {
+    auto status = mbgl::NetworkStatus::Get();
+    if (status == mbgl::NetworkStatus::Status::Offline) {


This can just be return (status == mbgl::NetworkStatus::Status::Offline);

platform/darwin/src/MGLNetworkConfiguration.h

tmpsantos · 2019-09-20T14:42:41Z

platform/darwin/src/MGLNetworkConfiguration.mm

@@ -66,6 +67,19 @@ - (NSURLSessionConfiguration *)defaultSessionConfiguration {
    return sessionConfiguration;
 }

+- (void)setStopsRequests:(BOOL)stopsRequests {
+    if (stopsRequests) {
+        mbgl::NetworkStatus::Set(mbgl::NetworkStatus::Status::Offline);


I could be wrong, but this will be Offline forever. Because once you set the status to Offline, the following code will always be true.

- (BOOL)stopsRequests { auto status = mbgl::NetworkStatus::Get(); return status == mbgl::NetworkStatus::Status::Offline; }

tmpsantos · 2019-09-20T14:45:20Z

platform/darwin/src/MGLNetworkConfiguration.mm

@@ -66,6 +67,19 @@ - (NSURLSessionConfiguration *)defaultSessionConfiguration {
    return sessionConfiguration;
 }

+- (void)setStopsRequests:(BOOL)stopsRequests {


stopRequests is not really a great name for this API, because is a double negation.

I agree with this; @tobrun will setConnected remain the same?

@fabian-guerra how about we go with a connected property? My concern here is that this might be confused whether there is a valid connection (a la reachability).

I think we should match Android in this instance; let's discuss.

friedbunny · 2019-09-20T23:19:13Z

platform/ios/test/MGLNetworkConfigurationTests.mm

+    networkStatus = mbgl::NetworkStatus::Get();
+    offline = networkStatus == mbgl::NetworkStatus::Status::Offline ? YES : NO;
+    XCTAssertEqual([MGLNetworkConfiguration sharedManager].stopsRequests, offline);
+}


These tests look good, thanks! Have you considered implementing tests of the offline/network functionality — e.g., does enabling this property actually result in no network requests?

My understanding is that this will stop tile and related requests, but other network activity would still occur.

julianrex

Just a couple of notes.

julianrex · 2019-10-10T03:59:13Z

platform/darwin/src/MGLNetworkConfiguration.mm

@@ -66,6 +67,19 @@ - (NSURLSessionConfiguration *)defaultSessionConfiguration {
    return sessionConfiguration;
 }

+- (void)setStopsRequests:(BOOL)stopsRequests {


I think we should match Android in this instance; let's discuss.

julianrex · 2019-10-10T04:00:10Z

platform/ios/test/MGLNetworkConfigurationTests.mm

+
+    networkStatus = mbgl::NetworkStatus::Get();
+    offline = networkStatus == mbgl::NetworkStatus::Status::Offline ? YES : NO;
+    // TODO: When the double linking framework fix lands this should be replaced to an equal assert


Let's link to an issue here that gives more detail. Perhaps #15724 ?

degtiarev · 2019-11-04T10:00:12Z

It crashes the app when I am trying to use it:
MGLNetworkConfiguration.sharedManager.stopsRequests = false

julianrex · 2020-03-17T03:28:45Z

@fabian-guerra @knov Closing this PR in preference of mapbox/mapbox-gl-native-ios#217, which tracks whether this needs to be ported.

fabian-guerra · 2020-09-03T16:19:02Z

Continues in mapbox/mapbox-gl-native-ios#416

fabian-guerra added feature iOS Mapbox Maps SDK for iOS labels Sep 17, 2019

fabian-guerra requested review from friedbunny and julianrex September 17, 2019 21:40

fabian-guerra requested a review from 1ec5 as a code owner September 17, 2019 21:40

fabian-guerra requested a review from a team September 17, 2019 21:40

fabian-guerra self-assigned this Sep 17, 2019

fabian-guerra added the needs changelog Indicates PR needs a changelog entry prior to merging. label Sep 17, 2019

julianrex added this to the release-ristretto milestone Sep 17, 2019

fabian-guerra force-pushed the fabian-network-pause-14469 branch 2 times, most recently from 1ce830a to 78fd9d6 Compare September 18, 2019 18:21

julianrex requested review from tmpsantos and removed request for a team and 1ec5 September 18, 2019 22:20

tmpsantos reviewed Sep 19, 2019

View reviewed changes

friedbunny added the needs tests label Sep 19, 2019

friedbunny suggested changes Sep 19, 2019

View reviewed changes

julianrex modified the milestones: release-ristretto, release-sangria Sep 19, 2019

julianrex reviewed Sep 19, 2019

View reviewed changes

friedbunny reviewed Sep 19, 2019

View reviewed changes

platform/darwin/src/MGLNetworkConfiguration.h Show resolved Hide resolved

tmpsantos approved these changes Sep 20, 2019

View reviewed changes

tmpsantos reviewed Sep 20, 2019

View reviewed changes

friedbunny reviewed Sep 20, 2019

View reviewed changes

friedbunny self-requested a review September 20, 2019 23:19

friedbunny mentioned this pull request Sep 23, 2019

Provide a way to disable network activity for maps #7443

Open

friedbunny added the offline label Sep 23, 2019

julianrex mentioned this pull request Sep 27, 2019

[ios] Don't link libmbgl-core twice; fix CI build by exposing core classes #15724

Closed

fabian-guerra added 4 commits October 7, 2019 15:22

[ios] Add a flag to pause network requests.

0d59621

[ios] Update changelog.

d184685

[ios] Use NetworkStatus::Status for stopping network requests.

5ac1c78

[ios] Add stopsRequests tests.

53b0b2f

fabian-guerra force-pushed the fabian-network-pause-14469 branch from a4bc0c4 to 53b0b2f Compare October 7, 2019 22:47

fabian-guerra removed needs changelog Indicates PR needs a changelog entry prior to merging. needs tests labels Oct 7, 2019

fabian-guerra added 5 commits October 7, 2019 15:52

[ios] Clarify MGLNetworkConfiguration.stopsRequests documentation.

f7e1e29

[macos] Update file import.

e9565c2

[ios] Fix a network test issue caused when stopsRequests is set to YES.

e440ad8

[macos] Fix macos.cmake file import.

3e2c37b

[ios, macos] Update generate-file-lists.js script

06ad587

julianrex reviewed Oct 10, 2019

View reviewed changes

julianrex mentioned this pull request Oct 11, 2019

Decide how to address open PRs/branches in mapbox-gl-native mapbox/mapbox-gl-native-ios#11

Closed

4 tasks

friedbunny removed their request for review October 23, 2019 20:54

This was referenced Nov 5, 2019

Add setConnected() method [Android only] rnmapbox/maps#508

Merged

Feature request: Add setConnected() for iOS rnmapbox/maps#513

Closed

tobrun mentioned this pull request Mar 6, 2020

Optimise Mapbox#setConnected mapbox/mapbox-gl-native-android#230

Closed

julianrex mentioned this pull request Mar 17, 2020

Add a flag to pause network requests mapbox/mapbox-gl-native-ios#217

Closed

julianrex closed this Mar 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] Add a flag to pause network requests. #15650

[ios] Add a flag to pause network requests. #15650

fabian-guerra commented Sep 17, 2019 •

edited by friedbunny

Loading

julianrex commented Sep 18, 2019

julianrex commented Sep 18, 2019

tmpsantos Sep 19, 2019

tobrun commented Sep 19, 2019

friedbunny left a comment

julianrex Sep 19, 2019

tmpsantos Sep 20, 2019

tmpsantos Sep 20, 2019

julianrex Sep 26, 2019

julianrex Oct 10, 2019

friedbunny Sep 20, 2019

julianrex Sep 26, 2019

julianrex left a comment

julianrex Oct 10, 2019

julianrex Oct 10, 2019

degtiarev commented Nov 4, 2019 •

edited

Loading

julianrex commented Mar 17, 2020

fabian-guerra commented Sep 3, 2020

[ios] Add a flag to pause network requests. #15650

[ios] Add a flag to pause network requests. #15650

Conversation

fabian-guerra commented Sep 17, 2019 • edited by friedbunny Loading

julianrex commented Sep 18, 2019

julianrex commented Sep 18, 2019

Choose a reason for hiding this comment

tobrun commented Sep 19, 2019

friedbunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianrex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

degtiarev commented Nov 4, 2019 • edited Loading

julianrex commented Mar 17, 2020

fabian-guerra commented Sep 3, 2020

fabian-guerra commented Sep 17, 2019 •

edited by friedbunny

Loading

degtiarev commented Nov 4, 2019 •

edited

Loading