Closed Bug 1220928 Opened 4 years ago Closed 4 years ago

Combine Synced Tabs and History home panels

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox45 --- affected
firefox48 --- verified

People

(Reporter: liuche, Assigned: liuche)

References

(Blocks 2 open bugs)

Details

Attachments

(21 files, 6 obsolete files)

176.54 KB, image/png
Details
194.53 KB, image/png
Details
123.98 KB, image/png
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
131.26 KB, image/png
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
172.96 KB, image/png
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
We've been talking about how history in Fennec can be confusing because we have 3 home panels dedicated to tab history - recent tabs, history, and synced tabs. Top Sites could be considered another one, because it's frecency history.

We should simplify our history display to fewer panels, to lighten the cognitive load.

One suggestion would be to have a single tab for Recent tabs, (collapsed) history, (collapsed) synced tabs, so all-things-history are in one panel. If we wanted to further cut down on home panels, we could combine all of these previously mentioned history items under the Top Sites grid view, keeping to Top Sites grid items but replacing the Top Sites list items (on beta, the combined clicks on list items is less than even the lowest Top Sites grid item on phones, and about 1/20 of the clicks of the top clicked grid item).
Summary: Combine various tabs home panels → Combine various history-related home panels
CCing Robin and Maria, because we've talked about this.

