Closed Bug 1277978 Opened 8 years ago Closed 8 years ago

Only show Recently Closed smartfolder when there are items

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox49 unaffected, fennec50+, firefox51 verified)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox49 --- unaffected
fennec 50+ ---
firefox51 --- verified

People

(Reporter: liuche, Assigned: JanH, NeedInfo)

References

Details

Attachments

(7 files)

Bug 1277978 - Part 0a - Fix typo.
58 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1277978 - Part 0b - Add target api annotation in BrowserApp.
58 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1277978 - Part 1 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown.
58 bytes, text/x-review-board-request
liuche
: review+
rnewman
: review+
Details
Bug 1277978 - Part 2 - Allow saving a cached recent tabs count in BrowserApp.
58 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1277978 - Part 3 - Allow the RecentTabsAdapter to indicate whether the tab count is reliable.
58 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1277978 - Part 4 - Actually cache the recent tabs count when it updates.
58 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1277978 - Part 5 - Once we can access the panelStateChangeListener, use it to retrieve the cached tabs count when initialising the HistoryAdapter.
58 bytes, text/x-review-board-request
liuche
: review+
Details 
In bug 1251362, we added a "Recently closed" smartfolder to history. This bug is for handling showing/hiding that folder depending on if there are items or not in the folder.

Antlam, there's the case where when Firefox starts from a cold start, it's unclear if there are tabs from a previous session, and that information could show up asynchronously. If we add the smartfolder in, then subsequent items can be shifted down.

Let us know what you'd like in this case.
Depends on: 1251362
Flags: needinfo?(alam)
Bug 1251362 missed the merge, so this would only affect 50.
tracking-fennec: --- → ?
status-firefox49: affected → unaffected
JanH is away until next week, but so is liuche. One of you two should deal with this when you're back :)
Assignee: nobody → jh+bugzilla
tracking-fennec: ? → 50+
Actually I think a cold start is less of a problem - where this is really noticeable is when the history panel is the currently active panel when entering the homepager - in that case you get the animation each time the homepager shows, because the history panel is shown almost immediately and the round trip through Gecko usually takes slightly longer even on a good day (at least on my phone)

In any case I'll try and see whether ahunt's panel data storage (bug 1060544) could be used to cache the number of recently closed tabs - somebody quick-fingered could still enter the folder and end up looking at the empty state until the closed tabs actually arrive, but at least we'd avoid that animation in the main panel that way.
Hmm no, that panel state is per tab, so not really suitable, but something similar should be possible.
Depends on: 1296411
Comment on attachment 8786472 [details]
Bug 1277978 - Part 0a - Fix typo.

https://reviewboard.mozilla.org/r/75390/#review74586
Attachment #8786472 - Flags: review?(liuche) → review+
Comment on attachment 8786473 [details]
Bug 1277978 - Part 0b - Add target api annotation in BrowserApp.

https://reviewboard.mozilla.org/r/75392/#review74588
Attachment #8786473 - Flags: review?(liuche) → review+
Comment on attachment 8786474 [details]
Bug 1277978 - Part 1 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown.

https://reviewboard.mozilla.org/r/75394/#review74590

