Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[image_picker_android] Name picked files to match the original filenames where possible #6096

Merged
Show file tree
Hide file tree
Changes from 5 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
6 changes: 5 additions & 1 deletion packages/image_picker/image_picker_android/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
## NEXT
## 0.8.5+4

* Fixes names of picked files to match original filenames where possible.

## 0.8.5+3

* Updates minimum Flutter version to 2.10.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,55 +25,46 @@

import android.content.ContentResolver;
import android.content.Context;
import android.database.Cursor;
import android.net.Uri;
import android.provider.MediaStore;
import android.webkit.MimeTypeMap;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer explicitly importing only the exact classes we use instead of importing everything under java.io

import java.util.UUID;

class FileUtils {

@SuppressWarnings("IOStreamConstructor")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a valid warning? I can't seem to find it anywhere.

String getPathFromUri(final Context context, final Uri uri) {
File file = null;
InputStream inputStream = null;
OutputStream outputStream = null;
boolean success = false;
try {
String extension = getImageExtension(context, uri);
inputStream = context.getContentResolver().openInputStream(uri);
file = File.createTempFile("image_picker", extension, context.getCacheDir());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing where the uuid is being added in the original code. This seems to produce a single file named image_picker.<ext> in the cache dir rather than image_picker<uuid>.<ext>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The createTempFile method used in the original code creates a file with a given prefix and suffix, but randomizes the part between them (but it's not explicitly said to be an UUID).

file.deleteOnExit();
outputStream = new FileOutputStream(file);
if (inputStream != null) {
copy(inputStream, outputStream);
success = true;
}
} catch (IOException ignored) {
} finally {
try {
if (inputStream != null) inputStream.close();
} catch (IOException ignored) {
try (InputStream inputStream = context.getContentResolver().openInputStream(uri)) {
String uuid = UUID.randomUUID().toString();
File targetDirectory = new File(context.getCacheDir(), uuid);
targetDirectory.mkdir();
// TODO(SynSzakala) according to the docs, `deleteOnExit` does not work reliably on Android; we should preferably
// just clear the picked files after the app startup.
targetDirectory.deleteOnExit();
String fileName = getImageName(context, uri);
if (fileName == null) {
fileName = uuid + getImageExtension(context, uri);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since targetDirectory is already a directory named uuid, repeating the uuid here in the file name as well seems redundant and less human readable than optimal.

It may make sense to be continuous and use the old naming scheme as fallback image_picker<extension>

}
try {
if (outputStream != null) outputStream.close();
} catch (IOException ignored) {
// If closing the output stream fails, we cannot be sure that the
// target file was written in full. Flushing the stream merely moves
// the bytes into the OS, not necessarily to the file.
success = false;
File file = new File(targetDirectory, fileName);
try (OutputStream outputStream = new FileOutputStream(file)) {
copy(inputStream, outputStream);
return file.getPath();
}
} catch (IOException e) {
// If closing the output stream fails, we cannot be sure that the
// target file was written in full. Flushing the stream merely moves
// the bytes into the OS, not necessarily to the file.
return null;
}
return success ? file.getPath() : null;
}

/** @return extension of image with dot, or default .jpg if it none. */
private static String getImageExtension(Context context, Uri uriImage) {
String extension = null;
String extension;

try {
String imagePath = uriImage.getPath();
if (uriImage.getScheme().equals(ContentResolver.SCHEME_CONTENT)) {
final MimeTypeMap mime = MimeTypeMap.getSingleton();
extension = mime.getExtensionFromMimeType(context.getContentResolver().getType(uriImage));
Expand All @@ -94,6 +85,21 @@ private static String getImageExtension(Context context, Uri uriImage) {
return "." + extension;
}

/** @return name of the image provided by ContentResolver; this may be null. */
private static String getImageName(Context context, Uri uriImage) {
try (Cursor cursor = queryImageName(context, uriImage)) {
if (cursor == null) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

validate cursor columns count to provide clearer error if encountered

cursor.moveToFirst();
return cursor.getString(0);
}
}

private static Cursor queryImageName(Context context, Uri uriImage) {
return context
.getContentResolver()
.query(uriImage, new String[] {MediaStore.MediaColumns.DISPLAY_NAME}, null, null, null);
}

private static void copy(InputStream in, OutputStream out) throws IOException {
final byte[] buffer = new byte[4 * 1024];
int bytesRead;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,21 @@
import static org.junit.Assert.assertTrue;
import static org.robolectric.Shadows.shadowOf;

import android.content.ContentProvider;
import android.content.ContentValues;
import android.content.Context;
import android.database.Cursor;
import android.database.MatrixCursor;
import android.net.Uri;
import android.provider.MediaStore;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.test.core.app.ApplicationProvider;
import java.io.BufferedInputStream;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, prefer explicit imports rather than wildcard

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.Robolectric;
import org.robolectric.RobolectricTestRunner;
import org.robolectric.shadows.ShadowContentResolver;

Expand Down Expand Up @@ -63,4 +67,62 @@ public void FileUtil_getImageExtension() throws IOException {
String path = fileUtils.getPathFromUri(context, uri);
assertTrue(path.endsWith(".jpg"));
}

@Test
public void FileUtil_getImageName() throws IOException {
Uri uri = Uri.parse("content://dummy/dummy.png");
Robolectric.buildContentProvider(MockContentProvider.class).create("dummy");
shadowContentResolver.registerInputStream(
uri, new ByteArrayInputStream("imageStream".getBytes(UTF_8)));
String path = fileUtils.getPathFromUri(context, uri);
assertTrue(path.endsWith("dummy.png"));
}

private static class MockContentProvider extends ContentProvider {

@Override
public boolean onCreate() {
return true;
}

@Nullable
@Override
public Cursor query(
@NonNull Uri uri,
@Nullable String[] projection,
@Nullable String selection,
@Nullable String[] selectionArgs,
@Nullable String sortOrder) {
MatrixCursor cursor = new MatrixCursor(new String[] {MediaStore.MediaColumns.DISPLAY_NAME});
cursor.addRow(new Object[] {"dummy.png"});
return cursor;
}

@Nullable
@Override
public String getType(@NonNull Uri uri) {
return "image/png";
}

@Nullable
@Override
public Uri insert(@NonNull Uri uri, @Nullable ContentValues values) {
return null;
}

@Override
public int delete(
@NonNull Uri uri, @Nullable String selection, @Nullable String[] selectionArgs) {
return 0;
}

@Override
public int update(
@NonNull Uri uri,
@Nullable ContentValues values,
@Nullable String selection,
@Nullable String[] selectionArgs) {
return 0;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,5 +63,7 @@ dependencies {
testImplementation 'junit:junit:4.13.2'
androidTestImplementation 'androidx.test:runner:1.2.0'
androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0'
implementation project(':image_picker_android')
implementation project(':espresso')
api 'androidx.test:core:1.4.0'
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package io.flutter.plugins.imagepickerexample;

import static androidx.test.espresso.flutter.EspressoFlutter.onFlutterWidget;
import static androidx.test.espresso.flutter.action.FlutterActions.click;
import static androidx.test.espresso.flutter.assertion.FlutterAssertions.matches;
import static androidx.test.espresso.flutter.matcher.FlutterMatchers.withText;
import static androidx.test.espresso.flutter.matcher.FlutterMatchers.withValueKey;
import static androidx.test.espresso.intent.Intents.intended;
import static androidx.test.espresso.intent.Intents.intending;
import static androidx.test.espresso.intent.matcher.IntentMatchers.hasAction;

import android.app.Activity;
import android.app.Instrumentation;
import android.content.Intent;
import android.net.Uri;
import androidx.test.espresso.intent.rule.IntentsTestRule;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestRule;

public class ImagePickerPickTest {

@Rule public TestRule rule = new IntentsTestRule<>(DriverExtensionActivity.class);

@Test
@Ignore("Doesn't run in Firebase Test Lab: https://github.com/flutter/flutter/issues/94748")
public void imageIsPickedWithOriginalName() {
Instrumentation.ActivityResult result =
new Instrumentation.ActivityResult(
Activity.RESULT_OK, new Intent().setData(Uri.parse("content://dummy/dummy.png")));
intending(hasAction(Intent.ACTION_GET_CONTENT)).respondWith(result);
onFlutterWidget(withValueKey("image_picker_example_from_gallery")).perform(click());
onFlutterWidget(withText("PICK")).perform(click());
intended(hasAction(Intent.ACTION_GET_CONTENT));
onFlutterWidget(withValueKey("image_picker_example_picked_image_name"))
.check(matches(withText("dummy.png")));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,17 @@
android:hardwareAccelerated="true"
android:windowSoftInputMode="adjustResize">
</activity>
<activity
android:name=".DriverExtensionActivity"
android:launchMode="singleTop"
android:theme="@android:style/Theme.Black.NoTitleBar"
android:configChanges="orientation|keyboardHidden|keyboard|screenSize|smallestScreenSize|locale|layoutDirection|fontScale|screenLayout|density|uiMode"
android:hardwareAccelerated="true"
android:windowSoftInputMode="adjustResize">
</activity>
<provider
android:authorities="dummy"
android:name=".DummyContentProvider"
android:exported="true"/>
</application>
</manifest>
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package io.flutter.plugins.imagepickerexample;

import androidx.annotation.NonNull;
import io.flutter.embedding.android.FlutterActivity;
import org.jetbrains.annotations.NotNull;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to use the jetbrains IDE specific annotation

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason you are using both this and @nonnull?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, just by an accident.


public class DriverExtensionActivity extends FlutterActivity {
@NonNull
@NotNull
@Override
public String getDartEntrypointFunctionName() {
return "appMain";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package io.flutter.plugins.imagepickerexample;

import android.content.ContentProvider;
import android.content.ContentValues;
import android.content.res.AssetFileDescriptor;
import android.database.Cursor;
import android.database.MatrixCursor;
import android.net.Uri;
import android.provider.MediaStore;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;

public class DummyContentProvider extends ContentProvider {
@Override
public boolean onCreate() {
return true;
}

@Nullable
@Override
public AssetFileDescriptor openAssetFile(@NonNull Uri uri, @NonNull String mode) {
return getContext().getResources().openRawResourceFd(R.raw.flutter_logo);
}

@Nullable
@Override
public Cursor query(
@NonNull Uri uri,
@Nullable String[] projection,
@Nullable String selection,
@Nullable String[] selectionArgs,
@Nullable String sortOrder) {
MatrixCursor cursor = new MatrixCursor(new String[] {MediaStore.MediaColumns.DISPLAY_NAME});
cursor.addRow(new Object[] {"dummy.png"});
return cursor;
}

@Nullable
@Override
public String getType(@NonNull Uri uri) {
return "image/png";
}

@Nullable
@Override
public Uri insert(@NonNull Uri uri, @Nullable ContentValues values) {
return null;
}

@Override
public int delete(
@NonNull Uri uri, @Nullable String selection, @Nullable String[] selectionArgs) {
return 0;
}

@Override
public int update(
@NonNull Uri uri,
@Nullable ContentValues values,
@Nullable String selection,
@Nullable String[] selectionArgs) {
return 0;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment refers to the flutter_logo.png file

We generally don't include image files that are non-trivially large in the repo if possible. We prefer to either get them in another repo and import them or if the goal is to have an image to test with, generate a trivial image. @stuartmorgan Do we have this rule in the plugins repo? What do you think about this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have a specific rule/enforcement around this for flutter/plugins, since checking out the entire repo isn't part of the normal usage of plugins (unlike for flutter/flutter). But using a copy of one of the images that's already in the repo, such as an app icon image from one of the example templates, would be preferable to a new image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot easily use an existing file, because it needs to be placed in the 'res/raw' folder. But I replaced this logo file with a copy of the smallest icon, just ~500 bytes so it shouldn't be a problem.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading