From d29aa2e2e527f63e8f33935151fc81b15fe22709 Mon Sep 17 00:00:00 2001 From: neslihanturan Date: Wed, 1 Aug 2018 23:24:08 +0300 Subject: [PATCH] Fix urgent crashes A and E (#1749) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Create utility class for contribution process * implement method to save five from given URİ * Add file utilities for directory checks * Add ContributionUtils for saving file during upload * Change method call acordingly with handleImagePicked() method * Call method to save file temproarily when a photo to upload is chosen from contributions list. * Call method to save file temproarily when a photo to upload is chosen from nearby list and map * Arrange method call * Write a method to save file temporarily during upload process. It will save the file to a internal path and it will be deleted by another method after upload process is done. * Add a method to save a file to a given path from a content provider Uri * On openAssetFileDescriptor method, use URi from temporarily saved file, instead of Contributions.getLocalUri which was Uri from content provider * Edit uploadContribution method so that it will use FileInputStream from temporarily saved file, insdeat of the Uri from content provider. * Make it work * Code cleanup * Add directory cleaner method * Call temp directory cleaner method at the end of uplpoad process * Use FileInputStream insted * Add directory cleaner method * Add file removal method * Use external directory instead * Make destination file name flexible * Make it work with share action coming from another activity * Make it work for Multiple hare Activity * Code cleanup * Solve camera issue * Fix camera crash * Cleanup * Revert change of commenting out posibly useles code, because I am not sure if it is useless or not. Requires discussion * Use timestamp in temoorary file names, so that we wont never create same file and access old file reference. It was a weird problem though * Code cleanup * Add nullable annotation to handleImagePicked method uri parameter * Add Nullable anotation to method * Code cleanup * Bugfix: use uri.getPath() instead uri.toString * Remove unecesarry file saving operation, which was added accidentally * Fix travis fail * Remove temp file if upload gets failed and file is still there * Code cleanup:Remove unused parameters from removeTempFile method * Empty temp directory on app create, in case some of files are still there * Add null check to array to prevent NPE on first run * Fix multiple uploads bug * Remove file if upload is succeed * Add external storage utility methods * Check external file permission before saving files temporarily * finish activity if permission is not granted * Add log lines * Remove files even if user decides to go back without sharing * Add easy null check * Change storage permission settings in singe upload fragment too * Finish app if permission is not granted * Code optimisation * Remove temp file if upload process never is finalised on activity stop * Bugfix maybe contribution is never created * Fix travis build --- .../free/nrw/commons/CommonsApplication.java | 4 +- .../free/nrw/commons/MediaWikiImageView.java | 14 +-- .../commons/contributions/Contribution.java | 9 ++ .../contributions/ContributionController.java | 11 ++- .../contributions/ContributionsActivity.java | 4 + .../ContributionsListFragment.java | 11 ++- .../mwapi/ApacheHttpClientMediaWikiApi.java | 11 ++- .../free/nrw/commons/mwapi/MediaWikiApi.java | 3 +- .../commons/nearby/NearbyListFragment.java | 9 +- .../nrw/commons/nearby/NearbyMapFragment.java | 10 +- .../fr/free/nrw/commons/upload/FileUtils.java | 1 + .../commons/upload/MultipleShareActivity.java | 82 ++++++++++++---- .../nrw/commons/upload/ShareActivity.java | 78 +++++++++++---- .../nrw/commons/upload/UploadController.java | 13 ++- .../nrw/commons/upload/UploadService.java | 15 ++- .../nrw/commons/utils/ContributionUtils.java | 95 +++++++++++++++++++ .../commons/utils/ExternalStorageUtils.java | 47 +++++++++ .../fr/free/nrw/commons/utils/FileUtils.java | 89 +++++++++++++++++ 18 files changed, 440 insertions(+), 66 deletions(-) create mode 100644 app/src/main/java/fr/free/nrw/commons/utils/ContributionUtils.java create mode 100644 app/src/main/java/fr/free/nrw/commons/utils/ExternalStorageUtils.java create mode 100644 app/src/main/java/fr/free/nrw/commons/utils/FileUtils.java diff --git a/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java b/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java index 61eecee00..cb47f75e9 100644 --- a/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java +++ b/app/src/main/java/fr/free/nrw/commons/CommonsApplication.java @@ -27,6 +27,7 @@ import fr.free.nrw.commons.data.DBOpenHelper; import fr.free.nrw.commons.di.ApplicationlessInjection; import fr.free.nrw.commons.modifications.ModifierSequenceDao; import fr.free.nrw.commons.upload.FileUtils; +import fr.free.nrw.commons.utils.ContributionUtils; import io.reactivex.android.schedulers.AndroidSchedulers; import io.reactivex.schedulers.Schedulers; import timber.log.Timber; @@ -68,7 +69,6 @@ public class CommonsApplication extends MultiDexApplication { @Override public void onCreate() { super.onCreate(); - ApplicationlessInjection .getInstance(this) .getCommonsApplicationComponent() @@ -81,6 +81,8 @@ public class CommonsApplication extends MultiDexApplication { if (setupLeakCanary() == RefWatcher.DISABLED) { return; } + // Empty temp directory in case some temp files are created and never removed. + ContributionUtils.emptyTemporaryDirectory(); Timber.plant(new Timber.DebugTree()); diff --git a/app/src/main/java/fr/free/nrw/commons/MediaWikiImageView.java b/app/src/main/java/fr/free/nrw/commons/MediaWikiImageView.java index 5e89e19a5..52952036a 100644 --- a/app/src/main/java/fr/free/nrw/commons/MediaWikiImageView.java +++ b/app/src/main/java/fr/free/nrw/commons/MediaWikiImageView.java @@ -51,16 +51,10 @@ public class MediaWikiImageView extends SimpleDraweeView { return; } - if(media.getFilename() != null) { - if (thumbnailUrlCache.get(media.getFilename()) != null) { - setImageUrl(thumbnailUrlCache.get(media.getFilename())); - } else { - setImageUrl(null); - currentThumbnailTask = new ThumbnailFetchTask(media, mwApi); - currentThumbnailTask.execute(media.getFilename()); - } - } else { // local image - setImageUrl(media.getLocalUri().toString()); + if (media.getFilename() != null && thumbnailUrlCache.get(media.getFilename()) != null) { + setImageUrl(thumbnailUrlCache.get(media.getFilename())); + } else { + setImageUrl(null); currentThumbnailTask = new ThumbnailFetchTask(media, mwApi); currentThumbnailTask.execute(media.getFilename()); } diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/Contribution.java b/app/src/main/java/fr/free/nrw/commons/contributions/Contribution.java index 99009c029..efaf5f221 100644 --- a/app/src/main/java/fr/free/nrw/commons/contributions/Contribution.java +++ b/app/src/main/java/fr/free/nrw/commons/contributions/Contribution.java @@ -46,6 +46,7 @@ public class Contribution extends Media { private String decimalCoords; private boolean isMultiple; private String wikiDataEntityId; + private Uri contentProviderUri; public Contribution(Uri contentUri, String filename, Uri localUri, String imageUrl, Date timestamp, int state, long dataLength, Date dateUploaded, long transferred, @@ -236,4 +237,12 @@ public class Contribution extends Media { public void setWikiDataEntityId(String wikiDataEntityId) { this.wikiDataEntityId = wikiDataEntityId; } + + public void setContentProviderUri(Uri contentProviderUri) { + this.contentProviderUri = contentProviderUri; + } + + public Uri getContentProviderUri() { + return contentProviderUri; + } } diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionController.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionController.java index 76f3e5a0f..f57c8e239 100644 --- a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionController.java +++ b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionController.java @@ -7,9 +7,11 @@ import android.content.pm.ResolveInfo; import android.net.Uri; import android.os.Bundle; import android.provider.MediaStore; +import android.support.annotation.Nullable; import android.support.v4.app.Fragment; import android.support.v4.app.FragmentActivity; import android.support.v4.content.FileProvider; +import android.util.Log; import java.io.File; import java.util.Date; @@ -28,8 +30,8 @@ import static fr.free.nrw.commons.wikidata.WikidataConstants.WIKIDATA_ENTITY_ID_ public class ContributionController { - private static final int SELECT_FROM_GALLERY = 1; - private static final int SELECT_FROM_CAMERA = 2; + public static final int SELECT_FROM_GALLERY = 1; + public static final int SELECT_FROM_CAMERA = 2; private Fragment fragment; @@ -91,8 +93,7 @@ public class ContributionController { fragment.startActivityForResult(pickImageIntent, SELECT_FROM_GALLERY); } - public void handleImagePicked(int requestCode, Intent data, boolean isDirectUpload, String wikiDataEntityId) { - Timber.d("Is direct upload %s and the Wikidata entity ID is %s", isDirectUpload, wikiDataEntityId); + public void handleImagePicked(int requestCode, @Nullable Uri uri, boolean isDirectUpload, String wikiDataEntityId) { FragmentActivity activity = fragment.getActivity(); Timber.d("handleImagePicked() called with onActivityResult()"); Intent shareIntent = new Intent(activity, ShareActivity.class); @@ -100,7 +101,7 @@ public class ContributionController { switch (requestCode) { case SELECT_FROM_GALLERY: //Handles image picked from gallery - Uri imageData = data.getData(); + Uri imageData = uri; shareIntent.setType(activity.getContentResolver().getType(imageData)); shareIntent.putExtra(EXTRA_STREAM, imageData); shareIntent.putExtra(EXTRA_SOURCE, SOURCE_GALLERY); diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsActivity.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsActivity.java index e4f884128..ba5894270 100644 --- a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsActivity.java @@ -38,6 +38,7 @@ import fr.free.nrw.commons.mwapi.MediaWikiApi; import fr.free.nrw.commons.quiz.QuizChecker; import fr.free.nrw.commons.settings.Prefs; import fr.free.nrw.commons.upload.UploadService; +import fr.free.nrw.commons.utils.ContributionUtils; import fr.free.nrw.commons.utils.ViewUtil; import io.reactivex.android.schedulers.AndroidSchedulers; import io.reactivex.disposables.CompositeDisposable; @@ -199,6 +200,9 @@ public class ContributionsActivity Contribution c = contributionDao.fromCursor(allContributions); if (c.getState() == STATE_FAILED) { Timber.d("Deleting failed contrib %s", c.toString()); + // If upload fails and then user decides to cancel upload at all, which means contribution + // object will be deleted. So we have to delete temp file for that contribution. + ContributionUtils.removeTemporaryFile(c.getLocalUri()); contributionDao.delete(c); } else { Timber.d("Skipping deletion for non-failed contrib %s", c.toString()); diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java index 0b600c5d0..6abb3ce43 100644 --- a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java @@ -3,11 +3,13 @@ package fr.free.nrw.commons.contributions; import android.content.Intent; import android.content.SharedPreferences; import android.content.pm.PackageManager; +import android.net.Uri; import android.os.Build; import android.os.Bundle; import android.support.annotation.NonNull; import android.support.v4.content.ContextCompat; import android.support.v7.app.AlertDialog; +import android.util.Log; import android.view.LayoutInflater; import android.view.Menu; import android.view.MenuInflater; @@ -31,6 +33,7 @@ import fr.free.nrw.commons.BuildConfig; import fr.free.nrw.commons.R; import fr.free.nrw.commons.di.CommonsDaggerSupportFragment; import fr.free.nrw.commons.nearby.NearbyActivity; +import fr.free.nrw.commons.utils.ContributionUtils; import timber.log.Timber; import static android.Manifest.permission.READ_EXTERNAL_STORAGE; @@ -117,7 +120,13 @@ public class ContributionsListFragment extends CommonsDaggerSupportFragment { if (resultCode == RESULT_OK) { Timber.d("OnActivityResult() parameters: Req code: %d Result code: %d Data: %s", requestCode, resultCode, data); - controller.handleImagePicked(requestCode, data, false, null); + if (requestCode == ContributionController.SELECT_FROM_CAMERA) { + // If coming from camera, pass null as uri. Because camera photos get saved to a + // fixed directory + controller.handleImagePicked(requestCode, null, false, null); + } else { + controller.handleImagePicked(requestCode, data.getData(), false, null); + } } else { Timber.e("OnActivityResult() parameters: Req code: %d Result code: %d Data: %s", requestCode, resultCode, data); diff --git a/app/src/main/java/fr/free/nrw/commons/mwapi/ApacheHttpClientMediaWikiApi.java b/app/src/main/java/fr/free/nrw/commons/mwapi/ApacheHttpClientMediaWikiApi.java index 1e347c5c9..886024170 100644 --- a/app/src/main/java/fr/free/nrw/commons/mwapi/ApacheHttpClientMediaWikiApi.java +++ b/app/src/main/java/fr/free/nrw/commons/mwapi/ApacheHttpClientMediaWikiApi.java @@ -2,6 +2,7 @@ package fr.free.nrw.commons.mwapi; import android.content.Context; import android.content.SharedPreferences; +import android.net.Uri; import android.os.Build; import android.support.annotation.NonNull; import android.support.annotation.Nullable; @@ -51,6 +52,7 @@ import fr.free.nrw.commons.category.CategoryImageUtils; import fr.free.nrw.commons.category.QueryContinue; import fr.free.nrw.commons.notification.Notification; import fr.free.nrw.commons.notification.NotificationUtils; +import fr.free.nrw.commons.utils.ContributionUtils; import in.yuvi.http.fluent.Http; import io.reactivex.Observable; import io.reactivex.Single; @@ -856,17 +858,23 @@ public class ApacheHttpClientMediaWikiApi implements MediaWikiApi { long dataLength, String pageContents, String editSummary, - final ProgressListener progressListener) throws IOException { + final ProgressListener progressListener, + Uri fileUri, + Uri contentProviderUri) throws IOException { + ApiResult result = api.upload(filename, file, dataLength, pageContents, editSummary, progressListener::onProgress); Log.e("WTF", "Result: " + result.toString()); String resultStatus = result.getString("/api/upload/@result"); + if (!resultStatus.equals("Success")) { String errorCode = result.getString("/api/error/@code"); Timber.e(errorCode); return new UploadResult(resultStatus, errorCode); } else { + // If success we have to remove file from temp directory + ContributionUtils.removeTemporaryFile(fileUri); Date dateUploaded = parseMWDate(result.getString("/api/upload/imageinfo/@timestamp")); String canonicalFilename = "File:" + result.getString("/api/upload/@filename").replace("_", " "); // Title vs Filename String imageUrl = result.getString("/api/upload/imageinfo/@url"); @@ -874,7 +882,6 @@ public class ApacheHttpClientMediaWikiApi implements MediaWikiApi { } } - @Override @NonNull public Single getUploadCount(String userName) { diff --git a/app/src/main/java/fr/free/nrw/commons/mwapi/MediaWikiApi.java b/app/src/main/java/fr/free/nrw/commons/mwapi/MediaWikiApi.java index 8c410dad7..a40f04bff 100644 --- a/app/src/main/java/fr/free/nrw/commons/mwapi/MediaWikiApi.java +++ b/app/src/main/java/fr/free/nrw/commons/mwapi/MediaWikiApi.java @@ -1,5 +1,6 @@ package fr.free.nrw.commons.mwapi; +import android.net.Uri; import android.support.annotation.NonNull; import android.support.annotation.Nullable; @@ -54,7 +55,7 @@ public interface MediaWikiApi { List searchCategory(String title, int offset); @NonNull - UploadResult uploadFile(String filename, InputStream file, long dataLength, String pageContents, String editSummary, ProgressListener progressListener) throws IOException; + UploadResult uploadFile(String filename, InputStream file, long dataLength, String pageContents, String editSummary, ProgressListener progressListener, Uri fileUri, Uri contentProviderUri) throws IOException; @Nullable String edit(String editToken, String processedPageContent, String filename, String summary) throws IOException; diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyListFragment.java b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyListFragment.java index 0599b4fe2..81768d389 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyListFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyListFragment.java @@ -30,6 +30,7 @@ import dagger.android.support.DaggerFragment; import fr.free.nrw.commons.R; import fr.free.nrw.commons.contributions.ContributionController; import fr.free.nrw.commons.location.LatLng; +import fr.free.nrw.commons.utils.ContributionUtils; import fr.free.nrw.commons.utils.UriDeserializer; import timber.log.Timber; @@ -147,7 +148,13 @@ public class NearbyListFragment extends DaggerFragment { if (resultCode == RESULT_OK) { Timber.d("OnActivityResult() parameters: Req code: %d Result code: %d Data: %s", requestCode, resultCode, data); - controller.handleImagePicked(requestCode, data, true, directPrefs.getString(WIKIDATA_ENTITY_ID_PREF, null)); + if (requestCode == ContributionController.SELECT_FROM_CAMERA) { + // If coming from camera, pass null as uri. Because camera photos get saved to a + // fixed directory + controller.handleImagePicked(requestCode, null, true, null); + } else { + controller.handleImagePicked(requestCode, data.getData(), true, null); + } } else { Timber.e("OnActivityResult() parameters: Req code: %d Result code: %d Data: %s", requestCode, resultCode, data); diff --git a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyMapFragment.java b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyMapFragment.java index 44807c195..bb2d9c671 100644 --- a/app/src/main/java/fr/free/nrw/commons/nearby/NearbyMapFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/nearby/NearbyMapFragment.java @@ -56,6 +56,7 @@ import dagger.android.support.DaggerFragment; import fr.free.nrw.commons.R; import fr.free.nrw.commons.Utils; import fr.free.nrw.commons.contributions.ContributionController; +import fr.free.nrw.commons.utils.ContributionUtils; import fr.free.nrw.commons.utils.UriDeserializer; import fr.free.nrw.commons.utils.ViewUtil; import timber.log.Timber; @@ -765,10 +766,17 @@ public class NearbyMapFragment extends DaggerFragment { public void onActivityResult(int requestCode, int resultCode, Intent data) { super.onActivityResult(requestCode, resultCode, data); + if (resultCode == RESULT_OK) { Timber.d("OnActivityResult() parameters: Req code: %d Result code: %d Data: %s", requestCode, resultCode, data); - controller.handleImagePicked(requestCode, data, true, directPrefs.getString(WIKIDATA_ENTITY_ID_PREF, null)); + if (requestCode == ContributionController.SELECT_FROM_CAMERA) { + // If coming from camera, pass null as uri. Because camera photos get saved to a + // fixed directory + controller.handleImagePicked(requestCode, null, true, null); + } else { + controller.handleImagePicked(requestCode, data.getData(), true, null); + } } else { Timber.e("OnActivityResult() parameters: Req code: %d Result code: %d Data: %s", requestCode, resultCode, data); diff --git a/app/src/main/java/fr/free/nrw/commons/upload/FileUtils.java b/app/src/main/java/fr/free/nrw/commons/upload/FileUtils.java index 0cd45c189..8c9e56314 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/FileUtils.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/FileUtils.java @@ -14,6 +14,7 @@ import android.provider.DocumentsContract; import android.provider.MediaStore; import android.support.annotation.NonNull; import android.support.annotation.Nullable; +import android.util.Log; import java.io.BufferedReader; import java.io.File; diff --git a/app/src/main/java/fr/free/nrw/commons/upload/MultipleShareActivity.java b/app/src/main/java/fr/free/nrw/commons/upload/MultipleShareActivity.java index 60986bda9..e429c3ee8 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/MultipleShareActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/MultipleShareActivity.java @@ -46,6 +46,8 @@ import fr.free.nrw.commons.modifications.ModifierSequence; import fr.free.nrw.commons.modifications.ModifierSequenceDao; import fr.free.nrw.commons.modifications.TemplateRemoveModifier; import fr.free.nrw.commons.mwapi.MediaWikiApi; +import fr.free.nrw.commons.utils.ContributionUtils; +import fr.free.nrw.commons.utils.ExternalStorageUtils; import timber.log.Timber; //TODO: We should use this class to see how multiple uploads are handled, and then REMOVE it. @@ -55,7 +57,8 @@ public class MultipleShareActivity extends AuthenticatedActivity AdapterView.OnItemClickListener, FragmentManager.OnBackStackChangedListener, MultipleUploadListFragment.OnMultipleUploadInitiatedHandler, - OnCategoriesSaveHandler { + OnCategoriesSaveHandler, + ActivityCompat.OnRequestPermissionsResultCallback{ @Inject MediaWikiApi mwApi; @@ -76,6 +79,8 @@ public class MultipleShareActivity extends AuthenticatedActivity private CategorizationFragment categorizationFragment; private boolean locationPermitted = false; + private boolean isMultipleUploadsPrepared = false; + private boolean isMultipleUploadsFinalised = false; // Checks is user clicked to upload button or regret before this phase @Override public Media getMediaAtPosition(int i) { @@ -114,30 +119,25 @@ public class MultipleShareActivity extends AuthenticatedActivity @Override public void OnMultipleUploadInitiated() { - - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { - //Check for Storage permission that is required for upload. Do not allow user to proceed without permission, otherwise will crash - if (ContextCompat.checkSelfPermission(this, Manifest.permission.READ_EXTERNAL_STORAGE) != PackageManager.PERMISSION_GRANTED) { - ActivityCompat.requestPermissions(this, new String[]{Manifest.permission.READ_EXTERNAL_STORAGE}, 1); - } else { - multipleUploadBegins(); - } - } else { - multipleUploadBegins(); - } + // No need to request external permission here, because if user can reach this point, then she permission granted + Timber.d("OnMultipleUploadInitiated"); + multipleUploadBegins(); } @Override public void onRequestPermissionsResult(int requestCode, @NonNull String[] permissions, @NonNull int[] grantResults) { if (requestCode == 1 && grantResults.length > 0 && grantResults[0] == PackageManager.PERMISSION_GRANTED) { - multipleUploadBegins(); + Timber.d("onRequestPermissionsResult external storage permission granted"); + prepareMultipleUpoadList(); + } else { + // Permission is not granted, close activity + finish(); } } private void multipleUploadBegins() { Timber.d("Multiple upload begins"); - final ProgressDialog dialog = new ProgressDialog(this); dialog.setIndeterminate(false); dialog.setProgressStyle(ProgressDialog.STYLE_HORIZONTAL); @@ -175,6 +175,7 @@ public class MultipleShareActivity extends AuthenticatedActivity getSupportFragmentManager().beginTransaction() .add(R.id.uploadsFragmentContainer, categorizationFragment, "categorization") .commitAllowingStateLoss(); + isMultipleUploadsFinalised = true; //See http://stackoverflow.com/questions/7469082/getting-exception-illegalstateexception-can-not-perform-this-action-after-onsa } @@ -254,13 +255,40 @@ public class MultipleShareActivity extends AuthenticatedActivity @Override protected void onSaveInstanceState(Bundle outState) { - super.onSaveInstanceState(outState); - outState.putParcelableArrayList("uploadsList", photosList); + /* This will be true if permission request is granted before we request. Otherwise we will + * explicitly call operations under this method again. + */ + if (isMultipleUploadsPrepared) { + super.onSaveInstanceState(outState); + Timber.d("onSaveInstanceState multiple uploads is prepared, permission granted"); + outState.putParcelableArrayList("uploadsList", photosList); + } else { + Timber.d("onSaveInstanceState multiple uploads is not prepared, permission not granted"); + return; + } } @Override protected void onAuthCookieAcquired(String authCookie) { + // Multiple uploads prepared boolean is used to decide when to call multipleUploadsBegin() + isMultipleUploadsFinalised = false; + isMultipleUploadsPrepared = false; mwApi.setAuthCookie(authCookie); + if (!ExternalStorageUtils.isStoragePermissionGranted(this)) { + ExternalStorageUtils.requestExternalStoragePermission(this); + isMultipleUploadsPrepared = false; + return; // Postpone operation to do after gettion permission + } else { + isMultipleUploadsPrepared = true; + prepareMultipleUpoadList(); + } + } + + /** + * Prepares a list from files will be uploaded. Saves these files temporarily to external + * storage. Adds them to uploads list + */ + private void prepareMultipleUpoadList() { Intent intent = getIntent(); if (Intent.ACTION_SEND_MULTIPLE.equals(intent.getAction())) { @@ -270,6 +298,8 @@ public class MultipleShareActivity extends AuthenticatedActivity for (int i = 0; i < urisList.size(); i++) { Contribution up = new Contribution(); Uri uri = urisList.get(i); + // Use temporarily saved file Uri instead + uri = ContributionUtils.saveFileBeingUploadedTemporarily(this, uri); up.setLocalUri(uri); up.setTag("mimeType", intent.getType()); up.setTag("sequence", i); @@ -351,4 +381,24 @@ public class MultipleShareActivity extends AuthenticatedActivity return null; } + + // If on back pressed before sharing + @Override + public void onBackPressed() { + super.onBackPressed(); + } + + @Override + protected void onStop() { + // Remove saved files if activity is stopped before upload operation, ie user changed mind + if (!isMultipleUploadsFinalised) { + if (photosList != null) { + for (Contribution contribution : photosList) { + Timber.d("User changed mind, didn't click to upload button, deleted file: "+contribution.getLocalUri()); + ContributionUtils.removeTemporaryFile(contribution.getLocalUri()); + } + } + } + super.onStop(); + } } \ No newline at end of file diff --git a/app/src/main/java/fr/free/nrw/commons/upload/ShareActivity.java b/app/src/main/java/fr/free/nrw/commons/upload/ShareActivity.java index d3dbe9ace..8f81e93af 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/ShareActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/ShareActivity.java @@ -22,7 +22,9 @@ import android.support.annotation.NonNull; import android.support.annotation.RequiresApi; import android.support.design.widget.FloatingActionButton; import android.support.graphics.drawable.VectorDrawableCompat; +import android.support.v4.app.ActivityCompat; import android.support.v4.content.ContextCompat; + import android.view.KeyEvent; import android.view.MenuItem; import android.view.View; @@ -34,6 +36,8 @@ import com.facebook.drawee.generic.GenericDraweeHierarchyBuilder; import com.facebook.drawee.view.SimpleDraweeView; import com.github.chrisbanes.photoview.PhotoView; +import java.io.File; +import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; @@ -62,6 +66,8 @@ import fr.free.nrw.commons.modifications.ModifierSequenceDao; import fr.free.nrw.commons.modifications.TemplateRemoveModifier; import fr.free.nrw.commons.mwapi.CategoryApi; import fr.free.nrw.commons.mwapi.MediaWikiApi; +import fr.free.nrw.commons.utils.ContributionUtils; +import fr.free.nrw.commons.utils.ExternalStorageUtils; import fr.free.nrw.commons.utils.ViewUtil; import timber.log.Timber; @@ -77,7 +83,9 @@ import static fr.free.nrw.commons.wikidata.WikidataConstants.WIKIDATA_ENTITY_ID_ public class ShareActivity extends AuthenticatedActivity implements SingleUploadFragment.OnUploadActionInitiated, - OnCategoriesSaveHandler { + OnCategoriesSaveHandler, + ActivityCompat.OnRequestPermissionsResultCallback { + private static final int REQUEST_PERM_ON_SUBMIT_STORAGE = 4; //Had to make them class variables, to extract out the click listeners, also I see no harm in this final Rect startBounds = new Rect(); @@ -120,6 +128,7 @@ public class ShareActivity private String mimeType; private CategorizationFragment categorizationFragment; private Uri mediaUri; + private Uri contentProviderUri; private Contribution contribution; private GPSExtractor gpsObj; private String decimalCoords; @@ -136,9 +145,12 @@ public class ShareActivity private long ShortAnimationDuration; private boolean isFABOpen = false; private float startScaleFinal; + private Bundle savedInstanceState; + private boolean isUploadFinalised = false; // Checks is user clicked to upload button or regret before this phase private boolean isZoom = false; + /** * Called when user taps the submit button. * Requests Storage permission, if needed. @@ -184,6 +196,7 @@ public class ShareActivity return !FileUtils.isSelfOwned(getApplicationContext(), mediaUri) && (ContextCompat.checkSelfPermission(this, Manifest.permission.READ_EXTERNAL_STORAGE) != PackageManager.PERMISSION_GRANTED); + //return false; } @@ -204,13 +217,12 @@ public class ShareActivity Timber.d("Cache the categories found"); } - uploadController.startUpload(title,mediaUri,description,mimeType,source,decimalCoords,wikiDataEntityId,c -> - - { - ShareActivity.this.contribution = c; - showPostUpload(); - }); -} + uploadController.startUpload(title, contentProviderUri, mediaUri, description, mimeType, source, decimalCoords, wikiDataEntityId, c -> { + ShareActivity.this.contribution = c; + showPostUpload(); + }); + isUploadFinalised = true; + } /** * Starts CategorizationFragment after uploadBegins. @@ -271,7 +283,7 @@ public class ShareActivity @Override public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); - + isUploadFinalised = false; setContentView(R.layout.activity_share); ButterKnife.bind(this); initBack(); @@ -282,9 +294,29 @@ public class ShareActivity .setFailureImage(VectorDrawableCompat.create(getResources(), R.drawable.ic_error_outline_black_24dp, getTheme())) .build()); + if (!ExternalStorageUtils.isStoragePermissionGranted(this)) { + this.savedInstanceState = savedInstanceState; + ExternalStorageUtils.requestExternalStoragePermission(this); + return; // Postpone operation to do after getting permission + } else { + receiveImageIntent(); + createContributionWithReceivedIntent(savedInstanceState); + } + } - receiveImageIntent(); + @Override + protected void onStop() { + // If upload is not finalised with failure or success, but contribution is created, + // we have to remove temp file, to prevent using unnecessary memory + if (!isUploadFinalised) { + if (mediaUri != null) { + ContributionUtils.removeTemporaryFile(mediaUri); + } + } + super.onStop(); + } + private void createContributionWithReceivedIntent(Bundle savedInstanceState) { if (savedInstanceState != null) { contribution = savedInstanceState.getParcelable("contribution"); } @@ -319,6 +351,11 @@ public class ShareActivity if (Intent.ACTION_SEND.equals(intent.getAction())) { mediaUri = intent.getParcelableExtra(Intent.EXTRA_STREAM); + + contentProviderUri = mediaUri; + + mediaUri = ContributionUtils.saveFileBeingUploadedTemporarily(this, mediaUri); + if (intent.hasExtra(UploadService.EXTRA_SOURCE)) { source = intent.getStringExtra(UploadService.EXTRA_SOURCE); } else { @@ -401,17 +438,20 @@ public class ShareActivity */ @Override public void onRequestPermissionsResult(int requestCode, @NonNull String[] permissions, @NonNull int[] grantResults) { - switch (requestCode) { - // Storage (from submit button) - this needs to be separate from (1) because only the - // submit button should bring user to next screen - case REQUEST_PERM_ON_SUBMIT_STORAGE: { - if (grantResults.length >= 1 && grantResults[0] == PackageManager.PERMISSION_GRANTED) { - checkIfFileExists(); + if (requestCode == 1 && grantResults.length > 0 && grantResults[0] == PackageManager.PERMISSION_GRANTED) { + Timber.d("onRequestPermissionsResult external storage permission granted"); + // You can receive image intent and save image to a temp file only if ext storage permission is granted + receiveImageIntent(); + createContributionWithReceivedIntent(savedInstanceState); - //Uploading only begins if storage permission granted from arrow icon - uploadBegins(); - } + if (requestCode == REQUEST_PERM_ON_SUBMIT_STORAGE) { + checkIfFileExists(); + //Uploading only begins if storage permission granted from arrow icon + uploadBegins(); } + + } else { + finish(); } } diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadController.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadController.java index dc87a5f05..b5aa4f514 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadController.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/UploadController.java @@ -15,9 +15,10 @@ import android.os.AsyncTask; import android.os.IBinder; import android.provider.MediaStore; import android.text.TextUtils; -import android.widget.Toast; +import android.util.Log; import java.io.BufferedInputStream; +import java.io.File; import java.io.IOException; import java.io.InputStream; import java.util.Date; @@ -100,7 +101,7 @@ public class UploadController { * @param wikiDataEntityId * @param onComplete the progress tracker */ - public void startUpload(String title, Uri mediaUri, String description, String mimeType, String source, String decimalCoords, String wikiDataEntityId, ContributionUploadProgress onComplete) { + public void startUpload(String title, Uri contentProviderUri, Uri mediaUri, String description, String mimeType, String source, String decimalCoords, String wikiDataEntityId, ContributionUploadProgress onComplete) { Contribution contribution; @@ -133,6 +134,7 @@ public class UploadController { contribution.setTag("mimeType", mimeType); contribution.setSource(source); contribution.setWikiDataEntityId(wikiDataEntityId); + contribution.setContentProviderUri(contentProviderUri); } @@ -168,9 +170,12 @@ public class UploadController { long length; ContentResolver contentResolver = context.getContentResolver(); try { + + //TODO: understand do we really need this code if (contribution.getDataLength() <= 0) { + Log.d("deneme","UploadController/doInBackground, contribution.getLocalUri():"+contribution.getLocalUri()); AssetFileDescriptor assetFileDescriptor = contentResolver - .openAssetFileDescriptor(contribution.getLocalUri(), "r"); + .openAssetFileDescriptor(Uri.fromFile(new File(contribution.getLocalUri().getPath())), "r"); if (assetFileDescriptor != null) { length = assetFileDescriptor.getLength(); if (length == -1) { @@ -220,7 +225,7 @@ public class UploadController { contribution.setDateCreated(new Date()); } } - return contribution; + return contribution; } @Override diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java index 0f3bff55c..505811ab1 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java @@ -10,9 +10,12 @@ import android.content.Intent; import android.graphics.BitmapFactory; import android.os.Bundle; import android.support.v4.app.NotificationCompat; +import android.util.Log; import android.webkit.MimeTypeMap; import android.widget.Toast; +import java.io.File; +import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; @@ -180,13 +183,15 @@ public class UploadService extends HandlerService { @SuppressLint("StringFormatInvalid") private void uploadContribution(Contribution contribution) { - InputStream file; + InputStream fileInputStream; String notificationTag = contribution.getLocalUri().toString(); try { //FIXME: Google Photos bug - file = this.getContentResolver().openInputStream(contribution.getLocalUri()); + File file1 = new File(contribution.getLocalUri().getPath()); + fileInputStream = new FileInputStream(file1); + //fileInputStream = this.getContentResolver().openInputStream(contribution.getLocalUri()); } catch (FileNotFoundException e) { Timber.d("File not found"); Toast fileNotFound = Toast.makeText(this, R.string.upload_failed, Toast.LENGTH_LONG); @@ -194,9 +199,9 @@ public class UploadService extends HandlerService { return; } - //As the file is null there's no point in continuing the upload process + //As the fileInputStream is null there's no point in continuing the upload process //mwapi.upload accepts a NonNull input stream - if(file == null) { + if(fileInputStream == null) { Timber.d("File not found"); return; } @@ -244,7 +249,7 @@ public class UploadService extends HandlerService { getString(R.string.upload_progress_notification_title_finishing, contribution.getDisplayTitle()), contribution ); - UploadResult uploadResult = mwApi.uploadFile(filename, file, contribution.getDataLength(), contribution.getPageContents(), contribution.getEditSummary(), notificationUpdater); + UploadResult uploadResult = mwApi.uploadFile(filename, fileInputStream, contribution.getDataLength(), contribution.getPageContents(), contribution.getEditSummary(), notificationUpdater, contribution.getLocalUri(), contribution.getContentProviderUri()); Timber.d("Response is %s", uploadResult.toString()); diff --git a/app/src/main/java/fr/free/nrw/commons/utils/ContributionUtils.java b/app/src/main/java/fr/free/nrw/commons/utils/ContributionUtils.java new file mode 100644 index 000000000..7be74feba --- /dev/null +++ b/app/src/main/java/fr/free/nrw/commons/utils/ContributionUtils.java @@ -0,0 +1,95 @@ +package fr.free.nrw.commons.utils; + +import android.content.Context; +import android.net.Uri; +import android.util.Log; + +import java.io.File; +import java.util.Random; + +import timber.log.Timber; + +/** + * This class includes utility methods for uploading process of images. + */ + +public class ContributionUtils { + + private static String TEMP_EXTERNAL_DIRECTORY = + android.os.Environment.getExternalStorageDirectory().getPath()+ + File.separatorChar+"UploadingByCommonsApp"; + + /** + * Saves images temporarily to a fixed folder and use Uri of that file during upload process. + * Otherwise, temporary Uri provided by content provider sometimes points to a null space and + * consequently upload fails. See: issue #1400A and E. + * Not: Saved image will be deleted, our directory will be empty after upload process. + * @return URI of saved image + */ + public static Uri saveFileBeingUploadedTemporarily(Context context, Uri URIfromContentProvider) { + // TODO add exceptions for Google Drive URİ is needed + Uri result = null; + + if (FileUtils.checkIfDirectoryExists(TEMP_EXTERNAL_DIRECTORY)) { + String destinationFilename = decideTempDestinationFileName(); + result = FileUtils.saveFileFromURI(context, URIfromContentProvider, destinationFilename); + } else { // If directory doesn't exist, create it and recursive call current method to check again + + File file = new File(TEMP_EXTERNAL_DIRECTORY); + if (file.mkdirs()) { + Timber.d("saveFileBeingUploadedTemporarily() parameters: URI from Content Provider %s", URIfromContentProvider); + result = saveFileBeingUploadedTemporarily(context, URIfromContentProvider); // If directory is created + } else { //An error occurred to create directory + Timber.e("saveFileBeingUploadedTemporarily() parameters: URI from Content Provider %s", URIfromContentProvider); + } + } + return result; + } + + /** + * Removes temp file created during upload + * @param tempFileUri + */ + public static void removeTemporaryFile(Uri tempFileUri) { + //TODO: do I have to notify file system about deletion? + File tempFile = new File(tempFileUri.getPath()); + if (tempFile.exists()) { + boolean isDeleted= tempFile.delete(); + Timber.e("removeTemporaryFile() parameters: URI tempFileUri %s, deleted status %b", tempFileUri, isDeleted); + } + } + + private static String decideTempDestinationFileName() { + int i = 0; + while (true) { + if (new File(TEMP_EXTERNAL_DIRECTORY +File.separatorChar+i+"_tmp").exists()) { + // This file is in use, try enother file + i++; + } else { + // Use time stamp for file name, so that two temporary file never has same file name + // to prevent previous file reference bug + Long tsLong = System.currentTimeMillis()/1000; + String ts = tsLong.toString(); + + // For multiple uploads, time randomisation should be combined with another random + // parameter, since they created at same time + int multipleUploadRandomParameter = new Random().nextInt(100); + return TEMP_EXTERNAL_DIRECTORY +File.separatorChar+ts+multipleUploadRandomParameter+"_tmp"; + } + } + } + + public static void emptyTemporaryDirectory() { + File dir = new File(TEMP_EXTERNAL_DIRECTORY); + if (dir.isDirectory()) + { + String[] children = dir.list(); + if (children != null && children.length >0) { + for (int i = 0; i < children.length; i++) + { + new File(dir, children[i]).delete(); + } + } + } + } +} diff --git a/app/src/main/java/fr/free/nrw/commons/utils/ExternalStorageUtils.java b/app/src/main/java/fr/free/nrw/commons/utils/ExternalStorageUtils.java new file mode 100644 index 000000000..56abcf6c6 --- /dev/null +++ b/app/src/main/java/fr/free/nrw/commons/utils/ExternalStorageUtils.java @@ -0,0 +1,47 @@ +package fr.free.nrw.commons.utils; + +import android.Manifest; +import android.app.Activity; +import android.content.Context; +import android.content.pm.PackageManager; +import android.os.Build; +import android.support.v4.app.ActivityCompat; +import android.util.Log; + +import timber.log.Timber; + +/** + * Created by root on 23.07.2018. + */ + +public class ExternalStorageUtils { + + /** + * Checks if external storage permission is granted + * @param context activity we are on + * @return true if permission is granted, false if not + */ + public static boolean isStoragePermissionGranted(Context context) { + if (Build.VERSION.SDK_INT >= 23) { + if (context.checkSelfPermission(android.Manifest.permission.WRITE_EXTERNAL_STORAGE) == PackageManager.PERMISSION_GRANTED) { + Timber.d("External storage permission granted, API >= 23"); + return true; + } else { + Timber.d("External storage permission not granted, API >= 23"); + return false; + } + } else { //permission is automatically granted on sdk<23 upon installation + Timber.d("External storage permission granted before, API < 23"); + return true; + } + } + + /** + * Requests external storage permission + * @param context activity we are on + */ + public static void requestExternalStoragePermission(Context context) { + Timber.d("External storage permission requested"); + ActivityCompat.requestPermissions((Activity) context, new String[]{Manifest.permission.WRITE_EXTERNAL_STORAGE}, 1); + } +} diff --git a/app/src/main/java/fr/free/nrw/commons/utils/FileUtils.java b/app/src/main/java/fr/free/nrw/commons/utils/FileUtils.java new file mode 100644 index 000000000..d1156c74f --- /dev/null +++ b/app/src/main/java/fr/free/nrw/commons/utils/FileUtils.java @@ -0,0 +1,89 @@ +package fr.free.nrw.commons.utils; + +import android.content.Context; +import android.net.Uri; + +import java.io.BufferedInputStream; +import java.io.BufferedOutputStream; +import java.io.File; +import java.io.FileInputStream; +import java.io.FileNotFoundException; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.net.URI; + +/** + * Created for file operations + */ + +public class FileUtils { + + /** + * Saves file from source URI to destination. + * @param sourceUri Uri which points to file to be saved + * @param destinationFilename where file will be located at + * @return Uri points to file saved + */ + public static Uri saveFileFromURI(Context context, Uri sourceUri, String destinationFilename) { + File file = new File(destinationFilename); + if (file.exists()) { + file.delete(); + } + + InputStream in = null; + OutputStream out = null; + try { + in = context.getContentResolver().openInputStream(sourceUri); + out = new FileOutputStream(new File(destinationFilename)); + + byte[] buf = new byte[1024]; + int len; + while((len=in.read(buf))>0){ + out.write(buf,0,len); + } + } catch (FileNotFoundException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } finally { + try { + out.close(); + in.close(); + } catch (IOException e) { + e.printStackTrace(); + } + } + + return Uri.parse("file://" + destinationFilename); + } + + /** + * Checks if directory exists + * @param pathToCheck path of directory to check + * @return true if directory exists, false otherwise + */ + public static boolean checkIfDirectoryExists(String pathToCheck) { + File director = new File(pathToCheck); + if(director.exists() && director.isDirectory()) { + return true; + } else { + return false; + } + } + + /** + * Creates new directory. + * @param pathToCreateAt where directory will be created at + * @return true if directory is created, false if an error occured, or already exists. + */ + public static boolean createDirectory(String pathToCreateAt) { + File directory = new File(pathToCreateAt); + if (!directory.exists()) { + return directory.mkdirs(); //true if directory is created + } else { + return false; //false if file already exists + } + } +}