Closed Bug 781262 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, enhancement)

ARM
Android
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 17

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(1 file, 6 obsolete files)

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).
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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?
Attachment #651033 - Flags: review?(sriram)
Attached patch Patch v2 (obsolete) — Splinter Review
As discussed in person, made a single instantiation of AboutHomePromoBox and now swap layout content with a "show" method.
Attachment #651033 - Attachment is obsolete: true
Attachment #651033 - Flags: review?(sriram)
Attachment #651078 - Flags: review?(sriram)
Attached patch Patch v3 (obsolete) — Splinter Review
Cleaned up a few unnecessary things.
Attachment #651078 - Attachment is obsolete: true
Attachment #651078 - Flags: review?(sriram)
Attachment #651079 - Flags: review?(sriram)
Attachment #651079 - Attachment is patch: true
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?
Attached patch Patch v4 (obsolete) — Splinter Review
I should read code more thoroughly before I copy-pasta. :( Fixed hard-coded string size.
Attachment #651079 - Attachment is obsolete: true
Attachment #651079 - Flags: review?(sriram)
Attachment #651083 - Flags: review?(sriram)
Attached file Patch v5 (obsolete) —
Forgot "static final" modifier on LOGTAG.
Attachment #651083 - Attachment is obsolete: true
Attachment #651083 - Flags: review?(sriram)
Attachment #651084 - Flags: review?(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+.
Attached patch Patch v6 (obsolete) — Splinter Review
> 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.
Attachment #651084 - Attachment is obsolete: true
Attachment #651084 - Flags: review?(sriram)
Attachment #651569 - Flags: review?(sriram)
Comment on attachment 651569 [details] [diff] [review]
Patch v6

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

Looks good to me.
Attachment #651569 - Flags: review?(sriram) → review+
Attached patch Patch v7Splinter Review
Moved r+. Rebase to trunk.
Attachment #651569 - Attachment is obsolete: true
Attachment #653568 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fb8c41ccafed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.