Closed Bug 1386735 Opened 4 years ago Closed 4 years ago

Add settings for toggling Activity Stream

Categories

(Firefox for Android Graveyard :: General, enhancement, P1)

ARM
Android
enhancement

Tracking

(fennec+, firefox55 unaffected, firefox56 unaffected, firefox57 verified)

VERIFIED FIXED
Firefox 57
Tracking Status
fennec + ---
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified

People

(Reporter: liuche, Assigned: liuche)

References

Details

(Whiteboard: [MobileAS])

Attachments

(4 files, 1 obsolete file)

Aaron Benson has made some mocks for adding settings to finely control the pieces of Activity Stream (Highlights, Pocket, Top Sites).
Flags: needinfo?(abenson)
Priority: -- → P2
Here's the InVision mocks for changing view settings on the Top Sites panel which includes turning on/off Pocket stories and adjusting what shows up in Highlights: https://mozilla.invisionapp.com/share/B5CVCBOP3#/screens/246811560_Settings_-_Home
Flags: needinfo?(abenson)
Priority: P2 → --
tracking-fennec: --- → ?
Priority: -- → P2
tracking-fennec: ? → +
Chenxia's changes will conflict with this.
Depends on: 1380808
Rank: 1
Since bug 1380808 landed this is no longer blocked.
I'm going to bump this up to the current sprint because this could be a more complex change that causes regressions (it's easy to get RecyclerView crashes when messing with the model) so I want to give this time to bake.
Iteration: --- → 1.29
Priority: P2 → P1
Assignee: nobody → liuche
Attached file Mock: Settings for newtab sections (obsolete) —
Attachment #8902911 - Attachment is obsolete: true
Aaron, is the name of the subsection definitely "Additional Content"? It feels a bit formal/long to me. bbell mentioned this is the place where we might in the future include Screenshots, Sent to device, etc, so maybe something like "Extras", "Extra Content",  "Best of", or "Personalized"? Also fine if you think we should keep that string.
Flags: needinfo?(abenson)
I think it reads correctly in the panel since it is "additional content" to the Top Sites section. It's also generic enough to take on more things in the future. I say we go with it.
Flags: needinfo?(abenson)
Blocks: 1395792
One more patch, to actually add the filtering of history/bookmarks from highlights based on the settings.

Also filed bug 1395792 to refresh activity stream when the prefs change.
Comment on attachment 8903420 [details]
Bug 1386735 - Support disabling titles in StreamRecyclerView.

https://reviewboard.mozilla.org/r/175260/#review180572

For my empty state, I don't hide the view - I just don't add it to the model. We should be consistent so maybe I'll rewrite it to do this. I don't have strong opinions on which is better but it'll be easy for me to change mine.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamTitleRow.java:34
(Diff revision 1)
> +        itemView.setVisibility(View.GONE);
> +        final RecyclerView.LayoutParams layoutParams = (RecyclerView.LayoutParams) itemView.getLayoutParams();
> +        layoutParams.setMargins(0, 0, 0, 0);
> +        layoutParams.height = 0;
> +        layoutParams.width = 0;
> +        itemView.setLayoutParams(layoutParams);

Why are layout parameters necessary if we change visibility? aAdd a comment.
Attachment #8903420 - Flags: review?(michael.l.comella) → review+
Blocks: 1396054
Comment on attachment 8903421 [details]
Bug 1386735 - Add additional preferences to Top Sites settings.

https://reviewboard.mozilla.org/r/175262/#review180628

Pretty clean - lgtm. As per my comments, I'd be surprised if we need the `SwitchPreferenceView` though.

::: mobile/android/app/src/main/res/layout/preference_switch.xml:17
(Diff revision 1)
> +              android:layout_width="wrap_content"
> +              android:layout_height="wrap_content"
> +              android:gravity="center_vertical"
> +              android:layout_weight="1"
> +              android:paddingStart="24dp"
> +              android:paddingLeft="24dp"

You need to explicitly specify paddingRight/End as 0dp.