Worth looking at the Chrome home and history views on iOS for an example of how these might be unified.
Component: General → Awesomescreen
Hardware: ARM → All
This is logged under Android - can we make it generic for Mobile (cloning to iOS may create more issues)? I'm very much in favor of simplifying history-related panels
I think this is a way for us to address bug 1195728 without creating more UX around our panels. To solve the problem of too many panels, let's have fewer panels! :)
(In reply to mpopova from comment #2)
> This is logged under Android - can we make it generic for Mobile (cloning to
> iOS may create more issues)? I'm very much in favor of simplifying
> history-related panels

Our panels do differ between iOS and Android, the work involved definitely does, and our tracking flags do too, so it's probably best that we have two (or more) bugs.

I do agree that we have similar issues to solve on both platforms (and on desktop!), so let's discuss with Robin and Anthony, and make the space we need to reconsider the problem and figure out what we might do.

I'll try to be the bridge to the history work we're thinking about in services and on desktop in the next year…
CC'ing Nick Chapman

This might overlap with some of Nick's ideas for a better task continuity experience.
I think this is something that we'd like to look at holistically with our other known Home Panel issues. I've be thinking about this more with Darrin and Robin so we can keep these components across mobile feeling more similar but not necessarily being the same.

more to come
Attached image prev_combinedhistory1.png (obsolete) —
Here's a WIP prototype of what I'm thinking as a "merged history" panel.

This places the synced devices at the top (collapsed by default), followed by the aggregated History list. 

There are some UI issues that we will need to tweak slightly to align better with the guidelines around these list items but it shouldn't be too much change. 

For example, the date divider (and "# devices hidden" item) in this mock now don't have as much visual prominence. That's done by adjusting the vertical height of these elements to be more consistent with each other, while remaining balanced against the vertical height of the regular list items.
Looks good! Does this mean we're getting rid of "recently closed X" completely, and you'll just go through "Today" history to see the tabs you've most recently opened?
(In reply to Chenxia Liu [:liuche] from comment #8)
> Looks good! Does this mean we're getting rid of "recently closed X"
> completely, and you'll just go through "Today" history to see the tabs
> you've most recently opened?

We'll have to figure out what this means for restoring these tabs. This would also remove the "Open all" functionality that we currently have in the recent tabs panel.

Given how few people use that panel, maybe it's fine to get rid of these buttons? We should look at telemetry data.

We could also provide a different way to restore your previous session. But maybe if you don't have session restore enabled by default, you don't really care.

In any case, I think we should implement this as a completely new panel, and we can worry about removing the old panels once we've either ported or decided to drop their functionality.
From a UX point of view, I think removing that panel works. 

But, I do think there's some connection with the "Restore tabs" settings pref that is OFF by default. I think it's ON by default in iOS and perhaps having that ON would help the situation.

That being said, if we feel that we need to test this, creating this as a new panel would be a good start.
(In reply to Anthony Lam (:antlam) from comment #10)
> From a UX point of view, I think removing that panel works. 
> 
> But, I do think there's some connection with the "Restore tabs" settings
> pref that is OFF by default. I think it's ON by default in iOS and perhaps
> having that ON would help the situation.

I am interested to know how much people use this restore tabs functionality. We do have a probe for how often people hit the "Open all" buttons - it's the "button" method here:
https://www.dropbox.com/s/4gxvuxguz158tyy/Screenshot%202016-01-05%2020.42.26.PNG?dl=0

But right now we don't distinguish between recently closed tabs and tabs from last time. I'll file a bug to improve the probes here.

However, you can see that clearly this is a pretty small percentage of the URL loads that come from the home panels.
Depends on: 1237144
I agree that the 4 panels are confusing and too similar.

OTOH, they serve different purposes. For example, I want to use only Bookmarks and (Full) History. I never want to use Top Sites nor Synced Tabs (because I don't use sync). "Restored Tabs" shouldn't even be a (long-living) panel, because I need it only when I start, just restore them (after asking me) and then I don't need them anymore. "Synced Tabs" could probably work the same way: They should be actualy tabs, not links on a panel. So, that would already remove "Restored Tabs" and "Synced Tabs"

That leaves only Bookmarks, Top Sites and Full History. I think these are easy enough to understand. It's a question of preference and work organization whether the user uses Bookmarks or Top Sites or both. I'm a control freak and want to organize my start page, including what appears and what does not, in which order, and with what title (thus I use "Bookmarks"), others couldn't care less and want it to be done automatically ("Top Sites").
Assignee: nobody → liuche
Attached image prev_history_mock3.png (obsolete) —
Talked about this more IRL - here's the updated mock. We'll reuse the current Bookmarks folder UX.
Attachment #8698683 - Attachment is obsolete: true
Morphing this bug to split the scope of this bug.

Combining Recent Tabs will now be tracked by bug 1251362.
Blocks: 1251362
Summary: Combine various history-related home panels → Combine Synced Tabs and History home panels
A few more things that need to be done:

* Add date/month headers to History view
* Update footer button in child view to say "Open all"
* Add context menu items
* Clean up code to make this more extensible for other panels

And as a next part, port the tablet dual-fragment view.
And some WIP patches for this work.
Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36983/diff/1-2/
Attachment #8724328 - Attachment description: MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. → MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian
Attachment #8724328 - Flags: review?(s.kaspari)
Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36985/diff/1-2/
Attachment #8724329 - Attachment description: MozReview Request: Clients and history loading. → MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian
Attachment #8724329 - Flags: review?(s.kaspari)
Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36989/diff/1-2/
Attachment #8724331 - Attachment description: MozReview Request: Make static viewType to ItemType converter. → MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian
Attachment #8724331 - Flags: review?(s.kaspari)
Attachment #8724332 - Attachment description: MozReview Request: Open links for history. → MozReview Request: Bug 1220928 - Open links for history. r=sebastian
Attachment #8724332 - Flags: review?(s.kaspari)
Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36991/diff/1-2/
Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36993/diff/1-2/
Attachment #8724333 - Attachment description: MozReview Request: Add remote client children. → MozReview Request: Bug 1220928 - Add remote client children. r=sebastian
Attachment #8724333 - Flags: review?(s.kaspari)
Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36995/diff/1-2/
Attachment #8724334 - Attachment description: MozReview Request: Add footerbar button. → MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian
Attachment #8724334 - Flags: review?(s.kaspari)
Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36987/diff/1-2/
Attachment #8724330 - Attachment description: MozReview Request: Lazy not-totally-correct favicon handling. → MozReview Request: Bug 1220928 - Add dividers. r=sebastian
Attachment #8724330 - Flags: review?(s.kaspari)
I cleaned up all the previous code and fixed all the little problems from this being a hack, and also added RecyclerView decorations.

All these patches so far should be ready for review, though there is more to come:
- Adding date headers for history
- Handling the empty state
- Context menus
- Hiding hidden remote clients
- Handling tablet state

And of course removing the HistoryPanel and replacing it with the CombinedHistoryPanel.
Attachment #8724325 - Attachment is obsolete: true
Attachment #8724326 - Attachment is obsolete: true
Some updated screenshots.
Is it just me, or does the Device row in the main History page look like a header and the history items look like history from that device?

Shouldn't the Device row give some indication that it spawns a child? Like a ">" on the right side? Don't we use that for folder? Isn't this similar in spirit?

Are we worried about having several Device rows at the top?
Flags: needinfo?(alam)
Attachment #8724328 - Flags: review?(s.kaspari) → review+
Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

https://reviewboard.mozilla.org/r/36983/#review35867

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:19
(Diff revision 2)
> +    private enum ItemType {
> +        CLIENT, HISTORY
> +    }

Just from this patch's point of view I'd use int constants instead of an enum; they have less overhead and your are only using .ordinal() here anyways. But maybe this changes in the other patches. :)

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:34
(Diff revision 2)
> +                view = inflater.inflate(R.layout.home_item_row, viewGroup, false);
> +                return new CombinedHistoryItem(view);

NIT: Indentation

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:41
(Diff revision 2)
> +        final int numClients = remoteClients == null ? 0 : remoteClients.size();

This code is not complete yet, but often this code is much simpler if we guarantee to always have a list (empty) and skip the null checks. Collections.emptyList() is helpful for that.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryItem.java:15
(Diff revision 2)
> +    public static class ClientItem extends CombinedHistoryItem {

It took a while to understand this: The inner class extends the outer class. Why not just two separate ViewHolder classes? Introduce an interface if you want the adapter to handle both through shared methods.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryRecyclerView.java:36
(Diff revision 2)
> +        layoutManager.scrollToPosition(0);

Is position 0 not the default anyways? :)
Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

https://reviewboard.mozilla.org/r/36985/#review35869

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:31
(Diff revision 2)
> +    public void replaceClients(List<RemoteClient> clients) {
> +        remoteClients = clients;
> +        notifyDataSetChanged();
> +    }
> +
> +    public void replaceHistory(Cursor history) {

I feel like setHistory() or setClients() would be easier to understand without needing to read the code? I was confused by "replace". :)

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:54
(Diff revision 2)
> +        context = viewGroup.getContext();

Maybe consider making this an explicit constructor argument if other methods need this. This will avoid NullPointerExceptions and there's no implicit "you can only use 'context' after onCreateViewHolder() has been called at least once".

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:80
(Diff revision 2)
> +        final int localPosition = transformPosition(itemType, position);
> +        switch (itemType) {
> +            case CLIENT:
> +                final CombinedHistoryItem.ClientItem clientItem = (CombinedHistoryItem.ClientItem) viewHolder;
> +                final RemoteClient client = remoteClients.get(localPosition);
> +                clientItem.nameView.setText(client.name);
> +                clientItem.nameView.setTextColor(ContextCompat.getColor(context, R.color.placeholder_active_grey));
> +                clientItem.deviceTypeView.setImageResource("desktop".equals(client.deviceType) ? R.drawable.sync_desktop : R.drawable.sync_mobile);
> +
> +                final long now = System.currentTimeMillis();
> +                clientItem.lastModifiedView.setText(RemoteTabsExpandableListAdapter.getLastSyncedString(context, now, client.lastModified));
> +                break;
> +
> +            case HISTORY:
> +                final TwoLinePageRow pageRow = (TwoLinePageRow) viewHolder.itemView;
> +                pageRow.setShowIcons(true);
> +                if (historyCursor == null || !historyCursor.moveToPosition(localPosition)) {
> +                    throw new IllegalStateException("Couldn't move cursor to position " + localPosition);
> +                }
> +                pageRow.updateFromCursor(historyCursor);
> +                break;
> +        }
> +    }

I often move this code into a bind() method into the view holders. The code here will just delegate and every view holder is only handling itself (simple code).

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:48
(Diff revision 2)
> +        mClearHistoryButton = view.findViewById(R.id.clear_history_button);

Just FYI: Currently the "clear history" button is shielded by Restrictions.isAllowed(context, Restrictable.CLEAR_HISTORY); A restricted profile might not be allowed to clear the history.
Attachment #8724329 - Flags: review?(s.kaspari) → review+
Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

https://reviewboard.mozilla.org/r/36989/#review35871

You could move some of that code into methods of the ItemType enum: toViewType(), createViewHolder(), static fromViewType(int). Not sure if this more readable?
Attachment #8724331 - Flags: review?(s.kaspari) → review+
Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

https://reviewboard.mozilla.org/r/36991/#review35873

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryRecyclerView.java:15
(Diff revision 2)
> +import java.util.EnumSet;

Oh cool, I have never used EnumSet. This seems to be handy.
Attachment #8724332 - Flags: review?(s.kaspari) → review+
(In reply to Mark Finkle (:mfinkle) from comment #38)
> Is it just me, or does the Device row in the main History page look like a
> header and the history items look like history from that device?

Yeah, I noticed that too. I believe :liuche polished up the padding there so that the labels were all vertically aligned. That way, there is no indentation.

> Shouldn't the Device row give some indication that it spawns a child? Like a
> ">" on the right side? Don't we use that for folder? Isn't this similar in
> spirit?

Actually, we don't have arrows for our folders currently. So, the plan right now is to revert the visual style to be consistent with our current folders UI. That is, removing the ">". We may bring this back later for actual "collapse/expand"-able items in the panel.

> Are we worried about having several Device rows at the top?

Not particularly. Since we know most users aren't really going to be having more than 3 or so. I have 5 and it still works fine I think. 

After all, this is the "biggest bucket" of the three main panels.
Flags: needinfo?(alam)
https://reviewboard.mozilla.org/r/36983/#review35867

> It took a while to understand this: The inner class extends the outer class. Why not just two separate ViewHolder classes? Introduce an interface if you want the adapter to handle both through shared methods.

Yeah, the original intent behind this was for 1) grouping all the ViewHolders together and 2) the History ViewHolder didn't do anything, so it seemed silly to give it an empty class. However, I think it's a good point that this is confusing, so I think I'll use two classes and also put the onBindView code into each ViewHolder class.

RecyclerView.Adapter requires a single type as the generic, and it also requires that that type inherit from ViewHolder, so trying out an interface doesn't work because interfaces can't extend any classes.
https://reviewboard.mozilla.org/r/36985/#review35869

> Just FYI: Currently the "clear history" button is shielded by Restrictions.isAllowed(context, Restrictable.CLEAR_HISTORY); A restricted profile might not be allowed to clear the history.

I'll address this in the empty state patch! Thanks for reminding me.
https://reviewboard.mozilla.org/r/36989/#review35871

Good call! I'll do that for the toViewType and fromViewType calls. I'd rather keep the createViewHolder call where it is, because I think the ItemType interface can be separate from layouts.
Attachment #8724333 - Flags: review?(s.kaspari) → review+
Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

https://reviewboard.mozilla.org/r/36993/#review36339
Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

https://reviewboard.mozilla.org/r/36995/#review36343

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:69
(Diff revision 2)
> +        mPanelFooterButton.setOnClickListener(new OnFooterButtonClickListener());
> +        mPanelFooterButton.setVisibility(View.VISIBLE);

Restrictions reminder. :)
Attachment #8724334 - Flags: review?(s.kaspari) → review+
Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

