Skip to content

[image_picker] Move disk accesses to background thread #4058

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions packages/image_picker/image_picker_android/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.8.6+17

* Moves disk accesses to background thread.

## 0.8.6+16

* Fixes crashes caused by `SecurityException` when calling `getPathFromUri()`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import android.content.Context;
import android.content.SharedPreferences;
import android.net.Uri;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import java.util.ArrayList;
Expand Down Expand Up @@ -51,10 +52,10 @@ public enum CacheType {
@VisibleForTesting
static final String SHARED_PREFERENCES_NAME = "flutter_image_picker_shared_preference";

private final SharedPreferences prefs;
private final @NonNull Context context;

ImagePickerCache(Context context) {
prefs = context.getSharedPreferences(SHARED_PREFERENCES_NAME, Context.MODE_PRIVATE);
ImagePickerCache(final @NonNull Context context) {
this.context = context;
}

void saveType(CacheType type) {
Expand All @@ -69,10 +70,14 @@ void saveType(CacheType type) {
}

private void setType(String type) {
final SharedPreferences prefs =
context.getSharedPreferences(SHARED_PREFERENCES_NAME, Context.MODE_PRIVATE);
prefs.edit().putString(SHARED_PREFERENCE_TYPE_KEY, type).apply();
}

void saveDimensionWithOutputOptions(Messages.ImageSelectionOptions options) {
final SharedPreferences prefs =
context.getSharedPreferences(SHARED_PREFERENCES_NAME, Context.MODE_PRIVATE);
SharedPreferences.Editor editor = prefs.edit();
if (options.getMaxWidth() != null) {
editor.putLong(
Expand All @@ -87,16 +92,21 @@ void saveDimensionWithOutputOptions(Messages.ImageSelectionOptions options) {
}

void savePendingCameraMediaUriPath(Uri uri) {
final SharedPreferences prefs =
context.getSharedPreferences(SHARED_PREFERENCES_NAME, Context.MODE_PRIVATE);
prefs.edit().putString(SHARED_PREFERENCE_PENDING_IMAGE_URI_PATH_KEY, uri.getPath()).apply();
}

String retrievePendingCameraMediaUriPath() {

final SharedPreferences prefs =
context.getSharedPreferences(SHARED_PREFERENCES_NAME, Context.MODE_PRIVATE);
return prefs.getString(SHARED_PREFERENCE_PENDING_IMAGE_URI_PATH_KEY, "");
}

void saveResult(
@Nullable ArrayList<String> path, @Nullable String errorCode, @Nullable String errorMessage) {
final SharedPreferences prefs =
context.getSharedPreferences(SHARED_PREFERENCES_NAME, Context.MODE_PRIVATE);

SharedPreferences.Editor editor = prefs.edit();
if (path != null) {
Expand All @@ -113,13 +123,18 @@ void saveResult(
}

void clear() {
final SharedPreferences prefs =
context.getSharedPreferences(SHARED_PREFERENCES_NAME, Context.MODE_PRIVATE);
prefs.edit().clear().apply();
}

Map<String, Object> getCacheMap() {
Map<String, Object> resultMap = new HashMap<>();
boolean hasData = false;

final SharedPreferences prefs =
context.getSharedPreferences(SHARED_PREFERENCES_NAME, Context.MODE_PRIVATE);

if (prefs.contains(FLUTTER_IMAGE_PICKER_IMAGE_PATH_KEY)) {
final Set<String> imagePathList =
prefs.getStringSet(FLUTTER_IMAGE_PICKER_IMAGE_PATH_KEY, null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ private static class PendingCallState {
@VisibleForTesting final String fileProviderName;

private final @NonNull Activity activity;
@VisibleForTesting final @NonNull File externalFilesDirectory;
private final @NonNull ImageResizer imageResizer;
private final @NonNull ImagePickerCache cache;
private final PermissionManager permissionManager;
Expand Down Expand Up @@ -141,12 +140,10 @@ interface OnPathReadyListener {

public ImagePickerDelegate(
final @NonNull Activity activity,
final @NonNull File externalFilesDirectory,
final @NonNull ImageResizer imageResizer,
final @NonNull ImagePickerCache cache) {
this(
activity,
externalFilesDirectory,
imageResizer,
null,
null,
Expand Down Expand Up @@ -195,7 +192,6 @@ public void getFullImagePath(final Uri imageUri, final OnPathReadyListener liste
@VisibleForTesting
ImagePickerDelegate(
final @NonNull Activity activity,
final @NonNull File externalFilesDirectory,
final @NonNull ImageResizer imageResizer,
final @Nullable ImageSelectionOptions pendingImageOptions,
final @Nullable VideoSelectionOptions pendingVideoOptions,
Expand All @@ -206,7 +202,6 @@ public void getFullImagePath(final Uri imageUri, final OnPathReadyListener liste
final FileUtils fileUtils,
final ExecutorService executor) {
this.activity = activity;
this.externalFilesDirectory = externalFilesDirectory;
this.imageResizer = imageResizer;
this.fileProviderName = activity.getPackageName() + ".flutter.image_provider";
if (result != null) {
Expand Down Expand Up @@ -493,6 +488,7 @@ private File createTemporaryWritableVideoFile() {
private File createTemporaryWritableFile(String suffix) {
String filename = UUID.randomUUID().toString();
File image;
File externalFilesDirectory = activity.getCacheDir();

try {
externalFilesDirectory.mkdirs();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import io.flutter.plugins.imagepicker.Messages.ImagePickerApi;
import io.flutter.plugins.imagepicker.Messages.Result;
import io.flutter.plugins.imagepicker.Messages.SourceSpecification;
import java.io.File;
import java.util.List;

@SuppressWarnings("deprecation")
Expand Down Expand Up @@ -266,10 +265,9 @@ private void tearDown() {
final ImagePickerDelegate constructDelegate(final Activity setupActivity) {
final ImagePickerCache cache = new ImagePickerCache(setupActivity);

final File externalFilesDirectory = setupActivity.getCacheDir();
final ExifDataCopier exifDataCopier = new ExifDataCopier();
final ImageResizer imageResizer = new ImageResizer(externalFilesDirectory, exifDataCopier);
return new ImagePickerDelegate(setupActivity, externalFilesDirectory, imageResizer, cache);
final ImageResizer imageResizer = new ImageResizer(setupActivity, exifDataCopier);
return new ImagePickerDelegate(setupActivity, imageResizer, cache);
}

private @Nullable ImagePickerDelegate getImagePickerDelegate() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package io.flutter.plugins.imagepicker;

import android.content.Context;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import android.util.Log;
Expand All @@ -16,11 +17,11 @@
import java.io.IOException;

class ImageResizer {
private final File externalFilesDirectory;
private final Context context;
Copy link
Contributor

Choose a reason for hiding this comment

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

It might have been simpler to keep this class as it was, and make the ImageResizer on demand instead. But this is fine too.

private final ExifDataCopier exifDataCopier;

ImageResizer(File externalFilesDirectory, ExifDataCopier exifDataCopier) {
this.externalFilesDirectory = externalFilesDirectory;
ImageResizer(final @NonNull Context context, final @NonNull ExifDataCopier exifDataCopier) {
this.context = context;
this.exifDataCopier = exifDataCopier;
}

Expand Down Expand Up @@ -193,7 +194,9 @@ private File createImageOnExternalDirectory(String name, Bitmap bitmap, int imag
saveAsPNG ? Bitmap.CompressFormat.PNG : Bitmap.CompressFormat.JPEG,
imageQuality,
outputStream);
File imageFile = createFile(externalFilesDirectory, name);

File cacheDirectory = context.getCacheDir();
File imageFile = createFile(cacheDirectory, name);
FileOutputStream fileOutput = createOutputStream(imageFile);
fileOutput.write(outputStream.toByteArray());
fileOutput.close();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -593,9 +593,13 @@ void pickVideos(
/** Sets up an instance of `ImagePickerApi` to handle messages through the `binaryMessenger`. */
static void setup(@NonNull BinaryMessenger binaryMessenger, @Nullable ImagePickerApi api) {
{
BinaryMessenger.TaskQueue taskQueue = binaryMessenger.makeBackgroundTaskQueue();
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger, "dev.flutter.pigeon.ImagePickerApi.pickImages", getCodec());
binaryMessenger,
"dev.flutter.pigeon.ImagePickerApi.pickImages",
getCodec(),
taskQueue);
if (api != null) {
channel.setMessageHandler(
(message, reply) -> {
Expand Down Expand Up @@ -626,9 +630,13 @@ public void error(Throwable error) {
}
}
{
BinaryMessenger.TaskQueue taskQueue = binaryMessenger.makeBackgroundTaskQueue();
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger, "dev.flutter.pigeon.ImagePickerApi.pickVideos", getCodec());
binaryMessenger,
"dev.flutter.pigeon.ImagePickerApi.pickVideos",
getCodec(),
taskQueue);
if (api != null) {
channel.setMessageHandler(
(message, reply) -> {
Expand Down Expand Up @@ -659,11 +667,13 @@ public void error(Throwable error) {
}
}
{
BinaryMessenger.TaskQueue taskQueue = binaryMessenger.makeBackgroundTaskQueue();
BasicMessageChannel<Object> channel =
new BasicMessageChannel<>(
binaryMessenger,
"dev.flutter.pigeon.ImagePickerApi.retrieveLostResults",
getCodec());
getCodec(),
taskQueue);
if (api != null) {
channel.setMessageHandler(
(message, reply) -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@
import io.flutter.plugins.imagepicker.Messages.ImageSelectionOptions;
import io.flutter.plugins.imagepicker.Messages.VideoSelectionOptions;
import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.ExecutorService;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
Expand Down Expand Up @@ -74,6 +76,8 @@ public class ImagePickerDelegateTest {

AutoCloseable mockCloseable;

File externalDirectory;

private static class MockFileUriResolver implements ImagePickerDelegate.FileUriResolver {
@Override
public Uri resolveFileProviderUriForFile(String fileProviderName, File imageFile) {
Expand All @@ -87,7 +91,7 @@ public void getFullImagePath(Uri imageUri, ImagePickerDelegate.OnPathReadyListen
}

@Before
public void setUp() {
public void setUp() throws IOException {
mockCloseable = MockitoAnnotations.openMocks(this);

mockStaticFile = Mockito.mockStatic(File.class);
Expand All @@ -98,6 +102,11 @@ public void setUp() {
when(mockActivity.getPackageName()).thenReturn("com.example.test");
when(mockActivity.getPackageManager()).thenReturn(mock(PackageManager.class));

TemporaryFolder temporaryFolder = new TemporaryFolder();
temporaryFolder.create();
externalDirectory = temporaryFolder.newFolder("image_picker_cache");
when(mockActivity.getCacheDir()).thenReturn(externalDirectory);

when(mockFileUtils.getPathFromUri(any(Context.class), any(Uri.class)))
.thenReturn("pathFromUri");

Expand Down Expand Up @@ -300,8 +309,7 @@ public void takeImageWithCamera_writesImageToCacheDirectory() {
delegate.takeImageWithCamera(DEFAULT_IMAGE_OPTIONS, mockResult);

mockStaticFile.verify(
() -> File.createTempFile(any(), eq(".jpg"), eq(new File("/image_picker_cache"))),
times(1));
() -> File.createTempFile(any(), eq(".jpg"), eq(externalDirectory)), times(1));
}

@Test
Expand Down Expand Up @@ -674,7 +682,6 @@ public void onActivityResult_withUnknownRequest_returnsFalse() {
private ImagePickerDelegate createDelegate() {
return new ImagePickerDelegate(
mockActivity,
new File("/image_picker_cache"),
mockImageResizer,
null,
null,
Expand All @@ -690,7 +697,6 @@ private ImagePickerDelegate createDelegateWithPendingResultAndOptions(
@Nullable ImageSelectionOptions imageOptions, @Nullable VideoSelectionOptions videoOptions) {
return new ImagePickerDelegate(
mockActivity,
new File("/image_picker_cache"),
mockImageResizer,
imageOptions,
videoOptions,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

package io.flutter.plugins.imagepicker;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.IsEqual.equalTo;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
Expand All @@ -14,7 +12,6 @@
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;
Expand All @@ -30,7 +27,6 @@
import io.flutter.plugins.imagepicker.Messages.ImageSelectionOptions;
import io.flutter.plugins.imagepicker.Messages.SourceSpecification;
import io.flutter.plugins.imagepicker.Messages.VideoSelectionOptions;
import java.io.File;
import java.util.List;
import org.junit.After;
import org.junit.Before;
Expand Down Expand Up @@ -202,20 +198,6 @@ public void onConstructor_whenContextTypeIsActivity_shouldNotCrash() {
"No exception thrown when ImagePickerPlugin() ran with context instanceof Activity", true);
}

@Test
public void constructDelegate_shouldUseInternalCacheDirectory() {
File mockDirectory = new File("/mockpath");
when(mockActivity.getCacheDir()).thenReturn(mockDirectory);

ImagePickerDelegate delegate = plugin.constructDelegate(mockActivity);

verify(mockActivity, times(1)).getCacheDir();
assertThat(
"Delegate uses cache directory for storing camera captures",
delegate.externalFilesDirectory,
equalTo(mockDirectory));
}

@Test
public void onDetachedFromActivity_shouldReleaseActivityState() {
final BinaryMessenger mockBinaryMessenger = mock(BinaryMessenger.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,12 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.when;

import android.content.Context;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import java.io.File;
Expand All @@ -33,8 +36,8 @@
// But we can still test whether the original or scaled file is created.
@RunWith(RobolectricTestRunner.class)
public class ImageResizerTest {

ImageResizer resizer;
Context mockContext;
File imageFile;
File svgImageFile;
File externalDirectory;
Expand All @@ -51,7 +54,9 @@ public void setUp() throws IOException {
TemporaryFolder temporaryFolder = new TemporaryFolder();
temporaryFolder.create();
externalDirectory = temporaryFolder.newFolder("image_picker_testing_path");
resizer = new ImageResizer(externalDirectory, new ExifDataCopier());
mockContext = mock(Context.class);
when(mockContext.getCacheDir()).thenReturn(externalDirectory);
resizer = new ImageResizer(mockContext, new ExifDataCopier());
}

@After
Expand Down Expand Up @@ -83,14 +88,6 @@ public void onResizeImageIfNeeded_whenHeightIsNotNull_shouldResize_returnResized
assertThat(outputFile, equalTo(externalDirectory.getPath() + "/scaled_pngImage.png"));
}

@Test
public void onResizeImageIfNeeded_whenParentDirectoryDoesNotExists_shouldNotCrash() {
File nonExistentDirectory = new File(externalDirectory, "/nonExistent");
ImageResizer invalidResizer = new ImageResizer(nonExistentDirectory, new ExifDataCopier());
String outputFile = invalidResizer.resizeImageIfNeeded(imageFile.getPath(), null, 50.0, 100);
assertThat(outputFile, equalTo(nonExistentDirectory.getPath() + "/scaled_pngImage.png"));
}

@Test
public void onResizeImageIfNeeded_whenImagePathIsNotBitmap_shouldReturnPathAndNotNull() {
String nonBitmapImagePath = svgImageFile.getPath();
Expand Down
Loading