::: mobile/android/app/src/main/res/layout/preference_switch.xml:22
(Diff revision 1)
> +              android:paddingLeft="24dp"
> +              android:paddingBottom="24dp"
> +              android:textSize="16sp"
> +              android:textColor="@android:color/black"/>
> +
> +    <android.support.v7.widget.SwitchCompat

Why can't you use the `text/textOn/textOff` properties instead of creating a new layout with a switch and a TextView? Add a comment.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamPanel.java:61
(Diff revision 1)
> +    public static final String PREF_VISITED_ENABLED = "pref_activitystream_visited_enabled";
> +    public static final String PREF_BOOKMARKS_ENABLED = "pref_activitystream_recentbookmarks_enabled";
> +
>      private int desiredTileWidth;
>      private int tileMargin;
> +    final SharedPreferences sharedPreferences;

nit: private?
Attachment #8903421 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8903815 [details]
Bug 1386735 - Rename FIXED_ROWS to clarify that they are Activity Stream sections.

https://reviewboard.mozilla.org/r/175570/#review180656

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:57
(Diff revision 1)
>      private Cursor topSitesCursor;
>      private List<RowModel> recyclerViewModel; // List of item types backing this RecyclerView.
>      private List<TopStory> topStoriesQueue;
>  
> -    private final RowItemType[] FIXED_ROWS = {RowItemType.TOP_PANEL, RowItemType.WELCOME, RowItemType.TOP_STORIES_TITLE, RowItemType.HIGHLIGHTS_TITLE};
> +    // Content sections available on the ActivityStream page. These may be hidden if the sections are disabled.
> +    private final RowItemType[] ACTIVITYSTREAM_SECTIONS = { RowItemType.TOP_PANEL, RowItemType.WELCOME, RowItemType.TOP_STORIES_TITLE, RowItemType.HIGHLIGHTS_TITLE };

nit: ACTIVITY_STREAM_SECTIONS
Attachment #8903815 - Flags: review?(michael.l.comella) → review+
Pushed by cliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a2869adf00c5
Support disabling titles in StreamRecyclerView. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/3bd4bc1b1a04
Add additional preferences to Top Sites settings. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/e5a2005e7534
Rename FIXED_ROWS to clarify that they are Activity Stream sections. r=mcomella
This lets the tier 2 Android lint job permafail: https://treeherder.mozilla.org/logviewer.html#?job_id=127877414&repo=autoland
Please fix it.
Flags: needinfo?(liuche)
Blocks: 1396407
Thanks Sebastian! Opened bug 1396407 and landed a fix for it.
Flags: needinfo?(liuche)
Trying this out on Nightly (yay!), two things to note:
- Recommendations don't disappear on an already-opened AS page after being disabled
- The "Additional content" section is _very_ hard to find. After failing to find it on Nightly, I was sure it didn't land yet, went to this bug, saw that it _did_ land, and had to check out the attached screenshot to figure out where it actually is.

Since there's a "default" sub-label under Top Sites (in General->Home), my assumption was that tapping on that row will let me change the default status, or maybe the order. There's absolutely no affordance in place to indicate that something as important as customizing AS's content might be hiding under that item.

IMO we should re-think how these menus are structured if we actually want people to find these toggles.
Flags: needinfo?(liuche)
Blocks: 1396983
> - Recommendations don't disappear on an already-opened AS page after being
> disabled

I filed bug 1395792 already as a follow-up and it's in this sprint, but I'll block it on this bug for tracing reasons.

> - The "Additional content" section is _very_ hard to find. After failing to
> find it on Nightly, I was sure it didn't land yet, went to this bug, saw
> that it _did_ land, and had to check out the attached screenshot to figure
> out where it actually is.

Just filed bug 1396983 for this. FWIW, from talking to UX, the goal wasn't to make it easy to find ways to turn off AS...
Flags: needinfo?(liuche)
Depends on: 1379793
Options to control top sites section have been added and currently work after doing a refresh to the section this will be handled in bug 1395792. Marking this as verified.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.