https://reviewboard.mozilla.org/r/36987/#review36345
Attachment #8724330 - Flags: review?(s.kaspari) → review+
Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36983/diff/2-3/
Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36985/diff/2-3/
Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36989/diff/2-3/
Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36991/diff/2-3/
Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36993/diff/2-3/
Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36995/diff/2-3/
Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36987/diff/2-3/
https://reviewboard.mozilla.org/r/40281/#review36793

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:255
(Diff revision 1)
>                      }
>                      break;
>              }
>          }
>      }
> +

I'm not super happy about this logic but it seems necessary to use the ViewStub/avoid inflating this view every time. With ListViews you could use setEmptyView and the listview would handle hiding and showing the view (I copied this empty view code from HistoryPanel), but recyclerview doesn't have the same [1].

[1] Although databinding suggests itself as a solution! However, I decided to forgo DB for now, because I'm not sure how it would interact with view stubs, though maybe we could bind the data inside of the viewstub (though since that's reused by all the home panels, this might require more refactoring and I didn't want to open that can of worms right now).
Also, sebastian - I added the logic for detecting Restricted profile, but the restricted profile restrictions didn't seem to work for me. I filed bug 1257302 for that, but I'd like to test these patches out again before I land them (since I wasn't able to actually verify they worked).
Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36983/diff/3-4/
Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36985/diff/3-4/
Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36989/diff/3-4/
Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36991/diff/3-4/
Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36993/diff/3-4/
Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36995/diff/3-4/
Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36987/diff/3-4/
Comment on attachment 8730969 [details]
MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40279/diff/1-2/
Comment on attachment 8730970 [details]
MozReview Request: Bug 1220928 - Add empty state. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40281/diff/1-2/
I pulled a lot of this code from HistoryPanel/Adapter, but cleaned it up a bit, fixed some bugs, renamed things to make them clearer, and also added more comments.
Attachment #8728763 - Attachment is obsolete: true
Comment on attachment 8730969 [details]
MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

https://reviewboard.mozilla.org/r/40279/#review37559
Attachment #8730969 - Flags: review?(s.kaspari) → review+
Comment on attachment 8730970 [details]
MozReview Request: Bug 1220928 - Add empty state. r=sebastian

https://reviewboard.mozilla.org/r/40281/#review37563

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:175
(Diff revision 2)
>                      break;
>              }
>  
> +            // Check and set empty state.
> +            updateButtonFromLevel(OnPanelLevelChangeListener.PanelLevel.PARENT);
> +            showEmptyView(mAdapter.getItemCount() == 0);

