Dbrant has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/361513 )
Change subject: Fix possible crash(es) when saving images from Gallery or Feed. ...................................................................... Fix possible crash(es) when saving images from Gallery or Feed. Half-hygiene, half crash fix: This patch brings our entire logic of saving images more up-to-date with our current patterns and practices. It removes the necessity to pass and persist Activity and Context instances in several places, and introduces proper Callback behavior that we know and love. It should also fix the following crash, if not a few others: https://rink.hockeyapp.net/manage/apps/226649/app_versions/204/crash_reasons/173113217 Change-Id: Ifc04526b8ce6107ee190c41644fb8a24c91d282c --- M app/src/main/java/org/wikipedia/gallery/GalleryActivity.java M app/src/main/java/org/wikipedia/gallery/GalleryItemFragment.java M app/src/main/java/org/wikipedia/gallery/ImagePipelineBitmapGetter.java M app/src/main/java/org/wikipedia/gallery/MediaDownloadReceiver.java M app/src/main/java/org/wikipedia/main/MainFragment.java 5 files changed, 95 insertions(+), 69 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/apps/android/wikipedia refs/changes/13/361513/1 diff --git a/app/src/main/java/org/wikipedia/gallery/GalleryActivity.java b/app/src/main/java/org/wikipedia/gallery/GalleryActivity.java index f9fcdde..2d7e224 100644 --- a/app/src/main/java/org/wikipedia/gallery/GalleryActivity.java +++ b/app/src/main/java/org/wikipedia/gallery/GalleryActivity.java @@ -5,6 +5,7 @@ import android.content.Context; import android.content.Intent; import android.content.IntentFilter; +import android.graphics.Bitmap; import android.net.Uri; import android.os.Bundle; import android.support.annotation.ColorInt; @@ -55,6 +56,7 @@ import org.wikipedia.views.ViewUtil; import org.wikipedia.views.WikiErrorView; +import java.io.File; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -64,7 +66,8 @@ import static org.wikipedia.util.UriUtil.handleExternalLink; import static org.wikipedia.util.UriUtil.resolveProtocolRelativeUrl; -public class GalleryActivity extends ThemedActionBarActivity implements LinkPreviewDialog.Callback { +public class GalleryActivity extends ThemedActionBarActivity implements LinkPreviewDialog.Callback, + GalleryItemFragment.Callback { public static final int ACTIVITY_RESULT_PAGE_SELECTED = 1; public static final String EXTRA_PAGETITLE = "pageTitle"; @@ -92,9 +95,6 @@ @Nullable private ViewPager.OnPageChangeListener pageChangeListener; @Nullable private GalleryFunnel funnel; - @Nullable protected GalleryFunnel getFunnel() { - return funnel; - } /** * If we have an intent that tells us a specific image to jump to within the gallery, @@ -109,7 +109,8 @@ private ViewPager galleryPager; private GalleryItemAdapter galleryAdapter; - private MediaDownloadReceiver downloadReceiver; + private MediaDownloadReceiver downloadReceiver = new MediaDownloadReceiver(); + private MediaDownloadReceiverCallback downloadReceiverCallback = new MediaDownloadReceiverCallback(); /** * Cache that stores GalleryItem information for each corresponding media item in @@ -171,7 +172,6 @@ super.onCreate(savedInstanceState); // force the theme to dark... setTheme(Theme.DARK.getResourceId()); - downloadReceiver = new MediaDownloadReceiver(this); getWindow().setFlags(WindowManager.LayoutParams.FLAG_FULLSCREEN, WindowManager.LayoutParams.FLAG_FULLSCREEN); setContentView(R.layout.activity_gallery); @@ -276,12 +276,32 @@ public void onResume() { super.onResume(); registerReceiver(downloadReceiver, new IntentFilter(DownloadManager.ACTION_DOWNLOAD_COMPLETE)); + downloadReceiver.setCallback(downloadReceiverCallback); } @Override public void onPause() { super.onPause(); + downloadReceiver.setCallback(null); unregisterReceiver(downloadReceiver); + } + + @Override + public void onDownload(@NonNull GalleryItem item) { + funnel.logGallerySave(pageTitle, item.getName()); + downloadReceiver.download(this, item); + FeedbackUtil.showMessage(this, R.string.gallery_save_progress); + } + + @Override + public void onShare(@NonNull GalleryItem item, @Nullable Bitmap bitmap, @NonNull String subject, @NonNull PageTitle title) { + funnel.logGalleryShare(pageTitle, item.getName()); + if (bitmap != null) { + ShareUtil.shareImage(this, bitmap, new File(item.getUrl()).getName(), subject, + title.getCanonicalUri()); + } else { + ShareUtil.shareText(this, title); + } } private class GalleryPageChangeListener extends ViewPager.SimpleOnPageChangeListener { @@ -331,10 +351,6 @@ funnel.logGalleryClose(pageTitle, getCurrentItem().getName()); } super.onBackPressed(); - } - - public MediaDownloadReceiver getDownloadReceiver() { - return downloadReceiver; } /** @@ -688,6 +704,13 @@ } } + private class MediaDownloadReceiverCallback implements MediaDownloadReceiver.Callback { + @Override + public void onSuccess() { + FeedbackUtil.showMessage(GalleryActivity.this, R.string.gallery_save_success); + } + } + @ColorInt private int color(@ColorRes int id) { return ContextCompat.getColor(this, id); } diff --git a/app/src/main/java/org/wikipedia/gallery/GalleryItemFragment.java b/app/src/main/java/org/wikipedia/gallery/GalleryItemFragment.java index 5d2e50a..be8e472 100644 --- a/app/src/main/java/org/wikipedia/gallery/GalleryItemFragment.java +++ b/app/src/main/java/org/wikipedia/gallery/GalleryItemFragment.java @@ -34,6 +34,7 @@ import org.wikipedia.Constants; import org.wikipedia.R; import org.wikipedia.WikipediaApp; +import org.wikipedia.activity.FragmentUtil; import org.wikipedia.dataclient.WikiSite; import org.wikipedia.page.PageTitle; import org.wikipedia.util.FeedbackUtil; @@ -42,7 +43,6 @@ import org.wikipedia.util.ShareUtil; import org.wikipedia.util.log.L; -import java.io.File; import java.util.Map; import static org.wikipedia.util.PermissionUtil.hasWriteExternalStoragePermission; @@ -54,6 +54,11 @@ public static final String ARG_MIMETYPE = "mimeType"; public static final String ARG_FEED_FEATURED_IMAGE = "feedFeaturedImage"; public static final String ARG_AGE = "age"; + + public interface Callback { + void onDownload(@NonNull GalleryItem item); + void onShare(@NonNull GalleryItem item, @Nullable Bitmap bitmap, @NonNull String subject, @NonNull PageTitle title); + } private ProgressBar progressBar; private ZoomableDraweeView imageView; @@ -406,33 +411,18 @@ .build()); } - /** - * Share the current image using an activity chooser, so that the user can choose the - * app with which to share the content. - * This is done by saving the image to a temporary file in external storage, then specifying - * that file in the share intent. The name of the temporary file is kept constant, so that - * it's overwritten every time an image is shared from the app, so that it takes up a - * constant amount of space. - */ private void shareImage() { if (galleryItem == null) { return; } - parentActivity.getFunnel().logGalleryShare(pageTitle, galleryItem.getName()); - new ImagePipelineBitmapGetter(getActivity(), galleryItem.getThumbUrl()){ + new ImagePipelineBitmapGetter(galleryItem.getThumbUrl()){ @Override public void onSuccess(@Nullable Bitmap bitmap) { if (!isAdded()) { return; } - if (bitmap != null) { - ShareUtil.shareImage(parentActivity, - bitmap, - new File(galleryItem.getUrl()).getName(), - getShareSubject(), - imageTitle.getCanonicalUri()); - } else { - ShareUtil.shareText(parentActivity, imageTitle); + if (callback() != null) { + callback().onShare(galleryItem, bitmap, getShareSubject(), imageTitle); } } }.get(); @@ -466,14 +456,16 @@ } private void saveImage() { - if (galleryItem == null) { - return; + if (galleryItem != null && callback() != null) { + callback().onDownload(galleryItem); } - parentActivity.getFunnel().logGallerySave(pageTitle, galleryItem.getName()); - parentActivity.getDownloadReceiver().download(galleryItem); } private boolean shouldHaveWhiteBackground(String mimeType) { return mimeType.contains("svg") || mimeType.contains("png") || mimeType.contains("gif"); } + + @Nullable private Callback callback() { + return FragmentUtil.getCallback(this, Callback.class); + } } diff --git a/app/src/main/java/org/wikipedia/gallery/ImagePipelineBitmapGetter.java b/app/src/main/java/org/wikipedia/gallery/ImagePipelineBitmapGetter.java index 3992563..4d1b149 100644 --- a/app/src/main/java/org/wikipedia/gallery/ImagePipelineBitmapGetter.java +++ b/app/src/main/java/org/wikipedia/gallery/ImagePipelineBitmapGetter.java @@ -1,6 +1,5 @@ package org.wikipedia.gallery; -import android.content.Context; import android.graphics.Bitmap; import android.graphics.Canvas; import android.graphics.Paint; @@ -18,19 +17,19 @@ import com.facebook.imagepipeline.request.ImageRequest; import com.facebook.imagepipeline.request.ImageRequestBuilder; +import org.wikipedia.WikipediaApp; + public abstract class ImagePipelineBitmapGetter { - private Context context; private String imageUrl; - public ImagePipelineBitmapGetter(Context context, String imageUrl) { - this.context = context; + public ImagePipelineBitmapGetter(String imageUrl) { this.imageUrl = imageUrl; } public abstract void onSuccess(@Nullable Bitmap bitmap); public void onError(Throwable t) { - Toast.makeText(context, t.getLocalizedMessage(), Toast.LENGTH_SHORT).show(); + Toast.makeText(WikipediaApp.getInstance(), t.getLocalizedMessage(), Toast.LENGTH_SHORT).show(); } public void get() { @@ -38,7 +37,8 @@ .setProgressiveRenderingEnabled(true) .build(); ImagePipeline imagePipeline = Fresco.getImagePipeline(); - DataSource<CloseableReference<CloseableImage>> dataSource = imagePipeline.fetchDecodedImage(request, context); + DataSource<CloseableReference<CloseableImage>> dataSource + = imagePipeline.fetchDecodedImage(request, WikipediaApp.getInstance()); dataSource.subscribe(new BitmapDataSubscriber(), UiThreadImmediateExecutorService.getInstance()); } diff --git a/app/src/main/java/org/wikipedia/gallery/MediaDownloadReceiver.java b/app/src/main/java/org/wikipedia/gallery/MediaDownloadReceiver.java index f67f5fb..aa583df 100644 --- a/app/src/main/java/org/wikipedia/gallery/MediaDownloadReceiver.java +++ b/app/src/main/java/org/wikipedia/gallery/MediaDownloadReceiver.java @@ -1,6 +1,5 @@ package org.wikipedia.gallery; -import android.app.Activity; import android.app.DownloadManager; import android.content.BroadcastReceiver; import android.content.ContentValues; @@ -13,29 +12,29 @@ import android.support.annotation.NonNull; import android.support.annotation.Nullable; -import org.wikipedia.R; import org.wikipedia.feed.image.FeaturedImage; -import org.wikipedia.util.FeedbackUtil; import org.wikipedia.util.FileUtil; public class MediaDownloadReceiver extends BroadcastReceiver { private static final String FILE_NAMESPACE = "File:"; - @NonNull private Activity activity; - @NonNull private DownloadManager downloadManager; - - public MediaDownloadReceiver(@NonNull Activity activity) { - this.activity = activity; - downloadManager = (DownloadManager) activity.getSystemService(Context.DOWNLOAD_SERVICE); + public interface Callback { + void onSuccess(); } - public void download(@NonNull FeaturedImage featuredImage) { + @Nullable private Callback callback; + + public void setCallback(@Nullable Callback callback) { + this.callback = callback; + } + + public void download(@NonNull Context context, @NonNull FeaturedImage featuredImage) { String filename = FileUtil.sanitizeFileName(featuredImage.title()); String targetDirectory = Environment.DIRECTORY_PICTURES; - performDownloadRequest(featuredImage.image().source(), targetDirectory, filename, null); + performDownloadRequest(context, featuredImage.image().source(), targetDirectory, filename, null); } - public void download(@NonNull GalleryItem galleryItem) { + public void download(@NonNull Context context, @NonNull GalleryItem galleryItem) { String saveFilename = FileUtil.sanitizeFileName(trimFileNamespace(galleryItem.getName())); String targetDirectory; if (FileUtil.isVideo(galleryItem.getMimeType())) { @@ -47,23 +46,22 @@ } else { targetDirectory = Environment.DIRECTORY_DOWNLOADS; } - performDownloadRequest(Uri.parse(galleryItem.getUrl()), targetDirectory, saveFilename, - galleryItem.getMimeType()); + performDownloadRequest(context, Uri.parse(galleryItem.getUrl()), targetDirectory, + saveFilename, galleryItem.getMimeType()); } - private void performDownloadRequest(@NonNull Uri uri, @NonNull String targetDirectory, - @NonNull String filename, @Nullable String mimeType) { + private void performDownloadRequest(@NonNull Context context, @NonNull Uri uri, + @NonNull String targetDirectory, @NonNull String filename, + @Nullable String mimeType) { + DownloadManager downloadManager = (DownloadManager) context.getSystemService(Context.DOWNLOAD_SERVICE); DownloadManager.Request request = new DownloadManager.Request(uri); - request.setDestinationInExternalFilesDir(activity, targetDirectory, filename); + request.setDestinationInExternalFilesDir(context, targetDirectory, filename); request.setNotificationVisibility(DownloadManager.Request.VISIBILITY_VISIBLE_NOTIFY_COMPLETED); if (mimeType != null) { request.setMimeType(mimeType); } request.allowScanningByMediaScanner(); - - downloadManager = (DownloadManager) activity.getSystemService(Context.DOWNLOAD_SERVICE); downloadManager.enqueue(request); - FeedbackUtil.showMessage(activity, R.string.gallery_save_progress); } @Override @@ -73,6 +71,7 @@ long downloadId = intent.getLongExtra(DownloadManager.EXTRA_DOWNLOAD_ID, 0); DownloadManager.Query query = new DownloadManager.Query(); query.setFilterById(downloadId); + DownloadManager downloadManager = (DownloadManager) context.getSystemService(Context.DOWNLOAD_SERVICE); Cursor c = downloadManager.query(query); try { if (c.moveToFirst()) { @@ -80,8 +79,10 @@ int pathIndex = c.getColumnIndexOrThrow(DownloadManager.COLUMN_LOCAL_URI); int mimeIndex = c.getColumnIndexOrThrow(DownloadManager.COLUMN_MEDIA_TYPE); if (DownloadManager.STATUS_SUCCESSFUL == c.getInt(statusIndex)) { - FeedbackUtil.showMessage(activity, R.string.gallery_save_success); - notifyContentResolver(Uri.parse(c.getString(pathIndex)).getPath(), + if (callback != null) { + callback.onSuccess(); + } + notifyContentResolver(context, Uri.parse(c.getString(pathIndex)).getPath(), c.getString(mimeIndex)); } } @@ -95,7 +96,7 @@ return filename.startsWith(FILE_NAMESPACE) ? filename.substring(FILE_NAMESPACE.length()) : filename; } - private void notifyContentResolver(@NonNull String path, @NonNull String mimeType) { + private void notifyContentResolver(@NonNull Context context, @NonNull String path, @NonNull String mimeType) { ContentValues values = new ContentValues(); Uri contentUri; if (FileUtil.isVideo(mimeType)) { @@ -111,6 +112,6 @@ values.put(MediaStore.Images.Media.MIME_TYPE, mimeType); contentUri = MediaStore.Images.Media.EXTERNAL_CONTENT_URI; } - activity.getContentResolver().insert(contentUri, values); + context.getContentResolver().insert(contentUri, values); } } diff --git a/app/src/main/java/org/wikipedia/main/MainFragment.java b/app/src/main/java/org/wikipedia/main/MainFragment.java index 43c457b..7f6ab17 100644 --- a/app/src/main/java/org/wikipedia/main/MainFragment.java +++ b/app/src/main/java/org/wikipedia/main/MainFragment.java @@ -78,7 +78,8 @@ @BindView(R.id.fragment_main_nav_tab_layout) NavTabLayout tabLayout; private Unbinder unbinder; private ExclusiveBottomSheetPresenter bottomSheetPresenter = new ExclusiveBottomSheetPresenter(); - private MediaDownloadReceiver mediaDownloadReceiver; + private MediaDownloadReceiver downloadReceiver = new MediaDownloadReceiver(); + private MediaDownloadReceiverCallback downloadReceiverCallback = new MediaDownloadReceiverCallback(); // The permissions request API doesn't take a callback, so in the event we have to // ask for permission to download a featured image from the feed, we'll have to hold @@ -125,13 +126,15 @@ @Override public void onPause() { super.onPause(); - getContext().unregisterReceiver(mediaDownloadReceiver); + downloadReceiver.setCallback(null); + getContext().unregisterReceiver(downloadReceiver); } @Override public void onResume() { super.onResume(); - getContext().registerReceiver(mediaDownloadReceiver, + getContext().registerReceiver(downloadReceiver, new IntentFilter(DownloadManager.ACTION_DOWNLOAD_COMPLETE)); + downloadReceiver.setCallback(downloadReceiverCallback); // update toolbar, since Tab count might have changed getActivity().supportInvalidateOptionsMenu(); // reset the last-page-viewed timer @@ -166,7 +169,6 @@ public void onActivityCreated(Bundle savedInstanceState) { super.onActivityCreated(savedInstanceState); setHasOptionsMenu(true); - mediaDownloadReceiver = new MediaDownloadReceiver(getActivity()); } @Override @@ -274,7 +276,7 @@ @Override public void onFeedShareImage(final FeaturedImageCard card) { final String thumbUrl = card.baseImage().thumbnail().source().toString(); final String fullSizeUrl = card.baseImage().image().source().toString(); - new ImagePipelineBitmapGetter(getContext(), thumbUrl) { + new ImagePipelineBitmapGetter(thumbUrl) { @Override public void onSuccess(@Nullable Bitmap bitmap) { if (bitmap != null) { @@ -445,7 +447,8 @@ private void download(@NonNull FeaturedImage image) { setPendingDownload(null); - new MediaDownloadReceiver(getActivity()).download(image); + downloadReceiver.download(getContext(), image); + FeedbackUtil.showMessage(this, R.string.gallery_save_progress); } private void setPendingDownload(@Nullable FeaturedImage image) { @@ -490,6 +493,13 @@ cancelSearch(); } + private class MediaDownloadReceiverCallback implements MediaDownloadReceiver.Callback { + @Override + public void onSuccess() { + FeedbackUtil.showMessage(getActivity(), R.string.gallery_save_success); + } + } + @Nullable private Callback callback() { return FragmentUtil.getCallback(this, Callback.class); } -- To view, visit https://gerrit.wikimedia.org/r/361513 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ifc04526b8ce6107ee190c41644fb8a24c91d282c Gerrit-PatchSet: 1 Gerrit-Project: apps/android/wikipedia Gerrit-Branch: master Gerrit-Owner: Dbrant <dbr...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
