1386735 - Add settings for toggling Activity Stream

Status: VERIFIED FIXED

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

Description

[Chenxia Liu [:liuche]]

Aaron Benson has made some mocks for adding settings to finely control the pieces of Activity Stream (Highlights, Pocket, Top Sites).

Comment 1

[Aaron Benson]

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

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Chenxia's changes will conflict with this.

Comment 3

[Chenxia Liu [:liuche]]

Since bug 1380808 landed this is no longer blocked.

Comment 4

[Chenxia Liu [:liuche]]

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.

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Mock: Settings for newtab sections

Comment 6

[Chenxia Liu [:liuche]]

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.

Comment 7

[Aaron Benson]

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.

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Bug 1386735 - Support disabling titles in StreamRecyclerView.

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

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Bug 1386735 - Add additional preferences to Top Sites settings.

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

Comment 10

[Chenxia Liu [:liuche]]

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 11

[Chenxia Liu [:liuche]]

Attachment: Screenhot: Settings page

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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?

Comment 14

[Chenxia Liu [:liuche]]

Attachment: Bug 1386735 - Rename FIXED_ROWS to clarify that they are Activity Stream sections.

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

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8903420 [details]
Bug 1386735 - Support disabling titles in StreamRecyclerView.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175260/diff/1-2/

Comment 16

[Chenxia Liu [:liuche]]

Comment on attachment 8903421 [details]
Bug 1386735 - Add additional preferences to Top Sites settings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175262/diff/1-2/

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 18

[Chenxia Liu [:liuche]]

Comment on attachment 8903815 [details]
Bug 1386735 - Rename FIXED_ROWS to clarify that they are Activity Stream sections.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175570/diff/1-2/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a2869adf00c5
https://hg.mozilla.org/mozilla-central/rev/3bd4bc1b1a04
https://hg.mozilla.org/mozilla-central/rev/e5a2005e7534

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This lets the tier 2 Android lint job permafail: https://treeherder.mozilla.org/logviewer.html#?job_id=127877414&repo=autoland
Please fix it.

Comment 22

[Chenxia Liu [:liuche]]

Thanks Sebastian! Opened bug 1396407 and landed a fix for it.

Comment 23

[:Grisha Kruglov]

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.

Comment 24

[Chenxia Liu [:liuche]]

> - 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...

Comment 25

[Bogdan Surd, QA [:bsurd, NI]]

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.