jenkins-bot has submitted this change and it was merged. ( https://gerrit.wikimedia.org/r/327005 )
Change subject: Hygiene: Clean up misuse of PageTitles in GalleryActivity ...................................................................... Hygiene: Clean up misuse of PageTitles in GalleryActivity The logger expects a PageTitle object coming from the gallery activity, but we may not be in the context of wiki page since the gallery can now also be launched from the feed featured image card. An earlier commit introduced the idea of keeping around a placeholder PageTitle for logging purposes in the event we're coming from the feed featured image card. But storing this in the pageTitle member variable is a bad idea since this makes creates a risk it'll end up getting inappropriately passed to the content model for the page data client or inserted into the cache. This patch eliminates such use of the PageTitle class and updates downstream usages of the PageTitle object as need. To follow up: figure out a good way to pass the featured image date through to the gallery for use in a share subject. A FeaturedImageGalleryItem subclass would likely be appropriate here. Bug: T152980 Change-Id: I74ef3557700018176310097180515664652b13ca --- M app/src/main/java/org/wikipedia/analytics/GalleryFunnel.java 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/main/MainFragment.java M app/src/main/java/org/wikipedia/util/ShareUtil.java 5 files changed, 42 insertions(+), 36 deletions(-) Approvals: Niedzielski: Looks good to me, approved jenkins-bot: Verified diff --git a/app/src/main/java/org/wikipedia/analytics/GalleryFunnel.java b/app/src/main/java/org/wikipedia/analytics/GalleryFunnel.java index 4403667..067dbd5 100644 --- a/app/src/main/java/org/wikipedia/analytics/GalleryFunnel.java +++ b/app/src/main/java/org/wikipedia/analytics/GalleryFunnel.java @@ -38,7 +38,9 @@ private void logGalleryAction(String action, PageTitle currentPageTitle, String currentMediaTitle) { log( "action", action, - "pageTitle", currentPageTitle.getDisplayText(), + "pageTitle", currentPageTitle != null + ? currentPageTitle.getDisplayText() + : "FeedFeaturedImage", "imageTitle", currentMediaTitle ); } diff --git a/app/src/main/java/org/wikipedia/gallery/GalleryActivity.java b/app/src/main/java/org/wikipedia/gallery/GalleryActivity.java index bd415a0..794d746 100644 --- a/app/src/main/java/org/wikipedia/gallery/GalleryActivity.java +++ b/app/src/main/java/org/wikipedia/gallery/GalleryActivity.java @@ -138,23 +138,26 @@ }; @NonNull - public static Intent newIntent(@NonNull Context context, @NonNull PageTitle pageTitle, - @NonNull String filename, @NonNull WikiSite wiki, int source, + public static Intent newIntent(@NonNull Context context, @NonNull String filename, + @NonNull WikiSite wiki, int source, @NonNull FeaturedImage image) { - return newIntent(context, pageTitle, filename, wiki, source) - .putExtra(EXTRA_FEATURED_IMAGE, GsonMarshaller.marshal(image)) - .putExtra(EXTRA_IS_FEATURED_IMAGE, true); + return newIntent(context, null, filename, wiki, + source).putExtra(EXTRA_FEATURED_IMAGE, GsonMarshaller.marshal(image)); } @NonNull - public static Intent newIntent(@NonNull Context context, @NonNull PageTitle pageTitle, + public static Intent newIntent(@NonNull Context context, @Nullable PageTitle pageTitle, @NonNull String filename, @NonNull WikiSite wiki, int source) { - return new Intent() + Intent intent = new Intent() .setClass(context, GalleryActivity.class) .putExtra(EXTRA_FILENAME, filename) .putExtra(EXTRA_WIKI, wiki) - .putExtra(EXTRA_PAGETITLE, pageTitle) .putExtra(EXTRA_SOURCE, source); + if (pageTitle != null) { + intent.putExtra(EXTRA_PAGETITLE, pageTitle); + } + + return intent; } @Override @@ -185,7 +188,9 @@ creditText = (TextView) findViewById(R.id.gallery_credit_text); creditText.setShadowLayer(2, 1, 1, color(R.color.lead_text_shadow)); - pageTitle = getIntent().getParcelableExtra(EXTRA_PAGETITLE); + if (getIntent().hasExtra(EXTRA_PAGETITLE)) { + pageTitle = getIntent().getParcelableExtra(EXTRA_PAGETITLE); + } initialFilename = getIntent().getStringExtra(EXTRA_FILENAME); wiki = getIntent().getParcelableExtra(EXTRA_WIKI); @@ -227,15 +232,10 @@ updateProgressBar(false, true, 0); - if (pageTitle == null) { - throw new IllegalStateException("pageTitle should not be null"); - } else if (getIntent().hasExtra(EXTRA_IS_FEATURED_IMAGE) - && getIntent().getBooleanExtra(EXTRA_IS_FEATURED_IMAGE, false)) { + if (getIntent().hasExtra(EXTRA_FEATURED_IMAGE)) { FeaturedImage featuredImage = GsonUnmarshaller.unmarshal(FeaturedImage.class, getIntent().getStringExtra(EXTRA_FEATURED_IMAGE)); - if (featuredImage != null) { - loadGalleryItemFor(featuredImage); - } + loadGalleryItemFor(featuredImage); } else { // find our Page in the page cache... app.getPageCache().get(pageTitle, 0, new PageCache.CacheGetListener() { @@ -269,7 +269,7 @@ super.onDestroy(); } - private void loadGalleryItemFor(FeaturedImage image) { + private void loadGalleryItemFor(@NonNull FeaturedImage image) { List<GalleryItem> list = new ArrayList<>(); list.add(new GalleryItem(image)); applyGalleryCollection(new GalleryCollection(list)); @@ -418,12 +418,12 @@ * Retrieve the complete list of media items for the current page. * When retrieved, the list will be passed to the ViewPager, and will become a * scrollable gallery of media. - * - * WARNING: This may be useful if/when we start loading multiple images into the gallery from - * the feed, but it won't work with the current dummy PageTitle we're using when coming from - * the feed. */ private void fetchGalleryCollection() { + if (pageTitle == null) { + return; + } + updateProgressBar(true, true, 0); new GalleryCollectionFetchTask(app.getAPIForSite(pageTitle.getWikiSite()), pageTitle.getWikiSite(), pageTitle) { @@ -450,7 +450,7 @@ * Apply a complete collection of media to our scrollable gallery. * @param collection GalleryCollection to apply to the ViewPager. */ - protected void applyGalleryCollection(GalleryCollection collection) { + protected void applyGalleryCollection(@NonNull GalleryCollection collection) { // remove the page transformer while we operate on the pager... galleryPager.setPageTransformer(false, null); // first, verify that the collection contains the item that the user @@ -582,7 +582,7 @@ fragmentArray = new SparseArray<>(); } - public void setCollection(GalleryCollection collection) { + public void setCollection(@NonNull GalleryCollection collection) { galleryCollection = collection; notifyDataSetChanged(); } diff --git a/app/src/main/java/org/wikipedia/gallery/GalleryItemFragment.java b/app/src/main/java/org/wikipedia/gallery/GalleryItemFragment.java index dd44278..4fd4fbc 100644 --- a/app/src/main/java/org/wikipedia/gallery/GalleryItemFragment.java +++ b/app/src/main/java/org/wikipedia/gallery/GalleryItemFragment.java @@ -418,7 +418,9 @@ ShareUtil.shareImage(parentActivity, bitmap, new File(galleryItem.getUrl()).getName(), - pageTitle.getDisplayText(), + pageTitle != null + ? pageTitle.getDisplayText() + : ShareUtil.getFeaturedImageShareSubject(getContext(), null), imageTitle.getCanonicalUri()); } else { ShareUtil.shareText(parentActivity, imageTitle); diff --git a/app/src/main/java/org/wikipedia/main/MainFragment.java b/app/src/main/java/org/wikipedia/main/MainFragment.java index 3f9111c..7a0ca03 100644 --- a/app/src/main/java/org/wikipedia/main/MainFragment.java +++ b/app/src/main/java/org/wikipedia/main/MainFragment.java @@ -31,7 +31,6 @@ import org.wikipedia.activity.FragmentUtil; import org.wikipedia.analytics.GalleryFunnel; import org.wikipedia.analytics.IntentFunnel; -import org.wikipedia.dataclient.WikiSite; import org.wikipedia.feed.FeedFragment; import org.wikipedia.feed.image.FeaturedImage; import org.wikipedia.feed.image.FeaturedImageCard; @@ -56,7 +55,6 @@ import org.wikipedia.search.SearchInvokeSource; import org.wikipedia.settings.Prefs; import org.wikipedia.util.ClipboardUtil; -import org.wikipedia.util.DateUtil; import org.wikipedia.util.FeedbackUtil; import org.wikipedia.util.PermissionUtil; import org.wikipedia.util.ShareUtil; @@ -252,7 +250,7 @@ ShareUtil.shareImage(getContext(), bitmap, new File(thumbUrl).getName(), - featuredImageShareSubject(card), + ShareUtil.getFeaturedImageShareSubject(getContext(), card), fullSizeUrl); } else { FeedbackUtil.showMessage(MainFragment.this, getString(R.string.gallery_share_error, card.baseImage().title())); @@ -271,9 +269,8 @@ } @Override public void onFeaturedImageSelected(FeaturedImageCard card) { - final WikiSite wiki = card.wikiSite(); - startActivityForResult(GalleryActivity.newIntent(getActivity(), new PageTitle(featuredImageShareSubject(card), wiki), - card.filename(), wiki, GalleryFunnel.SOURCE_FEED_FEATURED_IMAGE, card.baseImage()), + startActivityForResult(GalleryActivity.newIntent(getActivity(), card.filename(), + card.wikiSite(), GalleryFunnel.SOURCE_FEED_FEATURED_IMAGE, card.baseImage()), Constants.ACTIVITY_REQUEST_GALLERY); } @@ -470,11 +467,6 @@ if (behavior != null) { behavior.onNestedFling(coordinatorLayout, paddingAppBar, null, 0, params.height, true); } - } - - private String featuredImageShareSubject(FeaturedImageCard card) { - return getString(R.string.feed_featured_image_share_subject) + " | " - + DateUtil.getFeedCardDateString(card.date().baseCalendar()); } @Nullable private Callback callback() { diff --git a/app/src/main/java/org/wikipedia/util/ShareUtil.java b/app/src/main/java/org/wikipedia/util/ShareUtil.java index 0b2ad1d..60b0196 100644 --- a/app/src/main/java/org/wikipedia/util/ShareUtil.java +++ b/app/src/main/java/org/wikipedia/util/ShareUtil.java @@ -16,6 +16,7 @@ import org.wikipedia.BuildConfig; import org.wikipedia.R; import org.wikipedia.concurrency.SaneAsyncTask; +import org.wikipedia.feed.image.FeaturedImageCard; import org.wikipedia.page.PageTitle; import org.wikipedia.util.log.L; @@ -89,6 +90,15 @@ }.execute(); } + public static String getFeaturedImageShareSubject(@NonNull Context context, + @Nullable FeaturedImageCard card) { + String result = context.getString(R.string.feed_featured_image_share_subject); + if (card != null) { + result = result + " | " + DateUtil.getFeedCardDateString(card.date().baseCalendar()); + } + return result; + } + public static Intent buildImageShareChooserIntent(Context context, String subject, String text, Uri uri) { Intent shareIntent = createImageShareIntent(subject, text, uri); return Intent.createChooser(shareIntent, -- To view, visit https://gerrit.wikimedia.org/r/327005 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I74ef3557700018176310097180515664652b13ca Gerrit-PatchSet: 3 Gerrit-Project: apps/android/wikipedia Gerrit-Branch: master Gerrit-Owner: Mholloway <mhollo...@wikimedia.org> Gerrit-Reviewer: BearND <bsitzm...@wikimedia.org> Gerrit-Reviewer: Brion VIBBER <br...@wikimedia.org> Gerrit-Reviewer: Dbrant <dbr...@wikimedia.org> Gerrit-Reviewer: Mholloway <mhollo...@wikimedia.org> Gerrit-Reviewer: Niedzielski <sniedziel...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits