Closed
Bug 1386735
Opened 7 years ago
Closed 7 years ago
Add settings for toggling Activity Stream
Categories
(Firefox for Android Graveyard :: General, enhancement, P1)
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).
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(abenson)
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Comment 1•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Priority: P2 → --
Assignee | ||
Updated•7 years ago
|
tracking-fennec: --- → ?
Updated•7 years ago
|
Blocks: as-android-blockers
Priority: -- → P2
Updated•7 years ago
|
tracking-fennec: ? → +
Assignee | ||
Updated•7 years ago
|
Rank: 1
Assignee | ||
Comment 3•7 years ago
|
||
Since bug 1380808 landed this is no longer blocked.
Assignee | ||
Comment 4•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → liuche
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8902911 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
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)
Comment 7•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
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.
Assignee | ||
Comment 11•7 years ago
|
||
Comment 12•7 years ago
|
||
mozreview-review |
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+
Comment 13•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
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
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a2869adf00c5
https://hg.mozilla.org/mozilla-central/rev/3bd4bc1b1a04
https://hg.mozilla.org/mozilla-central/rev/e5a2005e7534
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 21•7 years ago
|
||
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)
Assignee | ||
Comment 22•7 years ago
|
||
Thanks Sebastian! Opened bug 1396407 and landed a fix for it.
Flags: needinfo?(liuche)
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
Comment 23•7 years ago
|
||
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)
Assignee | ||
Comment 24•7 years ago
|
||
> - 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...
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(liuche)
Comment 25•7 years ago
|
||
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
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•