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

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.

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

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.

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

Talked about this more IRL - here's the updated mock. We'll reuse the current Bookmarks folder UX.

Morphing this bug to split the scope of this bug. Combining Recent Tabs will now be tracked by bug 1251362.

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.

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

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

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

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

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

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

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

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/

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/

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/

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/

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/

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/

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.

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?

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.

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?

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.

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

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.

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

Comment on attachment 8724330 [details] MozReview Request: Bug 1220928 - Add dividers. r=sebastian https://reviewboard.mozilla.org/r/36987/#review36345

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/

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

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

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/

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

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.

Comment on attachment 8730969 [details] MozReview Request: Bug 1220928 - Align synced clients with history items. r=sebastian https://reviewboard.mozilla.org/r/40279/#review37559

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.

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.

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/

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

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

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

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

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

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/

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 8734209 [details] MozReview Request: Bug 1220928 - Add context menu. r=sebastian https://reviewboard.mozilla.org/r/42135/#review39433

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?

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/

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.

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?

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

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

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.

https://hg.mozilla.org/integration/fx-team/rev/d64938914b3e https://hg.mozilla.org/integration/fx-team/rev/ceb669fb17f2 https://hg.mozilla.org/integration/fx-team/rev/f464c44bf882 https://hg.mozilla.org/integration/fx-team/rev/6220841fe652 https://hg.mozilla.org/integration/fx-team/rev/875fbe8190d3 https://hg.mozilla.org/integration/fx-team/rev/297774d61b59 https://hg.mozilla.org/integration/fx-team/rev/450290121373 https://hg.mozilla.org/integration/fx-team/rev/f2a5f6a248ed https://hg.mozilla.org/integration/fx-team/rev/4fc2017aee75 https://hg.mozilla.org/integration/fx-team/rev/671ce80172a0 https://hg.mozilla.org/integration/fx-team/rev/b2828872f3e7 https://hg.mozilla.org/integration/fx-team/rev/81b90ed9ff46 https://hg.mozilla.org/integration/fx-team/rev/916fb74ed491 https://hg.mozilla.org/integration/fx-team/rev/ac7d8d5ec09e https://hg.mozilla.org/integration/fx-team/rev/c1a1079daea1

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

Backout: https://hg.mozilla.org/integration/fx-team/rev/c89262d70608 https://hg.mozilla.org/integration/fx-team/rev/f4449c8ce9a8 https://hg.mozilla.org/integration/fx-team/rev/a30dd3829cd0 https://hg.mozilla.org/integration/fx-team/rev/3cdaff81f4d5 https://hg.mozilla.org/integration/fx-team/rev/9c2817f5c900 https://hg.mozilla.org/integration/fx-team/rev/0bc1a886196e https://hg.mozilla.org/integration/fx-team/rev/19947a4a78b4 https://hg.mozilla.org/integration/fx-team/rev/48cabf56e441 https://hg.mozilla.org/integration/fx-team/rev/81a8f88a50c3 https://hg.mozilla.org/integration/fx-team/rev/bacf5386e4b7 https://hg.mozilla.org/integration/fx-team/rev/7709b4037ab1 https://hg.mozilla.org/integration/fx-team/rev/6c3e6356f8a7 https://hg.mozilla.org/integration/fx-team/rev/d27578912ddd https://hg.mozilla.org/integration/fx-team/rev/52247b54de2a https://hg.mozilla.org/integration/fx-team/rev/435d12d164aa

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

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

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/

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

https://hg.mozilla.org/integration/fx-team/rev/c3b0d96c2329 https://hg.mozilla.org/integration/fx-team/rev/da52ef729a4b https://hg.mozilla.org/integration/fx-team/rev/82af1bb19859 https://hg.mozilla.org/integration/fx-team/rev/df8b8021d419 https://hg.mozilla.org/integration/fx-team/rev/1139e8f7b278 https://hg.mozilla.org/integration/fx-team/rev/fe241494b578 https://hg.mozilla.org/integration/fx-team/rev/60cd54c9bf89 https://hg.mozilla.org/integration/fx-team/rev/1ee51352897e https://hg.mozilla.org/integration/fx-team/rev/5539cc5e5980 https://hg.mozilla.org/integration/fx-team/rev/024280137587 https://hg.mozilla.org/integration/fx-team/rev/d55a8f609a81 https://hg.mozilla.org/integration/fx-team/rev/72040ea10a61 https://hg.mozilla.org/integration/fx-team/rev/c5cdf619cd2e https://hg.mozilla.org/integration/fx-team/rev/c63615d8c252 https://hg.mozilla.org/integration/fx-team/rev/251bd4b73f61 https://hg.mozilla.org/integration/fx-team/rev/af73f5dcd1b1

https://hg.mozilla.org/mozilla-central/rev/c3b0d96c2329 https://hg.mozilla.org/mozilla-central/rev/da52ef729a4b https://hg.mozilla.org/mozilla-central/rev/82af1bb19859 https://hg.mozilla.org/mozilla-central/rev/df8b8021d419 https://hg.mozilla.org/mozilla-central/rev/1139e8f7b278 https://hg.mozilla.org/mozilla-central/rev/fe241494b578 https://hg.mozilla.org/mozilla-central/rev/60cd54c9bf89 https://hg.mozilla.org/mozilla-central/rev/1ee51352897e https://hg.mozilla.org/mozilla-central/rev/5539cc5e5980 https://hg.mozilla.org/mozilla-central/rev/024280137587 https://hg.mozilla.org/mozilla-central/rev/d55a8f609a81 https://hg.mozilla.org/mozilla-central/rev/72040ea10a61 https://hg.mozilla.org/mozilla-central/rev/c5cdf619cd2e https://hg.mozilla.org/mozilla-central/rev/c63615d8c252 https://hg.mozilla.org/mozilla-central/rev/251bd4b73f61 https://hg.mozilla.org/mozilla-central/rev/af73f5dcd1b1

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

Hi Chenxia, Noted, all clear now, thanks a lot for the explanations, if i have further questions we'll keep in touch.

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