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

Status: VERIFIED FIXED

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

Description

[Chenxia Liu [:liuche]]

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.

Comment 2

[Chenxia Liu [:liuche]]

Moving this to P2 because of update on merge schedule.

Comment 3

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

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.

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Bug 1395792 - Add listener for AS SharedPreferences.

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

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Bug 1395792 - Add show/hide to onBindVH.

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

Comment 6

[Chenxia Liu [:liuche]]

Attachment: Bug 1395792 - Destroy loaders on reload.

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

Comment 7

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

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

Comment 8

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

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

Comment 9

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

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.

Comment 10

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

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 11

[Chenxia Liu [:liuche]]

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 12

[Chenxia Liu [:liuche]]

Comment on attachment 8909593 [details]
Bug 1395792 - Add listener for AS SharedPreferences.

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

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8909594 [details]
Bug 1395792 - Add show/hide to onBindVH.

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

Comment 14

[Chenxia Liu [:liuche]]

Comment on attachment 8909595 [details]
Bug 1395792 - Destroy loaders on reload.

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

Comment 15

[Chenxia Liu [:liuche]]

Attachment: Bug 1395792 - Hide Highlights empty state when Highlights are disabled.

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

Comment 17

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

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.

Comment 18

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

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)

Comment 19

[Chenxia Liu [:liuche]]

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.

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8909593 [details]
Bug 1395792 - Add listener for AS SharedPreferences.

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

Comment 21

[Chenxia Liu [:liuche]]

Comment on attachment 8909594 [details]
Bug 1395792 - Add show/hide to onBindVH.

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

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 8909595 [details]
Bug 1395792 - Destroy loaders on reload.

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

Comment 23

[Chenxia Liu [:liuche]]

Comment on attachment 8910060 [details]
Bug 1395792 - Hide Highlights empty state when Highlights are disabled.

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

Comment 24

[Pulsebot]

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

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/4a9fda296c1a
https://hg.mozilla.org/mozilla-central/rev/b5b59c79c04d
https://hg.mozilla.org/mozilla-central/rev/ed817e1a17ba
https://hg.mozilla.org/mozilla-central/rev/072a236f67fe

Comment 26

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

Verified as in beta. Top Sites properly refresh