Closed Bug 1395792 Opened 7 years ago Closed 7 years ago

Follow-up: Refresh sections in newtab immediately when settings change

Categories

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

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
enhancement

Tracking

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

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

People

(Reporter: liuche, Assigned: liuche)

References

Details

(Whiteboard: [MobileAS])

Attachments

(4 files)

Bug 1395792 - Add listener for AS SharedPreferences.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1395792 - Add show/hide to onBindVH.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1395792 - Destroy loaders on reload.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1395792 - Hide Highlights empty state when Highlights are disabled.
59 bytes, text/x-review-board-request
mcomella
: review+
Details 
Splitting this off from bug 1386735. If the newtab page is open, we don't refresh the sections even if the settings are changed until creating a new newtab. Might try refreshing in onStart or through listeners to the prefs.
Summary: Refresh sections in newtab immediately when settings change → Follow-up: Refresh sections in newtab immediately when settings change
Priority: -- → P1
Assignee: nobody → liuche
Iteration: 1.29 → 1.30
Moving this to P2 because of update on merge schedule.
Rank: 2
Priority: P1 → P2
Iteration: 1.30 → ---
Iteration: --- → 1.30
Rank: 2 → 1
Priority: P2 → P1
wrt triage: Maria finds this jarring so it'd be great to fix it (even if not in the most correct way) before we land in 57.
tracking-fennec: --- → +
Rank: 1
Comment on attachment 8909593 [details]
Bug 1395792 - Add listener for AS SharedPreferences.

https://reviewboard.mozilla.org/r/181064/#review186676

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamHomeFragment.java:38
(Diff revision 1)
>  
>      @Override
> +    public void onAttach(Activity activity) {
> +        super.onAttach(activity);
> +        context = activity.getApplicationContext();
> +        final SharedPreferences prefs = GeckoSharedPrefs.forProfile(context);

nit: can we keep a reference to prefs so we don't have to keep a reference to Context?

Also, I think there's a `Fragment.getContext` method (though that may have been added later and I don't know if we're using the support library here)

If we do keep the reference, we should change the name to `applicationContext` to make it clear.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/ActivityStreamHomeFragment.java:59
(Diff revision 1)
> +
> +        if (shouldReload) {
> +            activityStreamPanel.reload(getLoaderManager());
> +        }
> +
> +    }

nit: ws above and non below
Attachment #8909593 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8909594 [details]
Bug 1395792 - Add show/hide to onBindVH.