The name is a bit confusing: I expect showEmptyView() to always show the view but it sometimes hides the view too.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:284
(Diff revision 2)
> +                emptyHint.setVisibility(View.GONE);
> +            }
> +            mEmptyView.setVisibility(View.VISIBLE);
> +        } else if (isEmpty) {
> +            mEmptyView.setVisibility(View.VISIBLE);
> +        } else if (mEmptyView != null){

Nit: Space before {

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryPanel.java:285
(Diff revision 2)
> +            }
> +            mEmptyView.setVisibility(View.VISIBLE);
> +        } else if (isEmpty) {
> +            mEmptyView.setVisibility(View.VISIBLE);
> +        } else if (mEmptyView != null){
> +            mEmptyView.setVisibility(View.GONE);

NIT: I had a bit problems following this if/else tree for the different situations. Particularly here I was stuck: Why do we not hide the view if it is empty. But then I realized we do: At this point isEmpty has to be false.

I wonder if this could be restructured to be more clear:

if (isEmpty) {
  if (mEmptyView == null) {
    // create view
  }
  mEmptyView.setVisibility(..);
} else {
  if (mEmptyView != null) {
    mEmptyView.setVisibility(..);
  }
}

What do you think? Your choice.
Attachment #8730970 - Flags: review?(s.kaspari) → review+
Comment on attachment 8731569 [details]
MozReview Request: Bug 1220928 - Add section headers. r=sebastian

https://reviewboard.mozilla.org/r/40717/#review37567

I remember seeing something similar (for quick jumping to sections in a list of text) in the framework using a binary search:
https://android.googlesource.com/platform/frameworks/base.git/+/master/core/java/android/widget/AlphabetIndexer.java
I don't think that's needed here but maybe it's interesting reference code. :)

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:185
(Diff revision 1)
> +     * @param sparseArray SparseArray to populate
> +     */
> +    private static void populateSectionHeaders(Cursor c, SparseArray<SectionHeader> sparseArray) {
> +        sparseArray.clear();
> +
> +        if (c == null || !c.moveToFirst()) {

NIT: You could ommit the moveToFirst() here and then start your loop with moveToNext():

while (cursor.moveToNext()) {
..
}

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:191
(Diff revision 1)
> +            final int historyPosition = c.getPosition();
> +            final long visitTime = c.getLong(c.getColumnIndexOrThrow(BrowserContract.History.DATE_LAST_VISITED));
> +            final SectionHeader itemSection = CombinedHistoryPanel.getSectionFromTime(visitTime);

I wonder if there's a performance gain if those variables are declared outside the loop and reused.
Attachment #8731569 - Flags: review?(s.kaspari) → review+
https://reviewboard.mozilla.org/r/40717/#review37567

> NIT: You could ommit the moveToFirst() here and then start your loop with moveToNext():
> 
> while (cursor.moveToNext()) {
> ..
> }

Hmm, I think that's a good point, since we're using the cursor immediately after setting it in the adapter. But since this is a static method, I'd like to guarantee that the cursor is at the right place when I start, so it doesn't need to assume any state.
Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36983/diff/4-5/
Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36985/diff/4-5/
Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36989/diff/4-5/
Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36991/diff/4-5/
Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36993/diff/4-5/
Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36995/diff/4-5/
Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36987/diff/4-5/
Comment on attachment 8730969 [details]
MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40279/diff/2-3/
Comment on attachment 8730970 [details]
MozReview Request: Bug 1220928 - Add empty state. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40281/diff/2-3/
Comment on attachment 8731569 [details]
MozReview Request: Bug 1220928 - Add section headers. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40717/diff/1-2/
QA Contact: mihai.ninu
***

Review commit: https://reviewboard.mozilla.org/r/42817/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42817/
Attachment #8734209 - Attachment description: MozReview Request: Bug 1220928 - Add context menu WIP. r=sebastion → MozReview Request: Bug 1220928 - Add context menu. r=sebastion
Attachment #8735654 - Flags: review?(s.kaspari)
Attachment #8735655 - Flags: review?(s.kaspari)
Attachment #8735656 - Flags: review?(s.kaspari)
Attachment #8735657 - Flags: review?(s.kaspari)
Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36983/diff/5-6/
Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36985/diff/5-6/
Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36989/diff/5-6/
Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36991/diff/5-6/
Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36993/diff/5-6/
Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36995/diff/5-6/
Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36987/diff/5-6/
Comment on attachment 8730969 [details]
MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40279/diff/3-4/
Comment on attachment 8730970 [details]
MozReview Request: Bug 1220928 - Add empty state. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40281/diff/3-4/
Comment on attachment 8731569 [details]
MozReview Request: Bug 1220928 - Add section headers. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40717/diff/2-3/
Comment on attachment 8734209 [details]
MozReview Request: Bug 1220928 - Add context menu. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42135/diff/1-2/
Blocks: 1260320
I'd like to finish up the phone mode of combined history panels, and then handle the tablet mode in a follow-up so we get more Nightly testing time, so I filed bug 1260320.

This also doesn't handle migrations of users with custom home panels, which I will do in bug 1260321.
https://reviewboard.mozilla.org/r/42135/#review39431

There's a typo in my name and reviewboard didn't flag me to review this patch. :)
Comment on attachment 8735654 [details]
MozReview Request: Bug 1220928 - Add client hiding/showing. r=sebastian

https://reviewboard.mozilla.org/r/42817/#review39435

There are two MozReview-Commit-ID in the commit message - this looks wrong. Maybe from collapsing two commits?
Attachment #8735654 - Flags: review?(s.kaspari)
Comment on attachment 8734209 [details]
MozReview Request: Bug 1220928 - Add context menu. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42135/diff/2-3/
Attachment #8734209 - Attachment description: MozReview Request: Bug 1220928 - Add context menu. r=sebastion → MozReview Request: Bug 1220928 - Add context menu. r=sebastian
Attachment #8735654 - Flags: review?(s.kaspari)
Comment on attachment 8735654 [details]
MozReview Request: Bug 1220928 - Add client hiding/showing. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42817/diff/1-2/
Comment on attachment 8735655 [details]
MozReview Request: Bug 1220928 - Handle back button when in client's child view. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42891/diff/1-2/
Comment on attachment 8735656 [details]
MozReview Request: Bug 1220928 - Remove old History and Synced Tabs panel for uncustomized home panels. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42893/diff/1-2/
Comment on attachment 8735657 [details]
MozReview Request: Bug 1220928 - Handle configuration changes. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42895/diff/1-2/
Whoops! Thanks for reviewing that patch anyways :) I fixed the mozreview commit ids - I think I had pushed a draft and then collapsed two commits and didn't delete the combined commit messages.
Hiding and showing the clients also uses the recyclerview sliding animation. I didn't do this for the history items because a) it's (read-only) cursor and we requery the database and b) it's inconsistent with how we remove link/history items in other panels, so I'll see if antlam cares about that inconsistency there.
Attachment #8731573 - Attachment is obsolete: true
Comment on attachment 8735654 [details]
MozReview Request: Bug 1220928 - Add client hiding/showing. r=sebastian

