781262 - Replace about:home Sync Box with "Promo Box" abstraction

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Allow the about:home "Promo Box" to contain various messages and icons, rather than the current static sync box.

Should contain the ability to rotate snippets based on priority and necessity (ex: you don't need to show the sync box if you already have sync enabled).

Primarily developed to add the ability to swap the sync box with a "My Apps" clickable (bug 766389).

Comment 1

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Patch

Followup patches (or followup bugs) should move the "is sync setup?" code into the AboutHomePromoBox class to better abstract away when each box should show. Additionally, bug 766389 should be implemented for the Apps Marketplace box.

We are in a time crunch to get this into Aurora before it moves to Beta so do you think this code should be in this bug (in another patch) or in a followup bug?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Patch v2

As discussed in person, made a single instantiation of AboutHomePromoBox and now swap layout content with a "show" method.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Patch v3

Cleaned up a few unnecessary things.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Instead of using mAppendArrowToText, it seems Wes just included it in the strings file for the other bug (https://bugzilla.mozilla.org/attachment.cgi?id=650990&action=diff). Should we do something similar here or is it better for internationalization purposes that we continue to add this in code?

Comment 5

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Patch v4

I should read code more thoroughly before I copy-pasta. :( Fixed hard-coded string size.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Patch v5

Forgot "static final" modifier on LOGTAG.

Comment 7

[Sriram Ramasubramanian [:sriram]]

1. Please rename AboutHomePromoBox.Types to AboutHomePromoBox.Type.

2. setPromoBoxVisibility(!syncIsSetup, AboutHomePromoBox.Types.SYNC); <-- we should probably break this.

3. Please rename mCurrentType as mType; mPromoTextView to mTextView, mPromoImageView to mImageView. That conveys the meaning.

4. promo_box_text to "text" and promo_box_icon to "icon". The box's name conveys the meaning. We don't need to have it again and again. Let's keep the id's simple.

5. +            case SYNC:
+                setSyncResources();
+                break;
Whiteline after this.

6.  public boolean hide(Types type) { ... }
This doesn't need an argument. Let's not be strict :P . It's just a hide() and it hides the box that's currently shown. Leave it to the implementer to do what he wants to show.

7.+    private void setResources(int textResource, boolean appendArrowToText, int boldTextResource,
+            boolean useBoldText, int imageResource, int backgroundResource,
+            View.OnClickListener onClickListener) {
+        mTextResource = textResource;
+        mAppendArrowToText = appendArrowToText;
+        mBoldTextResource = boldTextResource;
+        mUseBoldText = useBoldText;
+        mImageResource = imageResource;
+        mBackgroundResource = backgroundResource;
+        mOnClickListener = onClickListener;
+    }

Do we need all these stuff? I am a bit concerned about Localization will work? 'This word is "bold" and this is not' -- Needn't actually translate to other language in a way where "bold" will be in bold, still making a proper sentence. Do you get what I mean?

+        mAppendArrowToText = appendArrowToText; <-- always added. Please remove this, and make it add always. (It's Ian's design language ;) )
+        mUseBoldText = useBoldText; <-- please remove this. If we have a bold text, we show the bold text.
+        mBackgroundResource = backgroundResource; <-- Please remove this. It's always the same. We needn't be this generic. We'll have just one type of box.

8. private void updateTextViewResources() { ... }

This will change a lot. Have it as there "is" a bold resource always, and ">>" are appended always.

9. +    // Types.SYNC: Setup Firefox sync.
+    private void setSyncResources() {
+        setResources(
+                /* mTextResource */ R.string.abouthome_about_sync,
+                /* mAppendArrowToText */ true,
+                /* mBoldTextResource */ R.string.abouthome_sync_bold_name,
+                /* mUseBoldText */ true,
+                /* mImageResource */ R.drawable.abouthome_sync_logo,
+                /* mBackgroundResource */ R.drawable.abouthome_sync_bg,
+                /* mOnClickListener */ new SyncOnClickListener());
+    }

This will reduce a lot. And we don't need comments here. Also, we don't need a click listener specify for this. Have one for the box, and do a switch case based on type.

10. +    private class SyncOnClickListener implements View.OnClickListener {
+        @Override
+        public void onClick(View v) {
+            final Context context = v.getContext();
+            final Intent intent = new Intent(context, SetupSyncActivity.class);
+            context.startActivity(intent);
+        }
+    }

This will be at the box level, and a switch case on the type is better.

Please request review with these changes. Mostly good. I just want to check it once more, before making it r+.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Patch v6

> 2. setPromoBoxVisibility(!syncIsSetup, AboutHomePromoBox.Types.SYNC); <-- we
> should probably break this.
I wasn't sure what you meant by this so I did nothing. Did you mean break it into multiple lines?

I also realized I neglected to include the "pressed" drawables so I updated that and renamed all the appropriate resources.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 651569 [details] [diff] [review]
Patch v6

Review of attachment 651569 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Attachment: Patch v7

Moved r+. Rebase to trunk.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/fb8c41ccafed

Comment 12

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/fb8c41ccafed