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

Cherry-pick to release agua #10633

Merged
merged 7 commits into from
Dec 6, 2017
Merged

Cherry-pick to release agua #10633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
853f824
[android] Enable map rendering when app is paused
ivovandongen Nov 20, 2017
e571277
[android] - activate filesource to list offline regions (#10531)
tobrun Nov 22, 2017
ab1b073
[android] - harden MarkerView integration by checking for null bitmap…
tobrun Nov 22, 2017
05f0ba4
[android] - use concurrent lists for camera change listeners (#10542)
tobrun Nov 29, 2017
fbd451a
[android] - handle destroying activity programmatically as part of th…
tobrun Nov 30, 2017
4e6a3ae
[android] - add FileSource activation/deactivation to MapSnapshotter,…
tobrun Dec 6, 2017
c331173
[core, ios, qt, android] Close race condition in RunLoop (issue #9620)
ChrisLoer Nov 22, 2017
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
2 changes: 1 addition & 1 deletion platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/annotations/Icon.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public String getId() {
* @return The bitmap being used for the icon.
*/
public Bitmap getBitmap() {
if (mBitmap.getConfig() != Bitmap.Config.ARGB_8888) {
if (mBitmap != null && mBitmap.getConfig() != Bitmap.Config.ARGB_8888) {
mBitmap = mBitmap.copy(Bitmap.Config.ARGB_8888, false);
}
return mBitmap;
Expand Down
51 changes: 25 additions & 26 deletions ...id/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/CameraChangeDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@

import android.support.annotation.NonNull;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;

import static com.mapbox.mapboxsdk.maps.MapboxMap.OnCameraIdleListener;
import static com.mapbox.mapboxsdk.maps.MapboxMap.OnCameraMoveCanceledListener;
Expand All @@ -15,10 +14,10 @@ class CameraChangeDispatcher implements MapboxMap.OnCameraMoveStartedListener, M

private boolean idle = true;

private final List<OnCameraMoveStartedListener> onCameraMoveStartedListenerList = new ArrayList<>();
private final List<OnCameraMoveCanceledListener> onCameraMoveCanceledListenerList = new ArrayList<>();
private final List<OnCameraMoveListener> onCameraMoveListenerList = new ArrayList<>();
private final List<OnCameraIdleListener> onCameraIdleListenerList = new ArrayList<>();
private final CopyOnWriteArrayList<OnCameraMoveStartedListener> onCameraMoveStarted = new CopyOnWriteArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

@tobrun Adding CopyOnWriteArrayList makes it thread-safe, but may result in a rare scenario when user unregisters the listener, yet still gets the callback. Do you think we should add synchronization?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't ran into any issues with that or haven't heard about any reports. The reason why I don't perceive this as an issue is that the interactions with this list allows occurs on the main thread.
The reason why we have a CopyOnWriteArrayList is to allow to remove the listener as part of it's callback to avoid a ConcurrentModificationException. Imo your remark is a tradeoff of this, happy to look more into this if there is a use-case for it.

Copy link
Contributor

@LukasPaczos LukasPaczos Dec 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The problem arises when you'd like to register/unregister on a separate thread (which in my scenario would have thrown ConcurrentModificationException before and now will just execute the callback). Since your use-case is concrete and the problem was there before anyway, just in a different form, I think we're good here.

The reason why I don't perceive this as an issue is that the interactions with this list allows occurs on the main thread.

Maybe adding @UiThread annotation when registering in MapboxMap would be nice if we expect users to register/unregister on the main thread?

Copy link
Member Author

Choose a reason for hiding this comment

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

We annotate the class with @UiThread, which is the equivalent to adding the annotation to each method of the class. Would love to see if we can improve the implementation in future iterations ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

We annotate the class with @UiThread

Missed that, totally makes sense :)

private final CopyOnWriteArrayList<OnCameraMoveCanceledListener> onCameraMoveCanceled = new CopyOnWriteArrayList<>();
private final CopyOnWriteArrayList<OnCameraMoveListener> onCameraMove = new CopyOnWriteArrayList<>();
private final CopyOnWriteArrayList<OnCameraIdleListener> onCameraIdle = new CopyOnWriteArrayList<>();

private OnCameraMoveStartedListener onCameraMoveStartedListener;
private OnCameraMoveCanceledListener onCameraMoveCanceledListener;
Expand Down Expand Up @@ -58,8 +57,8 @@ public void onCameraMoveStarted(int reason) {
}

// new API
if (!onCameraMoveStartedListenerList.isEmpty()) {
for (OnCameraMoveStartedListener cameraMoveStartedListener : onCameraMoveStartedListenerList) {
if (!onCameraMoveStarted.isEmpty()) {
for (OnCameraMoveStartedListener cameraMoveStartedListener : onCameraMoveStarted) {
cameraMoveStartedListener.onCameraMoveStarted(reason);
}
}
Expand All @@ -73,8 +72,8 @@ public void onCameraMove() {
}

// new API
if (!onCameraMoveListenerList.isEmpty() && !idle) {
for (OnCameraMoveListener cameraMoveListener : onCameraMoveListenerList) {
if (!onCameraMove.isEmpty() && !idle) {
for (OnCameraMoveListener cameraMoveListener : onCameraMove) {
cameraMoveListener.onCameraMove();
}
}
Expand All @@ -88,8 +87,8 @@ public void onCameraMoveCanceled() {
}

// new API
if (!onCameraMoveCanceledListenerList.isEmpty() && !idle) {
for (OnCameraMoveCanceledListener cameraMoveCanceledListener : onCameraMoveCanceledListenerList) {
if (!onCameraMoveCanceled.isEmpty() && !idle) {
for (OnCameraMoveCanceledListener cameraMoveCanceledListener : onCameraMoveCanceled) {
cameraMoveCanceledListener.onCameraMoveCanceled();
}
}
Expand All @@ -105,51 +104,51 @@ public void onCameraIdle() {
}

// new API
if (!onCameraIdleListenerList.isEmpty()) {
for (OnCameraIdleListener cameraIdleListener : onCameraIdleListenerList) {
if (!onCameraIdle.isEmpty()) {
for (OnCameraIdleListener cameraIdleListener : onCameraIdle) {
cameraIdleListener.onCameraIdle();
}
}
}
}

void addOnCameraIdleListener(@NonNull OnCameraIdleListener listener) {
onCameraIdleListenerList.add(listener);
onCameraIdle.add(listener);
}

void removeOnCameraIdleListener(@NonNull OnCameraIdleListener listener) {
if (onCameraIdleListenerList.contains(listener)) {
onCameraIdleListenerList.remove(listener);
if (onCameraIdle.contains(listener)) {
onCameraIdle.remove(listener);
}
}

void addOnCameraMoveCancelListener(OnCameraMoveCanceledListener listener) {
onCameraMoveCanceledListenerList.add(listener);
onCameraMoveCanceled.add(listener);
}

void removeOnCameraMoveCancelListener(OnCameraMoveCanceledListener listener) {
if (onCameraMoveCanceledListenerList.contains(listener)) {
onCameraMoveCanceledListenerList.remove(listener);
if (onCameraMoveCanceled.contains(listener)) {
onCameraMoveCanceled.remove(listener);
}
}

void addOnCameraMoveStartedListener(OnCameraMoveStartedListener listener) {
onCameraMoveStartedListenerList.add(listener);
onCameraMoveStarted.add(listener);
}

void removeOnCameraMoveStartedListener(OnCameraMoveStartedListener listener) {
if (onCameraMoveStartedListenerList.contains(listener)) {
onCameraMoveStartedListenerList.remove(listener);
if (onCameraMoveStarted.contains(listener)) {
onCameraMoveStarted.remove(listener);
}
}

void addOnCameraMoveListener(OnCameraMoveListener listener) {
onCameraMoveListenerList.add(listener);
onCameraMove.add(listener);
}

void removeOnCameraMoveListener(OnCameraMoveListener listener) {
if (onCameraMoveListenerList.contains(listener)) {
onCameraMoveListenerList.remove(listener);
if (onCameraMove.contains(listener)) {
onCameraMove.remove(listener);
}
}
}
17 changes: 15 additions & 2 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 @@ -365,6 +365,10 @@ public void onStart() {
if (mapboxMap != null) {
mapboxMap.onStart();
}

if (mapRenderer != null) {
mapRenderer.onStart();
}
}

/**
Expand Down Expand Up @@ -396,6 +400,11 @@ public void onStop() {
// map was destroyed before it was started
mapboxMap.onStop();
}

if (mapRenderer != null) {
mapRenderer.onStop();
}

ConnectivityReceiver.instance(getContext()).deactivate();
FileSource.getInstance(getContext()).deactivate();
}
Expand All @@ -407,8 +416,12 @@ public void onStop() {
public void onDestroy() {
destroyed = true;
mapCallback.clearOnMapReadyCallbacks();
nativeMapView.destroy();
nativeMapView = null;

if (nativeMapView != null) {
// null when destroying an activity programmatically mapbox-navigation-android/issues/503
nativeMapView.destroy();
nativeMapView = null;
}

if (mapRenderer != null) {
mapRenderer.onDestroy();
Expand Down
8 changes: 8 additions & 0 deletions ...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/renderer/MapRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ public MapRenderer(Context context) {
nativeInitialize(this, fileSource, pixelRatio, programCacheDir);
}

public void onStart() {
// Implement if needed
}

public void onPause() {
// Implement if needed
}
Expand All @@ -41,6 +45,10 @@ public void onResume() {
// Implement if needed
}

public void onStop() {
// Implement if needed
}

public void onDestroy() {
// Implement if needed
}
Expand Down
4 changes: 2 additions & 2 deletions .../main/java/com/mapbox/mapboxsdk/maps/renderer/glsurfaceview/GLSurfaceViewMapRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ public GLSurfaceViewMapRenderer(Context context, GLSurfaceView glSurfaceView) {
}

@Override
public void onPause() {
public void onStop() {
glSurfaceView.onPause();
}

@Override
public void onResume() {
public void onStart() {
glSurfaceView.onResume();
}

Expand Down
4 changes: 2 additions & 2 deletions .../src/main/java/com/mapbox/mapboxsdk/maps/renderer/textureview/TextureViewMapRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,15 @@ public void queueEvent(Runnable runnable) {
* {@inheritDoc}
*/
@Override
public void onPause() {
public void onStop() {
renderThread.onPause();
}

/**
* {@inheritDoc}
*/
@Override
public void onResume() {
public void onStart() {
renderThread.onResume();
}

Expand Down
7 changes: 5 additions & 2 deletions ...android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/offline/OfflineManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,16 @@ private Handler getHandler() {
*
* @param callback the callback to be invoked
*/
public void listOfflineRegions(@NonNull
final ListOfflineRegionsCallback callback) {
public void listOfflineRegions(@NonNull final ListOfflineRegionsCallback callback) {
fileSource.activate();
listOfflineRegions(fileSource, new ListOfflineRegionsCallback() {

@Override
public void onList(final OfflineRegion[] offlineRegions) {
getHandler().post(new Runnable() {
@Override
public void run() {
fileSource.deactivate();
callback.onList(offlineRegions);
}
});
Expand All @@ -168,6 +169,7 @@ public void onError(final String error) {
getHandler().post(new Runnable() {
@Override
public void run() {
fileSource.deactivate();
callback.onError(error);
}
});
Expand Down Expand Up @@ -241,6 +243,7 @@ private boolean isValidOfflineRegionDefinition(OfflineRegionDefinition definitio
/**
* Changing or bypassing this limit without permission from Mapbox is prohibited
* by the Mapbox Terms of Service.
*
* @param limit the new tile count limit.
*/
public native void setOfflineMapboxTileCountLimit(long limit);
Expand Down
24 changes: 2 additions & 22 deletions ...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 @@ -6,10 +6,8 @@
import android.content.res.AssetManager;
import android.os.Environment;
import android.support.annotation.NonNull;

import com.mapbox.mapboxsdk.Mapbox;
import com.mapbox.mapboxsdk.constants.MapboxConstants;

import timber.log.Timber;

/**
Expand Down Expand Up @@ -119,39 +117,21 @@ 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() {
activeCounter++;
if (activeCounter == 1 && wasPaused) {
wasPaused = false;
resume();
}
}
public native void activate();

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

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
19 changes: 15 additions & 4 deletions platform/android/src/file_source.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,22 @@ void FileSource::setResourceTransform(jni::JNIEnv& env, jni::Object<FileSource::
}

void FileSource::resume(jni::JNIEnv&) {
fileSource->resume();
if (!activationCounter) {
activationCounter = optional<int>(1) ;
return;
}

activationCounter.value()++;
if (activationCounter == 1) {
fileSource->resume();
}
}

void FileSource::pause(jni::JNIEnv&) {
fileSource->pause();
activationCounter.value()--;
if (activationCounter == 0) {
fileSource->pause();
}
}

jni::Class<FileSource> FileSource::javaClass;
Expand Down Expand Up @@ -100,8 +111,8 @@ void FileSource::registerNative(jni::JNIEnv& env) {
METHOD(&FileSource::setAccessToken, "setAccessToken"),
METHOD(&FileSource::setAPIBaseUrl, "setApiBaseUrl"),
METHOD(&FileSource::setResourceTransform, "setResourceTransform"),
METHOD(&FileSource::resume, "resume"),
METHOD(&FileSource::pause, "pause")
METHOD(&FileSource::resume, "activate"),
METHOD(&FileSource::pause, "deactivate")
);
}

Expand Down
1 change: 1 addition & 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 @@ -54,6 +54,7 @@ class FileSource {
static void registerNative(jni::JNIEnv&);

private:
optional<int> activationCounter;
std::unique_ptr<Actor<ResourceTransform>> resourceTransform;
std::unique_ptr<mbgl::DefaultFileSource> fileSource;
};
Expand Down
6 changes: 4 additions & 2 deletions platform/android/src/run_loop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,10 @@ LOOP_HANDLE RunLoop::getLoopHandle() {
}

void RunLoop::push(std::shared_ptr<WorkTask> task) {
withMutex([&] { queue.push(std::move(task)); });
impl->wake();
withMutex([&] {
queue.push(std::move(task));
impl->wake();
});
}

void RunLoop::run() {
Expand Down
Loading