1246238 - First time a RV item is Bookmarked, use Helper UI to tell users of Offline ability

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_firsttime_feedback1.png

1) User RV's an item 
2) User Bookmarks said item
3) See this helper UI about Bookmarks integration

4A) DISMISS -> press outside helper, see snackbar confirmation
4B) BOOKMARKS -> press big orange button, go to Bookmarks -> Reading List folder

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ re-uses our helper UI styles from https://bugzilla.mozilla.org/show_bug.cgi?id=1217174#c63

Will attach the graphic next.

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: img_offlinebookmarks.zip

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Note: Copy TBD

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_firsttime_feedback1.png

Copy finalized.

Comment 5

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1246238 - Pre: Implement SimpleHelperUI r?sebastian

This provides a basic helper UI that can be customised with images/text.
We need a very similar helper for both reader-view offline bookmarking related
helpers (Bug 1236328 and Bug 1247689), hence it's useful to have a common
class implementing most of the required functionality.

Most of the new helper is borrowed from the existing HomeScreenPrompt. I will
extract the common functionality in a followup Bug.

Review commit: https://reviewboard.mozilla.org/r/47999/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47999/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1246238 - Show helper UI first time a reader view page is bookmarked r?sebastian

Review commit: https://reviewboard.mozilla.org/r/48001/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48001/

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8743717 [details]
MozReview Request: Bug 1246238 - Pre: Implement SimpleHelperUI r?sebastian

https://reviewboard.mozilla.org/r/47999/#review45155

It looks like you are trying to create one implementation for all helper UIs. I wonder if you are going to succeed with that: HomeScreenPrompt has additional code to load the icon (no "resource") and TabQueuePrompt currently has a lot of permission code in there (even though this could be simplified).

Maybe it makes more sense to create a "base" activity that creates the basic layout, handles the animations, touching outside, clicking the close button, has helpful methods and the different prompts extend from it, inject their own layout in the shell provided by the base class and then do whatever they want to do. What's nice about this is that it is more flexible and you can handle the success/rejection case in the prompt (if possible) and do not necessarily need to return something and this is again handled somewhere else.

However this is a step in the direction (r+) and we can transform this in a follow-up bug if you agree.

::: mobile/android/base/java/org/mozilla/gecko/promotion/SimpleHelperUI.java:100
(Diff revision 1)
> +    private void setupViews() {
> +        final Intent i = getIntent();
> +
> +        setContentView(R.layout.simple_helper_ui);
> +
> +        ((ImageView) findViewById(R.id.image)).setImageResource(i.getIntExtra(EXTRA_IMAGE, -1));

I've seen 0 used as "no resource id" but never -1. Note that resource ids can be negative, so maybe -1 could even be a valid resource?
Let's avoid settig something if there's nothing to set.

::: mobile/android/base/java/org/mozilla/gecko/promotion/SimpleHelperUI.java:110
(Diff revision 1)
> +
> +        ((Button) findViewById(R.id.button)).setText(i.getIntExtra(EXTRA_BUTTON, -1));
> +
> +        containerView = findViewById(R.id.container);
> +
> +        findViewById(R.id.button).setOnClickListener(new View.OnClickListener() {

NIT: As soon as you search the same view by id twice it might be worth keeping a reference to the result.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8743718 [details]
MozReview Request: Bug 1246238 - Show helper UI first time a reader view page is bookmarked r?sebastian

https://reviewboard.mozilla.org/r/48001/#review45157

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:378
(Diff revision 1)
> +                    final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this);
> +
> +                    final boolean hasFirstReaderViewPromptBeenShownBefore = prefs.getBoolean(SimpleHelperUI.PREF_FIRST_RVBP_SHOWN, false);
> +
> +                    if (hasFirstReaderViewPromptBeenShownBefore) {
> -                    showReaderModeBookmarkAddedSnackbar();
> +                        showReaderModeBookmarkAddedSnackbar();
> +                    } else {
> +                        SimpleHelperUI.show(this,
> +                                SimpleHelperUI.FIRST_RVBP_SHOWN_TELEMETRYEXTRA,
> +                                ACTIVITY_REQUEST_FIRST_READERVIEW_BOOKMARK,
> +                                R.string.helper_first_offline_bookmark_title, R.string.helper_first_offline_bookmark_message,
> +                                R.drawable.helper_first_readerview_bookmark, R.string.helper_first_offline_bookmark_button,
> +                                ACTIVITY_RESULT_FIRST_READERVIEW_BOOKMARKS_GOTO_BOOKMARKS,
> +                                ACTIVITY_RESULT_FIRST_READERVIEW_BOOKMARKS_IGNORE);
> +
> +                        GeckoSharedPrefs.forProfile(this)
> +                                .edit()
> +                                .putBoolean(SimpleHelperUI.PREF_FIRST_RVBP_SHOWN, true)
> +                                .apply();
> +                    }

(Like comment in bug 1247689): I would like to see this moved into its own class.

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: img_offlinebookmarks2.zip

Since this hasn't closed yet, I just want to update this image. The other images will be replaced in bug 1266899.

Comment 10

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/47999/#review45155

Yup: in this case I knew I was implementing 2 helper UIs that were identical except for the text in them, I doubt we'll be able to reuse the layout for any of the existing prompts. I'm guessing we *might* have more prompts in future in which case the SimpleHelperUI might be useful, but for now I just wanted to share code for these two prompts.

My main aim in Bug 1266182 is to refactor out the actual common functionality (i.e. the "base" activity you mention), I'll copy your comments to there. I didn't want to do too much in this direction as part of this bug since I was focussing on getting this into 48!

Comment 11

[Andrzej Hunt :ahunt]

(In reply to Sebastian Kaspari (:sebastian) from comment #8)
> (Like comment in bug 1247689): I would like to see this moved into its own
> class.

Because of uplifts / string-freeze (which I think is tomorrow), and to ensure this makes into 48, I've decided to land as is (but with updated images). It'll be much easier to move this code into a separate class once the BrowserAppDelegate lands (which I'll need to extend with an API for handling onActivityResult), so I'll probably wait for that to land, and then cleanup as part of bug 1247689.

Comment 12

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/d0bed027367312449a35fb4f3a50b68a250545d9
Bug 1246238 - Pre: Implement SimpleHelperUI r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/f9177e655c8c7c3250087d71c0b6fed5e335e1b5
Bug 1246238 - Show helper UI first time a reader view page is bookmarked r=sebastian

Comment 13

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/ecdcf1026a3ba4bcd21304627a0eceba8f285ba0
Bug 1246238 - Post: make checkstyle happy r=me

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0bed0273673
https://hg.mozilla.org/mozilla-central/rev/f9177e655c8c
https://hg.mozilla.org/mozilla-central/rev/ecdcf1026a3b

Comment 15

[Andrzej Hunt :ahunt]

Attachment: Screenshot as landed

And here's a screenshot from the version that landed!

Comment 16

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_20160429-122020.png

Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 48.0a2 (2016-04-28)