https://reviewboard.mozilla.org/r/181066/#review186702

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:103
(Diff revision 1)
>              recyclerViewModel.add(makeRowModelFromType(type));
>          }
>          topStoriesQueue = Collections.emptyList();
>      }
>  
>      public void reset() {

nit: the code from this method is largely duplicated in the constructor: we should put it in a helper method. This will help ensure this code works and is tested in cases other than reset (which is fairly uncommon).

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:187
(Diff revision 1)
>          } else if (type == RowItemType.TOP_STORIES_ITEM.getViewType()) {
>              final TopStory story = (TopStory) recyclerViewModel.get(position);
>              ((WebpageItemRow) holder).bind(story, position, tilesSize);
> +        } else if (type == RowItemType.HIGHLIGHTS_TITLE.getViewType()) {
> +            final StreamTitleRow streamTitleRow = (StreamTitleRow) holder;
> +            final SharedPreferences sharedPreferences = GeckoSharedPrefs.forProfile(streamTitleRow.context);

You can get the context from `holder.itemView.getContext()`, here and below

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamTitleRow.java:29
(Diff revision 1)
> +
>      public static final int LAYOUT_ID = R.layout.activity_stream_main_highlightstitle;
>  
> -    public StreamTitleRow(final View itemView, final @StringRes @NonNull int titleResId, boolean isEnabled) {
> +    public StreamTitleRow(final View itemView, final @StringRes @NonNull int titleResId) {
>          super(itemView);
> +        context = itemView.getContext();

As per the above, no need to keep a reference to context here.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamTitleRow.java:62
(Diff revision 1)
> -    private static void hideView(final View itemView) {
> -        itemView.setVisibility(View.GONE);
> -        // We also need to set the layout height, width, and margins to 0 for the RecyclerView child.
> +    public void setVisible(boolean toShow) {
> +        itemView.setVisibility(toShow ? View.VISIBLE : View.GONE);
> +        // We also need to set the layout height and width to 0 for the RecyclerView child.
>          final RecyclerView.LayoutParams layoutParams = (RecyclerView.LayoutParams) itemView.getLayoutParams();
> -        layoutParams.setMargins(0, 0, 0, 0);
> +        if (toShow) {
> +            layoutParams.height = RecyclerView.LayoutParams.WRAP_CONTENT;

nit: add a comment to the XML that you'll always override the layout_height/width in code (and maybe mention this is a hack and it'd be better to add/remove items from the recycler view).
Attachment #8909594 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8909595 [details]
Bug 1395792 - Destroy loaders on reload.

https://reviewboard.mozilla.org/r/181068/#review186708

As we spoke about IRL, this patch doesn't seem necessary - let me know if you find something out that changes your mind on that.
Attachment #8909595 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8909594 [details]
Bug 1395792 - Add show/hide to onBindVH.

https://reviewboard.mozilla.org/r/181066/#review186712

Also, while this will hide the views, I think we also need to take care of cancelling the loader (or exiting early from the loader's load methods) to avoid doing unnecessary work (or hitting the network!).
Comment on attachment 8909594 [details]
Bug 1395792 - Add show/hide to onBindVH.

https://reviewboard.mozilla.org/r/181066/#review186770

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:103
(Diff revision 1)
>              recyclerViewModel.add(makeRowModelFromType(type));
>          }
>          topStoriesQueue = Collections.emptyList();
>      }
>  
>      public void reset() {

Okay - I renamed this to clearAndInit.
Comment on attachment 8909595 [details]
Bug 1395792 - Destroy loaders on reload.

https://reviewboard.mozilla.org/r/181068/#review187120

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:188
(Diff revision 2)
>              ((WebpageItemRow) holder).bind(story, position, tilesSize);
>          } else if (type == RowItemType.HIGHLIGHTS_TITLE.getViewType()) {
> -            final SharedPreferences sharedPreferences = GeckoSharedPrefs.forProfile(holder.itemView.getContext());
> -            final boolean bookmarksEnabled = sharedPreferences.getBoolean(ActivityStreamPanel.PREF_BOOKMARKS_ENABLED, true);
> -            final boolean visitedEnabled = sharedPreferences.getBoolean(ActivityStreamPanel.PREF_VISITED_ENABLED, true);
> +            final Context context = holder.itemView.getContext();
> +            final SharedPreferences sharedPreferences = GeckoSharedPrefs.forProfile(context);
> +            final boolean bookmarksEnabled = sharedPreferences.getBoolean(ActivityStreamPanel.PREF_BOOKMARKS_ENABLED,
> +                    context.getResources().getBoolean(R.bool.pref_activitystream_recentbookmarks_enabled_default));

nit: I wonder if it'd be better to have a static method `getIsASBookmarksEnabled` so we:
1. Don't need a constant defined in XML resources
2. Don't need to duplicate this verbose code (because it's called elsewhere, right?)

That being said, no need to change this now if this works.
Attachment #8909595 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8910060 [details]
Bug 1395792 - Hide Highlights empty state when Highlights are disabled.

https://reviewboard.mozilla.org/r/181532/#review187124

Cool, I like that the StreamAdapter owns setting its children's visibility and layout params.

::: mobile/android/app/src/main/res/layout/activity_stream_highlights_empty_state.xml:13
(Diff revision 1)
> +
> +     NB: This is a hack because the adapter does not know whether this view should be enabled,
> +     which requires a check to SharedPreferences, which requires a Context, so we do it in
> +     onBindViewHolder.
> +
> +     A more correct implementation would dynamically add/remove these title views rather than

nit: I think this, "Why do we do this hack" comment should be in a central location used by all layouts (`setViewVisible`) and each of these layouts should say, "see StreamAdapter.setViewVisible for details on this hack" or similar.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:206
(Diff revision 1)
> +    private static void setViewVisible(boolean toShow, final View view) {
> +        view.setVisibility(toShow ? View.VISIBLE : View.GONE);
> +        // We also need to set the layout height and width to 0 for the RecyclerView child.
> +        final RecyclerView.LayoutParams layoutParams = (RecyclerView.LayoutParams) view.getLayoutParams();
> +        if (toShow) {
> +            layoutParams.height = RecyclerView.LayoutParams.WRAP_CONTENT;

nit: are we sure all the views that will be called on this method are WRAP_CONTENT & MATCH_PARENT? Add a comment how we know that. (i.e. make it obvious to someone who calls `setViewVisible`, but may not read the code (or comments!), about this behavior)
Attachment #8910060 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8909595 [details]
Bug 1395792 - Destroy loaders on reload.

https://reviewboard.mozilla.org/r/181068/#review187120

> nit: I wonder if it'd be better to have a static method `getIsASBookmarksEnabled` so we:
> 1. Don't need a constant defined in XML resources
> 2. Don't need to duplicate this verbose code (because it's called elsewhere, right?)
> 
> That being said, no need to change this now if this works.

lol, I actually had that as a method, but decided that since it was only used in one place it didn't need to be a method :P But you're right that this code is used elsewhere outside of this file. I'll leave it as is for now.
Pushed by cliu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a9fda296c1a
Add listener for AS SharedPreferences. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/b5b59c79c04d
Add show/hide to onBindVH. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/ed817e1a17ba
Destroy loaders on reload. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/072a236f67fe
Hide Highlights empty state when Highlights are disabled. r=mcomella
Verified as in beta. Top Sites properly refresh
Status: RESOLVED → VERIFIED
status-firefox57: fixed → 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.

Attachment

General

Creator:
Created:
Updated:
Size: