The Remote Tabs version of Bug 846569. When we open a page and switch back, the remote tabs list shouldn't jump back to the top.

I'm happy to mentor this ticket, but it needs a little research to move it forward. First, we need to understand what ExpandableListView [1] already does to save its position when you pause and resume an activity. Then we need to experiment with using that position when we go back to the panel; that's tricky because we remove and insert the set of remote tabs when the panel is opened, but also when a sync completes. We probably want to try to keep a tab (or some tabs) that were on-screen before we switched away on-screen when we switch back. Lots of room to experiment in this ticket. [1] http://developer.android.com/reference/android/widget/ExpandableListView.html

The approach taken in Bug 846569 is to mark one of the items (in that case, the current tab) as selected. A similar approach might work here, with the implied need to find out what the new position should be. A corollary bug might be to not blindly throw everything away on redisplay!

I spoke with nalexander on IRC and would like to work on this.

(In reply to vivek from comment #3) > I spoke with nalexander on IRC and would like to work on this. vivek: any progress here?

Hi Nick, I'm Sorry that I didn't make much progress with this bug. However, I've time this weekend and I intend to make some progress by Monday.

capella: don't spend too much time on this in its current form. I'd rather see this tackled *after* we transition Remote Tabs to a home panel (like Bookmarks, etc). It would be better to figure out how to make all HomeListViews (I think that's the correct class) remember positions than do something funky for RemoteTabsListPanel. In fact, if you could dig into remembering position in bookmarks/history, that would be useful.

It wasn't a lot of effort, this is functional. groupListItems now expand / collapse, and position is remembered between refreshes such that the groupItem that had activity (selection, expand, collapse) will be re-positioned on the page.

screen shot with new group indicators https://www.dropbox.com/lightbox/home

er, that is https://www.dropbox.com/s/7ikshbskkoibh92/bug1003610.png

Comment on attachment 8448061 [details] [diff] [review] bug1003610.diff (v0) Review of attachment 8448061 [details] [diff] [review]: ----------------------------------------------------------------- In general, I like this approach and am happy to land something like it. I have some specific comments about the implementation, but they're not significant. I'd like to land without the indicators, however, because they don't match the UX vision presented in Bug 899643. That'll make the patch much smaller at the cost of the grouping clarity. ::: mobile/android/base/resources/layout/remote_tabs_group.xml @@ +8,5 @@ > android:layout_width="match_parent" > android:layout_height="@dimen/remote_tab_child_row_height" > android:gravity="center_vertical" > android:orientation="vertical" > + android:paddingLeft="29dp" Urgh, no. ::: mobile/android/base/tabspanel/RemoteTabsList.java @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public nit: trailing ws throughout. @@ +37,5 @@ > > private final Context context; > private TabsPanel tabsPanel; > > + private ArrayList <HashMap <String, String>> clients; I'd be happier if this didn't persist. We're going to be moving to a view backed from a Cursor (like a home panel), so this is a step in the wrong direction. @@ +42,3 @@ > private ArrayList <ArrayList <HashMap <String, String>>> tabsList; > > + // Keep a list of expanded groups, and the group that was expanded/collapsed most recently nit: make this either, "We keep a list...", or better yet, just: "A list of the groups currently expanded...". And put the doc for selectedGroupPosition with that member: "The group that was expanded or collapsed...". And in general, it's better to talk about *clients* than groups. That's the real domain level concept. So expandedClients, selectedClientPosition. @@ +71,5 @@ > + } else { > + expandGroup(groupPosition); > + expandedGroupList.add(groupGuid); > + } > + I'd prefer to do the list handling, and then pass through (return false) so that we get *exactly* what the platform does for expansion. @@ +100,5 @@ > if (remoteTabs == null || remoteTabs.size() == 0) > return; > > + final String selectedGuid = (selectedGroupPosition != -1) ? > + clients.get(selectedGroupPosition).get("guid") : ""; This is dangerous. I think it will work, but you're indexing across a list that can change. Be much more careful checking bounds, etc. @@ +108,5 @@ > tabsList = new ArrayList <ArrayList <HashMap <String, String>>>(); > > + // Init a new expanded tabs list / selected tab > + List<String> newExpandedGroupList = new ArrayList<String>(); > + nit: trialing ws! @@ +147,5 @@ > R.layout.remote_tabs_child, > TAB_KEY, > TAB_RESOURCE)); > > + // Update the expanded tabs list / selected tab nit: full sentence comments throughout.

We can move the groupIndicators to the right ... (or add them later) ... do you have the .png's made up yet?

(In reply to Mark Capella [:capella] from comment #11) > We can move the groupIndicators to the right ... (or add them later) ... do > you have the .png's made up yet? I do not have .PNGs. There are major issues positioning groupIndicators (Google it), so I elected not to use them in my WIP patches for making this a home panel. Better to configure the group views ourselves. So either do this as a part 2, or a separate ticket.

I re-worked the groupIndicator PNG's and left them in, as I'm a little worried about completely removing them. As the new code first opens with all groups collapsed by default: https://www.dropbox.com/s/44qfwqmg07i73fh/bug1003610Default.png I'm worried that the fact that the listitems are clickable/expandable, won't easily be discoverable without the visual clue. https://www.dropbox.com/s/zimik1gfg175hfr/bug1003610Expanded.png Maybe you'll reconsider? If not, they're quickly removed. I'm not sure what |Urgh, no.| means, but it did point out I wasn't consistent in my |paddingLeft| definitions. Both my GS3 and N7 seem to use the layout-xlarge-v11/remote_tabs_group.xml, so I'm not sure how to review what the layout/remote_tabs_group.xml version will look like. re: |I'd prefer to do the list handling, and then pass through (return false)| ... not sure what you're looking for here either. The android docs for onGroupClick() and onChildClick() both state to return true if the click was handled (which we do). If anything, this existing piece of code might want to be changed to false: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabspanel/RemoteTabsList.java?rev=ee7ce47e5212&mark=70-70#65

(In reply to Mark Capella [:capella] from comment #13) > Created attachment 8448308 [details] [diff] [review] > bug1003610.diff (v1) > > I re-worked the groupIndicator PNG's and left them in, as I'm a little > worried about completely removing them. As the new code first opens with all > groups collapsed by default: > https://www.dropbox.com/s/44qfwqmg07i73fh/bug1003610Default.png > > I'm worried that the fact that the listitems are clickable/expandable, won't > easily be discoverable without the visual clue. > https://www.dropbox.com/s/zimik1gfg175hfr/bug1003610Expanded.png > > Maybe you'll reconsider? If not, they're quickly removed. If we're going to have them, we should persist the state across refreshes, and probably across application usage. That's a different ticket. So I think we just land this little piece for this ticket. > I'm not sure what |Urgh, no.| means, but it did point out I wasn't > consistent in my |paddingLeft| definitions. Both my GS3 and N7 seem to use > the layout-xlarge-v11/remote_tabs_group.xml, so I'm not sure how to review > what the layout/remote_tabs_group.xml version will look like. > > re: |I'd prefer to do the list handling, and then pass through (return > false)| ... not sure what you're looking for here either. The android docs > for onGroupClick() and onChildClick() both state to return true if the click > was handled (which we do). If anything, this existing piece of code might > want to be changed to false: I want us to do stuff (make a note), and then pass through. That's not handling the event. That is, I want the system to handle the event, 'cuz I want whatever the standard behaviour is (just a little bit more). > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tabspanel/ > RemoteTabsList.java?rev=ee7ce47e5212&mark=70-70#65 Nah, we're hiding there so we want to swallow the event.

Comment on attachment 8448308 [details] [diff] [review] bug1003610.diff (v1) Review of attachment 8448308 [details] [diff] [review]: ----------------------------------------------------------------- I think we should put this aside until we make progress on the Home Panel approach. My vision for that moves us to a database-backed ExpandableListView, which will obviate tracking these clients in this way. However, this code is basically fine; we could land just the scroll position stuff if you would like, and leave the group indicators to a future ticket. ::: mobile/android/base/resources/layout-xlarge-v11/remote_tabs_child.xml @@ +8,5 @@ > android:gravity="center_vertical" > android:layout_width="match_parent" > android:layout_height="@dimen/remote_tab_child_row_height" > android:orientation="vertical" > + android:paddingLeft="14dp" These values seem magic. Are they the width of your indicators? Why are they different on different platforms (phone, tablet)? In antlam's mocks, the left padding depends on the device icon. We should anticipate that rather than landing with indicators. ::: mobile/android/base/tabspanel/RemoteTabsList.java @@ +42,3 @@ > private ArrayList <ArrayList <HashMap <String, String>>> tabsList; > > + // A list of the clients currently expanded... nit: full sentences, no ellipsis. @@ +42,4 @@ > private ArrayList <ArrayList <HashMap <String, String>>> tabsList; > > + // A list of the clients currently expanded... > + private List<String> expandedClentList = new ArrayList<String>(); This is typoed. Also, to me, this should be a set of strings, to which we add and remove guids. @@ +44,5 @@ > + // A list of the clients currently expanded... > + private List<String> expandedClentList = new ArrayList<String>(); > + > + // The client that was expanded or collapsed... > + private int selectedClientPosition = -1; Let's not index into the list this way. Let's save the selectedClient's GUID and run through the list looking for a match. null or no match get handled the same -- just focus the first client. @@ +74,5 @@ > + expandGroup(groupPosition); > + expandedClentList.add(clientGuid); > + } > + > + selectedClientPosition = groupPosition; As I said in another comment, I think this should pass through to super (return false) and not do the expand/collapseGroup itself. @@ +150,5 @@ > R.layout.remote_tabs_child, > TAB_KEY, > TAB_RESOURCE)); > > + // Update the expanded clients list, and the selected client. ... and set the selected ...

Comment on attachment 8448308 [details] [diff] [review] bug1003610.diff (v1) Review of attachment 8448308 [details] [diff] [review]: ----------------------------------------------------------------- I think we should put this aside until we make progress on the Home Panel approach. My vision for that moves us to a database-backed ExpandableListView, which will obviate tracking these clients in this way. However, this code is basically fine; we could land just the scroll position stuff if you would like, and leave the group indicators to a future ticket. ::: mobile/android/base/resources/layout-xlarge-v11/remote_tabs_child.xml @@ +8,5 @@ > android:gravity="center_vertical" > android:layout_width="match_parent" > android:layout_height="@dimen/remote_tab_child_row_height" > android:orientation="vertical" > + android:paddingLeft="14dp" These values seem magic. Are they the width of your indicators? Why are they different on different platforms (phone, tablet)? In antlam's mocks, the left padding depends on the device icon. We should anticipate that rather than landing with indicators. ::: mobile/android/base/tabspanel/RemoteTabsList.java @@ +42,3 @@ > private ArrayList <ArrayList <HashMap <String, String>>> tabsList; > > + // A list of the clients currently expanded... nit: full sentences, no ellipsis. @@ +42,4 @@ > private ArrayList <ArrayList <HashMap <String, String>>> tabsList; > > + // A list of the clients currently expanded... > + private List<String> expandedClentList = new ArrayList<String>(); This is typoed. Also, to me, this should be a set of strings, to which we add and remove guids. @@ +44,5 @@ > + // A list of the clients currently expanded... > + private List<String> expandedClentList = new ArrayList<String>(); > + > + // The client that was expanded or collapsed... > + private int selectedClientPosition = -1; Let's not index into the list this way. Let's save the selectedClient's GUID and run through the list looking for a match. null or no match get handled the same -- just focus the first client. @@ +74,5 @@ > + expandGroup(groupPosition); > + expandedClentList.add(clientGuid); > + } > + > + selectedClientPosition = groupPosition; As I said in another comment, I think this should pass through to super (return false) and not do the expand/collapseGroup itself. @@ +150,5 @@ > R.layout.remote_tabs_child, > TAB_KEY, > TAB_RESOURCE)); > > + // Update the expanded clients list, and the selected client. ... and set the selected ... @@ +165,5 @@ > + expandedClentList = newExpandedClientList; > + > + // Reposition the selected client. > + if (selectedClientPosition != -1) { > + setSelectedGroup(selectedClientPosition); Forgot to say: my read of the Android docs is that this sets the "selected" attribute. We don't really want to "select" things; we want to scroll to them.

Gah. There's only one additional comment (the last) in the second accidental re-review. Sorry about that.

Ah, yah, forgot the device-icon changes coming with antlams patch, so I'll drop the group indicators here. I added them because I was there, it seemed simple, and basically I forgot :p This should give address the rest of your concerns.

Comment on attachment 8450652 [details] [diff] [review] bug1003610.diff (v2) Review of attachment 8450652 [details] [diff] [review]: ----------------------------------------------------------------- If it works for you in testing, it works for me. ::: mobile/android/base/tabspanel/RemoteTabsList.java @@ +37,5 @@ > > private final Context context; > private TabsPanel tabsPanel; > > + private ArrayList <HashMap <String, String>> clients; This can just be an ArrayList<String>, since it's just position -> client GUID. @@ +67,5 @@ > + public boolean onGroupClick(ExpandableListView parent, View view, int groupPosition, long id) { > + final String clientGuid = clients.get(groupPosition).get("guid"); > + > + if (isGroupExpanded(groupPosition)) { > + collapseGroup(groupPosition); We're passing through, so super will collapse and expand, correct? @@ +153,5 @@ > + } > + > + if (clientGuid.equals(clientScrollPosition)) { > + setSelectedGroup(i); > + } I'm still not convinced setSelectedGroup is correct; I feel like it should be just scrolling to show the group, but you've done the testing.

) re: private ArrayList <HashMap <String, String>> clients; |clients| isn't a new array, it's the re-purposed existing one that I moved to global scope for persistence. Is my java-fu letting me down? (I don't follow the suggested change). ) Yes, we're passing through, so super does collapse and expand. ) setSelectedGroup() wfm using two devices, and my laptop sync-ing lengthy open/remote sets of tabs. I always push-to-try in case I've missed touching up some tests, so there'll be a testable build from that if you'd like.

(In reply to Mark Capella [:capella] from comment #20) > ) re: private ArrayList <HashMap <String, String>> clients; > |clients| isn't a new array, it's the re-purposed existing one that I > moved to global scope for persistence. Is my java-fu letting me down? (I > don't follow the suggested change). Roger that. > ) Yes, we're passing through, so super does collapse and expand. v.g., so we don't need it locally. > ) setSelectedGroup() wfm using two devices, and my laptop sync-ing lengthy > open/remote sets of tabs. I always push-to-try in case I've missed touching > up some tests, so there'll be a testable build from that if you'd like. v.g., I trust your testing more than my questions. Looks good!

re: |Yes, we're passing through, so super does collapse and expand.| I wasn't happy with my understanding of that, and the suggestion to return |false| instead of |true|, so I did some more testing and found that after changing to |false| we wind up expanding/collapsing and so does super. Basically, items under the group appear twice on expanding with a first-tap. The next (second) tap collapses half of them, and then the third tap collapses the group all the way. Restoring the code to return |true| works properly as I observed in my original testing, but overlooked after changing. (I noticed things expanding/collapsing, but not exactly what was involved).

Try push ... patch w/original |return true;|. https://tbpl.mozilla.org/?tree=Try&rev=13e088a98b77 I'm going to wait for you to r+ this last bit before I push to fx-team.

Tweaked the return value, carried over r+ from irc https://hg.mozilla.org/integration/fx-team/rev/ff71b1bb0ae3