https://reviewboard.mozilla.org/r/42817/#review39545

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:53
(Diff revision 2)
>      private List<RemoteClient> remoteClients = Collections.emptyList();
>      private List<RemoteTab> clientChildren;
>      private Cursor historyCursor;
>  
> +    // Maintain group collapsed and hidden state. Only accessed from the UI thread.
> +    protected static RemoteTabsExpandableListState sState;

Why is this static? From a quick glimpse I doesn't seem to be very expensive. It reads and writes from SharedPreferences.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:125
(Diff revision 2)
> +                if (!hasHiddenClients) {
> +                    // Add item for unhiding clients;
> +                    remoteClients.add(null);
> +                } else {
> +                    // Update hidden devices number.
> +                    notifyItemChanged(remoteClients.size() - 1);

"remoteClients.size() - 1" is used multiple times and I wonder if we should hide this behind a helper method?
Attachment #8735654 - Flags: review?(s.kaspari) → review+
Comment on attachment 8735655 [details]
MozReview Request: Bug 1220928 - Handle back button when in client's child view. r=sebastian

https://reviewboard.mozilla.org/r/42891/#review39547
Attachment #8735655 - Flags: review?(s.kaspari) → review+
Attachment #8735656 - Flags: review?(s.kaspari)
Comment on attachment 8735656 [details]
MozReview Request: Bug 1220928 - Remove old History and Synced Tabs panel for uncustomized home panels. r=sebastian

https://reviewboard.mozilla.org/r/42893/#review39549

Should those config changes be behind a build or switchboard flag?
Comment on attachment 8735656 [details]
MozReview Request: Bug 1220928 - Remove old History and Synced Tabs panel for uncustomized home panels. r=sebastian

https://reviewboard.mozilla.org/r/42893/#review39551
Attachment #8735657 - Flags: review?(s.kaspari) → review+
Comment on attachment 8735657 [details]
MozReview Request: Bug 1220928 - Handle configuration changes. r=sebastian

https://reviewboard.mozilla.org/r/42895/#review39553

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:48
(Diff revision 2)
>          public static int itemTypeToViewType(ItemType itemType) {
>              return itemType.ordinal();
>          }
>      }
>  
> +