Great - thanks for working on this and being meticulous about the states!

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:116
(Diff revision 1)
> +
> +                                if (isRecentTabsFolderVisible()) {
> +                                    notifyItemInserted(RECENT_TABS_SMARTFOLDER_INDEX);
> +                                    // If the list exceeds the display height and we want to show the new
> +                                    // item inserted at position 0, we need to scroll up manually
> +                                    // (see https://code.google.com/p/android/issues/detail?id=174227#c2).

Good find, thanks for making a note.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:120
(Diff revision 1)
> +                                    // item inserted at position 0, we need to scroll up manually
> +                                    // (see https://code.google.com/p/android/issues/detail?id=174227#c2).
> +                                    // However we only do this if our current scroll position is at the
> +                                    // top of the list.
> +                                    if (linearLayoutManager != null &&
> +                                            linearLayoutManager.findFirstCompletelyVisibleItemPosition() == 0) {

Does the ordering of notify/scroll matter? as in, when does FirstCompletelyVisibleItemPosition resolve? If it doesn't matter, optionally add a comment about our assumption on what is the 0th position item.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:171
(Diff revision 1)
>          animator.setAddDuration(100);
>          animator.setChangeDuration(100);
>          animator.setMoveDuration(100);
>          animator.setRemoveDuration(100);
>          mRecyclerView.setLayoutManager(new LinearLayoutManager(getContext()));
> +        mHistoryAdapter.setLinearLayoutManager((LinearLayoutManager) mRecyclerView.getLayoutManager());

I was wondering if this LinearLayoutManager can ever get stale - but I guess that unless the RecyclerView gets destroyed separate from the adapter and does not get set again, this should be okay.
Attachment #8786474 - Flags: review?(liuche) → review+
I need to run and want to read the caching code more carefully, but I will try to finish this over the weekend!
Comment on attachment 8786474 [details]
Bug 1277978 - Part 1 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown.

https://reviewboard.mozilla.org/r/75394/#review74590

> Does the ordering of notify/scroll matter? as in, when does FirstCompletelyVisibleItemPosition resolve? If it doesn't matter, optionally add a comment about our assumption on what is the 0th position item.

Good point - I might have wondered about this myself, but then this patch was temporarily shelved and it's been quite a while since then. The documentation says that "This position does not include adapter changes that were dispatched after the last layout pass." and I suppose that with the current recycler view implementation, querying the position directly after calling `notifyItemInserted` is soon enough to happen before the next layout pass.
However capturing the current position before calling `notifyItemInserted` would definitively do a better job of reflecting normal temporal causality, so I'll go with that instead.
Comment on attachment 8786474 [details]
Bug 1277978 - Part 1 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown.

https://reviewboard.mozilla.org/r/75394/#review74644

I saw this fly by in my bugmail, and figured it would be worth looking at.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:47
(Diff revision 2)
>      private DevicesUpdateHandler devicesUpdateHandler;
>      private int deviceCount = 0;
>      private RecentTabsUpdateHandler recentTabsUpdateHandler;
>      private int recentTabsCount = 0;
>  
> +    private LinearLayoutManager linearLayoutManager;

At the very least this should be volatile: it's a member that's accessed from at least one thread. If interacting with this implies interacting with, e.g., `sectionHeaders`, then you might need a more sophisticated concurrency approach.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:50
(Diff revision 2)
>      private int recentTabsCount = 0;
>  
> +    private LinearLayoutManager linearLayoutManager;
> +
>      // We use a sparse array to store each section header's position in the panel [more cheaply than a HashMap].
>      private final SparseArray<SectionHeader> sectionHeaders;

I don't think `SparseArray` is thread-safe, but you populate this when handling a cursor, or when refreshing the view, and it's used when rendering. You should re-examine this from a thread-safety perspective.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:114
(Diff revision 2)
> +                                // we need to recalculate the history section header positions.
> +                                populateSectionHeaders(historyCursor, sectionHeaders);
> +
> +                                if (isRecentTabsFolderVisible()) {
> +                                    int scrollPos = -1;
> +                                    if (linearLayoutManager != null) {

Capture this once, rather than accessing it multiple times during this method.
Comment on attachment 8786474 [details]
Bug 1277978 - Part 1 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown.

https://reviewboard.mozilla.org/r/75394/#review74644

Thanks for taking the time.

> At the very least this should be volatile: it's a member that's accessed from at least one thread. If interacting with this implies interacting with, e.g., `sectionHeaders`, then you might need a more sophisticated concurrency approach.

`setLinearLayoutManager()` is ultimately called from `combinedHistoryPanel.onViewCreated()` which runs on the UI thread. Within the adapter, the layout manager is then only ever used from the UI thread as well, so in the end we should be safe, though?

> I don't think `SparseArray` is thread-safe, but you populate this when handling a cursor, or when refreshing the view, and it's used when rendering. You should re-examine this from a thread-safety perspective.

You're right in pointing this out (and a `SparseArray` is indeed not thread-safe), but again, as far as I can tell, the exisiting calls to `populateSectionHeaders()` all run on the UI thread [1], so my `RecentTabsUpdateHandler` doesn't add anything new into the mix because all of its logic explicitly runs on the UI thread, too.

[1] Via `setHistory()` and ultimately originating from either `onLoaderReset()` or `onLoadFinished()` in the history panel.
Comment on attachment 8786475 [details]
Bug 1277978 - Part 2 - Allow saving a cached recent tabs count in BrowserApp.

https://reviewboard.mozilla.org/r/75396/#review75628
Attachment #8786475 - Flags: review?(liuche) → review+
Comment on attachment 8786476 [details]
Bug 1277978 - Part 3 - Allow the RecentTabsAdapter to indicate whether the tab count is reliable.

https://reviewboard.mozilla.org/r/75398/#review75636
Attachment #8786476 - Flags: review?(liuche) → review+
Comment on attachment 8786477 [details]
Bug 1277978 - Part 4 - Actually cache the recent tabs count when it updates.

https://reviewboard.mozilla.org/r/75400/#review75650
Attachment #8786477 - Flags: review?(liuche) → review+
Comment on attachment 8786476 [details]
Bug 1277978 - Part 3 - Allow the RecentTabsAdapter to indicate whether the tab count is reliable.

https://reviewboard.mozilla.org/r/75398/#review75702

::: mobile/android/base/java/org/mozilla/gecko/home/RecentTabsAdapter.java:133
(Diff revision 3)
>                  int prevClosedTabsCount = recentlyClosedTabs.length;
>                  boolean prevSectionHeaderVisibility = isSectionHeaderVisible();
>                  int prevSectionHeaderIndex = getSectionHeaderIndex();
>  
>                  recentlyClosedTabs = closedTabs;
> -                recentTabsUpdateHandler.onRecentTabsCountUpdated(getClosedTabsCount());
> +                recentlyClosedTabsReceived = true;

I'm wondering whether recentlyClosedTabsReceived should ever be set to false, i.e., does the count get out of date and therefore should not get cached?
Comment on attachment 8786478 [details]
Bug 1277978 - Part 5 - Once we can access the panelStateChangeListener, use it to retrieve the cached tabs count when initialising the HistoryAdapter.

https://reviewboard.mozilla.org/r/75402/#review75706

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:111
(Diff revision 3)
>                      // Recent Tabs folder, only update the recentTabsCount on the UI thread.
>                      ThreadUtils.postToUiThread(new Runnable() {
>                          @UiThread
>                          @Override
>                          public void run() {
> +                            if (!countReliable && count <= recentTabsCount) {

Why count <= recentTabsCount? Document the assumption in a comment here. It looks like to me that we're comparing the recentlyClosedTabs/lastSessionTabs arrays to the value we cached in BrowserApp, from the last time countReliable == true, but it's not clear to me what case this is covering.
Comment on attachment 8786478 [details]
Bug 1277978 - Part 5 - Once we can access the panelStateChangeListener, use it to retrieve the cached tabs count when initialising the HistoryAdapter.

https://reviewboard.mozilla.org/r/75402/#review75706

> Why count <= recentTabsCount? Document the assumption in a comment here. It looks like to me that we're comparing the recentlyClosedTabs/lastSessionTabs arrays to the value we cached in BrowserApp, from the last time countReliable == true, but it's not clear to me what case this is covering.

One example that is probably the tyical scenario if your tab restore setting is *Always restore*: Once the RecentTabsAdapter intialises, it immediately looks for the *Tabs from last time* (sessionstore.old). If there aren't any, it calls `onRecentTabsCountUpdated()` with a count of 0 (and `countReliable` = false). So if the HistoryAdapter already had a (cached) recentTabsCount > 0 (meaning the panel intialises with the folder visible), that would mean that the count would temporarily be reset to 0 (folder gets hidden), only to jump back up to its actual value once the session store finally sends us its closed tabs (folder gets shown again). So to avoid all that back and forth, we only revise the number of closed tabs upwards as long the count isn't final. I'll add a comment to that effect.
Comment on attachment 8786476 [details]
Bug 1277978 - Part 3 - Allow the RecentTabsAdapter to indicate whether the tab count is reliable.

https://reviewboard.mozilla.org/r/75398/#review75702

> I'm wondering whether recentlyClosedTabsReceived should ever be set to false, i.e., does the count get out of date and therefore should not get cached?

Maybe technically once we stop listening for closed tabs, although in practice this only happens when the home panels are discarded and the adapter is going to be destroyed. I've added it just to be on the safe side, though.
Comment on attachment 8786474 [details]
Bug 1277978 - Part 1 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown.

https://reviewboard.mozilla.org/r/75394/#review76440

I scattered some UiThread notes throughout; I don't have this open in my IDE, so I haven't checked whether the anno already crawls up through the call hierarchy in all of these places.

Consider adding ThreadUtils assertions anywhere you want to be really sure of your thread safety.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:81
(Diff revision 4)
>          if (devicesUpdateHandler == null) {
>              devicesUpdateHandler = new DevicesUpdateHandler() {
>                  @Override
>                  public void onDeviceCountUpdated(int count) {
>                      deviceCount = count;
> -                    notifyItemChanged(SYNCED_DEVICES_SMARTFOLDER_INDEX);
> +                    notifyItemChanged(getSyncedDevicesSmartfolderIndex());

Nit: Smartfolder -> SmartFolder

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:93
(Diff revision 4)
>      public interface RecentTabsUpdateHandler {
>          void onRecentTabsCountUpdated(int count);
>      }
>  
>      public RecentTabsUpdateHandler getRecentTabsUpdateHandler() {
>          if (recentTabsUpdateHandler == null) {

Nit: consider reducing some of the nesting here by inverting conditionals and using early returns:

```
if (recentTabsUpdateHandler != null) {
    return recentTabsUpdateHandler;
}
recentTabsUpdateHandler = new Recen…
```

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:109
(Diff revision 4)
> -                    recentTabsCount = count;
> +                            recentTabsCount = count;
> +                            final boolean folderVisible = isRecentTabsFolderVisible();
> +
> +                            if (prevFolderVisibility == folderVisible) {
> +                                if (prevFolderVisibility) {
> -                    notifyItemChanged(RECENT_TABS_SMARTFOLDER_INDEX);
> +                                    notifyItemChanged(RECENT_TABS_SMARTFOLDER_INDEX);

`getSyncedDevicesSmartFolderIndex` would notify `RECENT_TABS_SMARTFOLDER_INDEX + 1` here. Bug?

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:143
(Diff revision 4)
>              };
>          }
>          return recentTabsUpdateHandler;
>      }
>  
> +    private boolean isRecentTabsFolderVisible() {

@UiThread

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:148
(Diff revision 4)
> +    private boolean isRecentTabsFolderVisible() {
> +        return recentTabsCount > 0;
> +    }
> +
> +    // Number of smart folders for determining practical empty state.
> +    public int getNumVisibleSmartFolders() {

@UiThread

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:158
(Diff revision 4)
> +        }
> +
> +        return visibleFolders;
> +    }
> +
> +    private int getSyncedDevicesSmartfolderIndex() {

@UiThread

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:226
(Diff revision 4)
>       *
>       * @param type ItemType of the item
>       * @param position position in the adapter
>       * @return position of the item in the data structure
>       */
>      private int transformAdapterPositionForDataStructure(CombinedHistoryItem.ItemType type, int position) {

@UiThread

(All of these reach all the way down to `recentTabsCount`, which you note as only being accessible from the UI thread.)

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:236
(Diff revision 4)
>          } else {
>              return position;
>          }
>      }
>  
>      private CombinedHistoryItem.ItemType getItemTypeForPosition(int position) {

Same.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:251
(Diff revision 4)
>          }
>          return CombinedHistoryItem.ItemType.HISTORY;
>      }
>  
>      @Override
>      public int getItemViewType(int position) {

Same.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:256
(Diff revision 4)
>      public int getItemViewType(int position) {
>          return CombinedHistoryItem.ItemType.itemTypeToViewType(getItemTypeForPosition(position));
>      }
>  
>      @Override
>      public int getItemCount() {

Same.
Attachment #8786474 - Flags: review?(rnewman) → review+
Comment on attachment 8786478 [details]
Bug 1277978 - Part 5 - Once we can access the panelStateChangeListener, use it to retrieve the cached tabs count when initialising the HistoryAdapter.

https://reviewboard.mozilla.org/r/75402/#review76462
Attachment #8786478 - Flags: review?(liuche) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c5c0de78a445
Part 0a - Fix typo. r=liuche
https://hg.mozilla.org/integration/autoland/rev/2329e6466009
Part 0b - Add target api annotation in BrowserApp. r=liuche
https://hg.mozilla.org/integration/autoland/rev/a93820dea9ba
Part 1 - Hide Recent Tabs smart folder if there aren't any closed tabs to be shown. r=liuche,rnewman
https://hg.mozilla.org/integration/autoland/rev/1ab243749c5e
Part 2 - Allow saving a cached recent tabs count in BrowserApp. r=liuche
https://hg.mozilla.org/integration/autoland/rev/434dfdb24918
Part 3 - Allow the RecentTabsAdapter to indicate whether the tab count is reliable. r=liuche
https://hg.mozilla.org/integration/autoland/rev/11658ac8e339
Part 4 - Actually cache the recent tabs count when it updates. r=liuche
https://hg.mozilla.org/integration/autoland/rev/31471e296123
Part 5 - Once we can access the panelStateChangeListener, use it to retrieve the cached tabs count when initialising the HistoryAdapter. r=liuche
Keywords: checkin-needed
Depends on: 1302424
Blocks: 1303394
Tested using:
Device: One A2001 (Android 6.0.1)
Build: Firefox for Android 51.0a2 (2016-09-21)

On a clean profile, 'Recently Closed' smartfolder is not displayed. If you load a page and close it, then the folder is displayed
status-firefox51: 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: