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

add FileSource pause/resume #9977

Merged
merged 1 commit into from
Oct 17, 2017
Merged

add FileSource pause/resume #9977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import com.mapbox.mapboxsdk.maps.widgets.MyLocationView;
import com.mapbox.mapboxsdk.maps.widgets.MyLocationViewSettings;
import com.mapbox.mapboxsdk.net.ConnectivityReceiver;
import com.mapbox.mapboxsdk.storage.FileSource;
import com.mapbox.services.android.telemetry.MapboxTelemetry;

import java.lang.annotation.Retention;
Expand Down Expand Up @@ -260,6 +261,7 @@ public void onSaveInstanceState(@NonNull Bundle outState) {
@UiThread
public void onStart() {
ConnectivityReceiver.instance(getContext()).activate();
FileSource.getInstance(getContext()).activate();
if (mapboxMap != null) {
mapboxMap.onStart();
}
Expand Down Expand Up @@ -288,6 +290,7 @@ public void onPause() {
public void onStop() {
mapboxMap.onStop();
ConnectivityReceiver.instance(getContext()).deactivate();
FileSource.getInstance(getContext()).deactivate();
}

/**
Expand Down
9 changes: 9 additions & 0 deletions .../android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineRegion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,10 +300,19 @@ public void run() {

/**
* Pause or resume downloading of regional resources.
* <p>
* After a download has been completed, you are required to reset the state of the region to STATE_INACTIVE.
* </p>
*
* @param state the download state
*/
public void setDownloadState(@DownloadState int state) {
if (state == STATE_ACTIVE) {
fileSource.activate();
} else {
fileSource.deactivate();
}

this.state = state;
setOfflineRegionDownloadState(state);
}
Expand Down
24 changes: 23 additions & 1 deletion ...orm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/storage/FileSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public static String getCachePath(Context context) {
MapboxConstants.KEY_META_DATA_SET_STORAGE_EXTERNAL,
MapboxConstants.DEFAULT_SET_STORAGE_EXTERNAL);
} catch (PackageManager.NameNotFoundException exception) {
Timber.e(exception,"Failed to read the package metadata: ");
Timber.e(exception, "Failed to read the package metadata: ");
} catch (Exception exception) {
Timber.e(exception, "Failed to read the storage key: ");
}
Expand Down Expand Up @@ -119,17 +119,39 @@ public static boolean isExternalStorageReadable() {
}

private long nativePtr;
private long activeCounter;
private boolean wasPaused;

private FileSource(String cachePath, AssetManager assetManager) {
initialize(Mapbox.getAccessToken(), cachePath, assetManager);
}

public void activate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would opt to make redundant calls possible instead of doing counting here. I suggested a patch to do so here: 034551f

This will also subtly break if the file source would be paused/resumed from another place in the future.

cc @jfirebaugh

Copy link
Member Author

@tobrun tobrun Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@jfirebaugh could you add your preference on who is responsible for ref counting? the binding or core?

Copy link
Contributor

Choose a reason for hiding this comment

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

As noted in #10174 (review), my preference is to keep pause/resume as strict as possible -- in particular callable only by the creating thread -- because they otherwise get very difficult to reason about.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ivovandongen can you 👍 this if you are onboard on adding this?

activeCounter++;
if (activeCounter == 1 && wasPaused) {
wasPaused = false;
resume();
}
}

public void deactivate() {
activeCounter--;
if (activeCounter == 0) {
wasPaused = true;
pause();
}
}

public native void setAccessToken(@NonNull String accessToken);

public native String getAccessToken();

public native void setApiBaseUrl(String baseUrl);

private native void resume();

private native void pause();

/**
* Sets a callback for transforming URLs requested from the internet
* <p>
Expand Down
3 changes: 3 additions & 0 deletions ...KTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/offline/OfflineActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ public void onStatusChanged(OfflineRegionStatus status) {
if (status.isComplete()) {
// Download complete
endProgress("Region downloaded successfully.");
offlineRegion.setDownloadState(OfflineRegion.STATE_INACTIVE);
Copy link
Member

Choose a reason for hiding this comment

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

@tobrun Ref counting aside, this looks like new behavior for region downloads. Are there any side effects if a developer (like until now) doesn't manually set the region as inactive?

Copy link
Member Author

@tobrun tobrun Oct 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This will result in not pausing the Filesource, in other words will match the same behavior as it is today. For this reason I'm not seeing this as a semver change but an enhancement to API.

Copy link
Member

Choose a reason for hiding this comment

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

@tobrun Makes sense, thanks for the clarification.

offlineRegion.setObserver(null);
return;
} else if (status.isRequiredResourceCountPrecise()) {
Expand All @@ -281,11 +282,13 @@ public void onStatusChanged(OfflineRegionStatus status) {
@Override
public void onError(OfflineRegionError error) {
Timber.e("onError: %s, %s", error.getReason(), error.getMessage());
offlineRegion.setDownloadState(OfflineRegion.STATE_INACTIVE);
}

@Override
public void mapboxTileCountLimitExceeded(long limit) {
Timber.e("Mapbox tile count limit exceeded: %s", limit);
offlineRegion.setDownloadState(OfflineRegion.STATE_INACTIVE);
}
});

Expand Down
14 changes: 11 additions & 3 deletions platform/android/src/file_source.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
#include "asset_manager_file_source.hpp"
#include "jni/generic_global_ref_deleter.hpp"

#include <string>

namespace mbgl {
namespace android {

Expand Down Expand Up @@ -64,6 +62,14 @@ void FileSource::setResourceTransform(jni::JNIEnv& env, jni::Object<FileSource::
}
}

void FileSource::resume(jni::JNIEnv&) {
fileSource->resume();
}

void FileSource::pause(jni::JNIEnv&) {
fileSource->pause();
}

jni::Class<FileSource> FileSource::javaClass;

FileSource* FileSource::getNativePeer(jni::JNIEnv& env, jni::Object<FileSource> jFileSource) {
Expand Down Expand Up @@ -93,7 +99,9 @@ void FileSource::registerNative(jni::JNIEnv& env) {
METHOD(&FileSource::getAccessToken, "getAccessToken"),
METHOD(&FileSource::setAccessToken, "setAccessToken"),
METHOD(&FileSource::setAPIBaseUrl, "setApiBaseUrl"),
METHOD(&FileSource::setResourceTransform, "setResourceTransform")
METHOD(&FileSource::setResourceTransform, "setResourceTransform"),
METHOD(&FileSource::resume, "resume"),
METHOD(&FileSource::pause, "pause")
);
}

Expand Down
4 changes: 4 additions & 0 deletions platform/android/src/file_source.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ class FileSource {

void setResourceTransform(jni::JNIEnv&, jni::Object<FileSource::ResourceTransformCallback>);

void resume(jni::JNIEnv&);

void pause(jni::JNIEnv&);

static jni::Class<FileSource> javaClass;

static FileSource* getNativePeer(jni::JNIEnv&, jni::Object<FileSource>);
Expand Down