Nit: Empty line?
https://reviewboard.mozilla.org/r/42817/#review39645

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:53
(Diff revision 2)
>      private List<RemoteClient> remoteClients = Collections.emptyList();
>      private List<RemoteTab> clientChildren;
>      private Cursor historyCursor;
>  
> +    // Maintain group collapsed and hidden state. Only accessed from the UI thread.
> +    protected static RemoteTabsExpandableListState sState;

This is static to decrease racing because ideally there is only on sState - although according to the RemoteTabs code, even if it did race, this shouldn't be a big deal because it ends up being backed by SharedPreferences.

::: mobile/android/base/java/org/mozilla/gecko/home/CombinedHistoryAdapter.java:125
(Diff revision 2)
> +                if (!hasHiddenClients) {
> +                    // Add item for unhiding clients;
> +                    remoteClients.add(null);
> +                } else {
> +                    // Update hidden devices number.
> +                    notifyItemChanged(remoteClients.size() - 1);

Okay, I added a separate method, with a comment.

Of note, the ordering of notifyItem...() calls does matter, which messed me up a bit here.
http://www.birbit.com/recyclerview-animations-part-2-behind-the-scenes/
https://reviewboard.mozilla.org/r/42893/#review39549

Putting these changes behind a switchboard flag makes it a little hard for migration, so we decided to not do that - this is just a partial landing to get everything out for more Nightly testing while I finish the tablet mode.
Note, this is a partial landing of the full feature because there are a lot of patches and we wanted to get this into Nightly.

STR: for devices without customized home panels (e.g., have not hidden or reordered or added any panels from settings). This does not implement landscape tablet mode.

- Removes Synced Tabs panel
- Shows clients in the History panel, and allows navigating into them to see synced tabs
- Maintains all old history behavior

Does not handle split-pane tablet landscape mode, or customized home panels.
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8311619&repo=fx-team
Flags: needinfo?(liuche)
Comment on attachment 8724328 [details]
MozReview Request: Bug 1220928 - Add RecyclerView boilerplate. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36983/diff/6-7/
Comment on attachment 8724329 [details]
MozReview Request: Bug 1220928 - Clients and history loading. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36985/diff/6-7/
Comment on attachment 8724331 [details]
MozReview Request: Bug 1220928 - Make static viewType to ItemType converter. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36989/diff/6-7/
Comment on attachment 8724332 [details]
MozReview Request: Bug 1220928 - Open links for history. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36991/diff/6-7/
Comment on attachment 8724333 [details]
MozReview Request: Bug 1220928 - Add remote client children. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36993/diff/6-7/
Comment on attachment 8724334 [details]
MozReview Request: Bug 1220928 - Add footerbar button. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36995/diff/6-7/
Comment on attachment 8724330 [details]
MozReview Request: Bug 1220928 - Add dividers. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36987/diff/6-7/
Comment on attachment 8730969 [details]
MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40279/diff/4-5/
Comment on attachment 8730970 [details]
MozReview Request: Bug 1220928 - Add empty state. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40281/diff/4-5/
Comment on attachment 8731569 [details]
MozReview Request: Bug 1220928 - Add section headers. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40717/diff/3-4/
Comment on attachment 8734209 [details]
MozReview Request: Bug 1220928 - Add context menu. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42135/diff/3-4/
Comment on attachment 8735654 [details]
MozReview Request: Bug 1220928 - Add client hiding/showing. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42817/diff/2-3/
Comment on attachment 8735655 [details]
MozReview Request: Bug 1220928 - Handle back button when in client's child view. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42891/diff/2-3/
Comment on attachment 8735656 [details]
MozReview Request: Bug 1220928 - Remove old History and Synced Tabs panel for uncustomized home panels. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42893/diff/2-3/
Comment on attachment 8735657 [details]
MozReview Request: Bug 1220928 - Handle configuration changes. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42895/diff/2-3/
(oh man, that was a lot of comments...!)

I forgot to update the tests, and did so in this last patch.

Green try run here (with some unrelated telemetry orange): https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2705c212a6c9dfd5a940dd5a4b9728dab9d2311
Flags: needinfo?(liuche)
Comment on attachment 8736516 [details]
MozReview Request: Bug 1220928 - Update test references to old history panel. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43337/diff/1-2/
Attachment #8736516 - Attachment description: MozReview Request: Bug 1220928 - Update references to old history panel. r=sebastian → MozReview Request: Bug 1220928 - Update test references to old history panel. r=sebastian
Comment on attachment 8736516 [details]
MozReview Request: Bug 1220928 - Update test references to old history panel. r=sebastian

https://reviewboard.mozilla.org/r/43337/#review40047
Attachment #8736516 - Flags: review?(s.kaspari) → review+
Blocks: 1261527
Depends on: 1261753
Hi Ninu, it looks like you pinged me on irc but I wasn't around. In the future, just comment in bugzilla and needinfo me.

I put an STR in this bug (comment #121) but it might have gotten lost in the backouts/review requests. Take a look there, and also at the dependencies on this bug, and the history panel metabug (bug 1262929).

Let me know if you have any more questions.

(Also, if you could add a [:ninu] to your Bugzilla name, it'll help people quickly NI you on bugzilla.)
Flags: needinfo?(mihai.ninu)
No longer blocks: 1251362, 1260320, 1261527
Hi Chenxia, 

Noted, all clear now, thanks a lot for the explanations, if i have further questions we'll keep in touch.
Flags: needinfo?(mihai.ninu)
Depends on: 1264262
Verified as fixed on both the Nightly (49.0a1 2016-04-27) and Aurora (48.0a2 2016-04-28) builds on a Xiaomi mi i4 with Android 5.1.1 and Samsung Galaxy S7 Edge with Android 6.0.1
Depends on: 1270162
No longer depends on: 1270162
Depends on: 1281797
You need to log in before you can comment on or make changes to this bug.