Closed Bug 1246238 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox48 verified)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- verified

People

(Reporter: antlam, Assigned: ahunt)

References

Details

Attachments

(6 files, 2 obsolete files)

Attached image prev_firsttime_feedback1.png (obsolete) —
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
^ re-uses our helper UI styles from https://bugzilla.mozilla.org/show_bug.cgi?id=1217174#c63

Will attach the graphic next.
Note: Copy TBD
Copy finalized.
Attachment #8716418 - Attachment is obsolete: true
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
Blocks: 1266182
Blocks: 1247689
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/
Attachment #8743717 - Flags: review?(s.kaspari)
Attachment #8743718 - Flags: review?(s.kaspari)
Attachment #8743717 - Flags: review?(s.kaspari) → review+
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 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.
Attachment #8743718 - Flags: review?(s.kaspari) → review+
Since this hasn't closed yet, I just want to update this image. The other images will be replaced in bug 1266899.
Attachment #8716500 - Attachment is obsolete: true
Flags: needinfo?(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!
(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.
Flags: needinfo?(ahunt)
https://hg.mozilla.org/mozilla-central/rev/d0bed0273673
https://hg.mozilla.org/mozilla-central/rev/f9177e655c8c
https://hg.mozilla.org/mozilla-central/rev/ecdcf1026a3b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Attached image Screenshot as landed
And here's a screenshot from the version that landed!
Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 48.0a2 (2016-04-28)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.