1063742 - Implement split-pane Remote Tabs home panel for tablets

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

There are a few things I can think of here:

1) The static views shown to tablet users will look very, very wide on tablets.  We need to tighten them up and center them vertically.  We might want to bump font sizes, too.

2) The list of synced tabs shown to tablet users does not follow yuan's mocks (see Bug 1014994 for mocks, specifically https://bugzilla.mozilla.org/attachment.cgi?id=8427449).

This ticket tracks addressing both issues.

Comment 1

[:Margaret Leibovic]

antlam, what are we looking to do here? Is this still something that can make 35?

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #1)
> antlam, what are we looking to do here? Is this still something that can
> make 35?

Bug 1063765 landed fixes for landscape phones that should fix item 1 (which was that the views would be very wide).  We could certainly uplift any changes antlam wants to the static views, but it's not a high priority.

That leaves item 2.  I'll morph this ticket to track implementing yuan's "split pane" tablet view.  We could probably uplift this but it would be a chunk of new Fragment-based code.  I don't think the current experience is bad on tablets so I don't see any reason to rush this through the trains.

Comment 3

[Nick Alexander :nalexander [he/him]]

Per yuan's mocks, this ticket tracks implementing a split-pane interface for viewing and opening remote tabs.

This looks like replacing [1] with a fragment containing two pane fragments and modifying/implementing the adapter or adapters to back the two fragments.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/RemoteTabsExpandableListFragment.java

Comment 4

[Nick Alexander :nalexander [he/him]]

antlam: the ask to you is whether the null-states look OK on tablets to you.  That is, if you don't have an Account, or you need to enter your password again, do the messaging screens look good enough on tablets (portrait and landscape) to you?

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Nick Alexander :nalexander from comment #2)
> (In reply to :Margaret Leibovic from comment #1)
> > antlam, what are we looking to do here? Is this still something that can
> > make 35?
> 
> Bug 1063765 landed fixes for landscape phones that should fix item 1 (which
> was that the views would be very wide).  We could certainly uplift any
> changes antlam wants to the static views, but it's not a high priority.
> 
> That leaves item 2.  I'll morph this ticket to track implementing yuan's
> "split pane" tablet view.  We could probably uplift this but it would be a
> chunk of new Fragment-based code.  I don't think the current experience is
> bad on tablets so I don't see any reason to rush this through the trains.

Yeah I think that's the right call.

(In reply to Nick Alexander :nalexander from comment #4)
> antlam: the ask to you is whether the null-states look OK on tablets to you.
> That is, if you don't have an Account, or you need to enter your password
> again, do the messaging screens look good enough on tablets (portrait and
> landscape) to you?

Yep, I think they're fine. Testing it on my N7.

Comment 6

[Nick Alexander :nalexander [he/him]]

This is a bit of a stretch goal for Firefox 36, but it would be nice to have it in place for the new tablet work landing.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's not force this for 36, but it could make it if everything comes together.

Comment 8

[Nick Alexander :nalexander [he/him]]

vivek: so, this a larger ticket than you've been working on, but are you interested?

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1063742#c3, this means implementing a new Remote Tabs home panel view that displays the list of clients and the list of tabs separately.  I expect that the backing data store can be the same (with a little work, perhaps) [1], but obviously the view hierarchy and the interaction model is pretty different. (See the mocks.)

Let me know and we can talk more!

[1] The data store is currently an ExpandableListAdapter.  It's not clear if we want to handle the ELA -> (clients list adapter, currently selected clients' tabs adapter) split ourselves, or have two regular ListAdapters that we query from the DB.  I was leaning towards one ELA so that it was more obvious when the underlying data store changed and there was less racing between DB queries, but I can't say for sure.

http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/RemoteTabsExpandableListAdapter.java

Comment 9

[Nick Alexander :nalexander [he/him]]

margaret: adding you as a second home panels mentor, since you more or less own that code now.

Comment 10

[Vivek Balakrishnan[:vivek]]

yes, I'm interested in working on this ticket and I've assigned myself to this bug.

Comment 11

[Vivek Balakrishnan[:vivek]]

Attachment: 1063742.patch

Two pane synced tabs panel implemented for tablets. 
* The Two list views are backed by the existing ELA. 
* Handled context menu for both the clients and tabs

Pending: Further work is needed to handle orientation changes, may be use master detail fragment pattern.

Comment 12

[Vivek Balakrishnan[:vivek]]

Attachment: 1063742.patch

Comment 13

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8546204 [details] [diff] [review]
1063742.patch

Updated Patch with the following:

* Handled fragment in switching in tablets
* Holder pattern in ELA for groupView.

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8546204 [details] [diff] [review]
1063742.patch

Review of attachment 8546204 [details] [diff] [review]:
-----------------------------------------------------------------

This is a pain to change throughout, but I think "split pane" is the standard name for these layouts.  I guess iOS says "split view" [1] and Android talks about "multi pane" views [2].  I feel pretty strongly about getting the name right at the beginning.  margaret, your thoughts?

[1] https://developer.apple.com/library/ios/documentation/WindowsViews/Conceptual/ViewControllerCatalog/Chapters/SplitViewControllers.html

[2] http://developer.android.com/design/patterns/multi-pane-layouts.html

Let's get the ExpandableListAdapter changes as a Part 1.

Can we get the ListState changes, with the changes to ELFragment, as a Part 2?  I'm not 100% happy with how the selected client is tracked.

Everything else in a part 3.

Wow, that was a tough first review.  Lots of comments but great patch.  Can't wait to see the re-review!  Bravo!

::: mobile/android/base/RemoteTabsExpandableListAdapter.java
@@ +28,5 @@
>   * specialization to home fragment styles.
>   */
>  public class RemoteTabsExpandableListAdapter extends BaseExpandableListAdapter {
>      protected final ArrayList<RemoteClient> clients;
> +    private final boolean isGrougIndicatorShown;

s/Groug/Group/

And this looks like a question; make it:

p f b showGroupIndicator;

@@ +33,5 @@
>      protected int groupLayoutId;
>      protected int childLayoutId;
>  
> +    private static class GroupViewHolder {
> +        public TextView nameView;

Why not final throughout?

@@ +51,5 @@
>       * @param childLayoutId
>       * @param clients
>       *            initial list of clients; can be null.
> +     * @param isGroupIndicatorShown
> +     *            flag to shown group indicator.

showGroupIndicator, and then it needs no comment.

@@ +156,4 @@
>          }
>  
> +        if (holder.deviceExpandedView != null) {
> +            // If there are no tabs to display or this is in dual list view, don't show an indicator at all.

This will likely be used outside of split pane, so keep the more generic comment.

::: mobile/android/base/home/HomeListView.java
@@ +89,5 @@
>              }
>  
>              mContextMenuInfo = mContextMenuInfoFactory.makeInfoForCursor(view, position, id, cursor);
>              return showContextMenuForChild(HomeListView.this);
> +        } else if(mContextMenuInfoFactory instanceof HomeContextMenuInfo.ListFactory) {

nit: if (.

This doesn't follow either of the two patterns (HomeListView/HomeExpandableListView), and ListFactory shouldn't extend Factory (since it's an error to get a Cursor in these cases, right)?

However, reading the code closely, I'm not going to insist you fix this broken abstraction.  What should happen is that we have a generic Factory makeInfoForHomeListView(details, listView) which extracts a cursor, or uses an adapter, or whatever.  Separate ticket.

::: mobile/android/base/home/RemoteTabsDualListFragment.java
@@ +46,5 @@
> +public class RemoteTabsDualListFragment extends RemoteTabsBaseFragment {
> +    // Logging tag name.
> +    private static final String LOGTAG = "GeckoRemoteTabsDualList";
> +
> +    // private ArrayAdapter<RemoteTab> tabsAdapter;

nit: remove this.  And why aren't these both ArrayAdapter<> instances?

@@ +72,5 @@
> +        return inflater.inflate(R.layout.home_remote_tabs_dual_list_panel, container, false);
> +    }
> +
> +    @Override
> +    public void onViewCreated(View view, Bundle savedInstanceState) {

Always call through to super.  Can the sync status listener be in the BaseFragment?

@@ +178,5 @@
> +            sState = new RemoteTabsExpandableListState(GeckoSharedPrefs.forProfile(getActivity()));
> +        }
> +
> +        // There is an unfortunate interaction between ExpandableListViews and
> +        // footer onClick handling. The footer view itself appears to not

Can't some of this go in the BaseFragment?  It's shared between the two children.

@@ +299,5 @@
> +        }
> +    }
> +
> +    private static class RemoteTabsAdapter extends BaseAdapter {
> +        private static final int MATHCING_GROUP_ID_NOT_FOUND = -1;

s/MATHC/MATCH/

I'm rather tired and having a hard time following this code.  Can you give me a nice explanation of what case you're handling?  When the client doesn't exist?  Make the RTA and RCA ArrayAdapters<> and update their lists explicitly than be tricky with the pass-through like this.  Is this the only way to get the pass-through getView to work?

@@ +327,5 @@
> +        }
> +
> +        @Override
> +        public int getCount() {
> +        int groupPosition = getGroupIdForSelectedClient(sState.getLastClickedClientGuid());

nit: indentation is off in a few places.

::: mobile/android/base/home/RemoteTabsExpandableListFragment.java
@@ +61,5 @@
>          return inflater.inflate(R.layout.home_remote_tabs_list_panel, container, false);
>      }
>  
>      @Override
> +    protected String getLogTag() {

Generally we don't bother.  Just set the LOG_TAG in all three classes; it's as useful to see the base class as the derived class in the log.

::: mobile/android/base/home/RemoteTabsExpandableListState.java
@@ +29,5 @@
>   */
>  public class RemoteTabsExpandableListState {
>      private static final String PREF_COLLAPSED_CLIENT_GUIDS = "remote_tabs_collapsed_client_guids";
>      private static final String PREF_HIDDEN_CLIENT_GUIDS = "remote_tabs_hidden_client_guids";
> +    private static final String PREF_LAST_CLICKED_CLIENT_GUID = "remote_tabs_last_clicked_client_guid";

Through-out: "last clicked" is a detail of the interaction.  You use "selected" elsewhere; let's use selected through-out.

@@ +43,5 @@
>      // "not present" means "shown".
>      // Only accessed from the UI thread.
>      protected final Set<String> hiddenClients;
>  
> +    // Synchronized by the state instance. The last user pressed client which is not hidden.

This isn't true.  Long tapping should not change the selected client.  Therefore, if you Long tap -> Hide a client that is selected, this will be a hidden client.

We have to handle the case where the "selected" client doesn't exist in the list no matter what, 'cuz client GUIDs disappear all the time, so don't worry about making a hidden/not hidden guarantee at all.  Better to be clear that this can be an invalid client guid, or null.

@@ +123,5 @@
> +        return isModified;
> +    }
> +
> +    /**
> +     * Mark a client as last clicked unless if it is updated.

Do you mean "unless it is hidden"?  If so, say what happens when the client is hidden.

@@ +145,5 @@
>          return collapsedClients.contains(clientGuid);
>      }
>  
>      /**
> +     * Mark a client as hidden and update it as the last selected client unless if it is hidden.

Mark a client as hidden.  If the client is not hidden, also select it.

But I don't think this makes sense.  The UI unhides a bunch of clients at the same time (click on the hidden list).  None of them should be selected.

::: mobile/android/base/home/RemoteTabsPanel.java
@@ +59,5 @@
>      // Lazily populated.
>      private Fragment mFallbackFragment;
>  
> +    private boolean hasDualPane;
> +

I'm not a fan of state if we can avoid it.  Why are we caching this at all?  And do we really need to do this?  Why not do this in code (one helper) using something like http://stackoverflow.com/a/6081574?

@@ +72,5 @@
>          super.onActivityCreated(savedInstanceState);
>  
>          // Create callbacks before the initial loader is started.
>          mAccountLoaderCallbacks = new AccountLoaderCallbacks();
> +

nit: no extraneous space.

@@ +77,5 @@
>          loadIfVisible();
>      }
>  
> +    private boolean isFragmentReloadNeeded(final Fragment cachedFragment) {
> +        return (!hasDualPane && cachedFragment instanceof RemoteTabsDualListFragment) ||

Give this a Javadoc: it's not obvious that this returns false if cachedFragment is one of the other types of fragments.  Or make it a clearer if/then.

@@ +81,5 @@
> +        return (!hasDualPane && cachedFragment instanceof RemoteTabsDualListFragment) ||
> +                (hasDualPane && cachedFragment instanceof RemoteTabsExpandableListFragment);
> +    }
> +
> +    @Override protected void loadIfVisible() {

nit: @Override on previous line.

@@ +82,5 @@
> +                (hasDualPane && cachedFragment instanceof RemoteTabsExpandableListFragment);
> +    }
> +
> +    @Override protected void loadIfVisible() {
> +        // Force load the child fragment if fragment is visible and displayed in Tablet with a valid account.

This I don't understand.  Why is this necessary?

::: mobile/android/base/moz.build
@@ +307,5 @@
>      'home/ReadingListPanel.java',
>      'home/ReadingListRow.java',
>      'home/RecentTabsPanel.java',
> +    'home/RemoteTabsBaseFragment.java',
> +    'home/RemoteTabsDualListFragment.java',

How do you feel about RemoteTabsSplitPlaneFragment?

::: mobile/android/base/resources/layout/home_remote_tabs_dual_list_panel.xml
@@ +21,5 @@
> +
> +            <org.mozilla.gecko.home.HomeListView
> +                         android:id="@+id/clients_list"
> +                         style="@style/Widget.RemoteTabsListView"
> +                         android:layout_weight="0.5"

The 0.5/0.5 ratio is off, although I don't know what yuan desired.  I think we should make this have a layout_weight of, say, 0.4-ish (look to the mocks), and a maxwidth of some number of dpi so that it doesn't get too big.  I'll play on my devices but we should get UX input.

::: mobile/android/base/resources/values-large-land-v11/layout.xml
@@ +4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <resources>
>      <item type="layout" name="tabs_panel">@layout/tabs_panel_sidebar</item>
> +    <bool name="has_two_panes_remote_tabs">true</bool>

This works, but I don't know if it matches "Fennec style".  It is "clear" -- although I would prefer to do something more general and then add this as a special case -- and it should avoid weird Cyanogen things where the device lies about its size/configuration.

margaret, can you comment?

Comment 15

[:Margaret Leibovic]

Comment on attachment 8546204 [details] [diff] [review]
1063742.patch

Review of attachment 8546204 [details] [diff] [review]:
-----------------------------------------------------------------

Great work! I didn't go through this patch in detail, since it looks like nalexander did a good job giving feedback (and he's more familiar with this code anyway).

::: mobile/android/base/RemoteTabsExpandableListAdapter.java
@@ +33,5 @@
>      protected int groupLayoutId;
>      protected int childLayoutId;
>  
> +    private static class GroupViewHolder {
> +        public TextView nameView;

If these are all going to be final, perhaps it would be better to have a constructor that takes all of these values as arguments and sets them itself, rather than calling 'holder.foo = bar' where this is used.

::: mobile/android/base/resources/values-large-land-v11/layout.xml
@@ +4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <resources>
>      <item type="layout" name="tabs_panel">@layout/tabs_panel_sidebar</item>
> +    <bool name="has_two_panes_remote_tabs">true</bool>

I don't have enough context around this use of layout.xml files. I'm going to request feedback from mhaigh for more details.

Comment 16

[Martyn Haigh (:mhaigh)]

Comment on attachment 8546204 [details] [diff] [review]
1063742.patch

Review of attachment 8546204 [details] [diff] [review]:
-----------------------------------------------------------------

This is f+ only for the layout config issue - I've not looked at the rest of the patch.

::: mobile/android/base/resources/values-large-land-v11/layout.xml
@@ +4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <resources>
>      <item type="layout" name="tabs_panel">@layout/tabs_panel_sidebar</item>
> +    <bool name="has_two_panes_remote_tabs">true</bool>

There are two ways of doing this - define a boolean, as done here, or check for the existence of a view once the layout has been inflated.  Google use the latter approach in their adaptive UI example (http://developer.android.com/training/multiscreen/adaptui.html#TaskDetermineCurLayout) but I've seen them recommend the former in other examples; I don't think there's a particularly good reason to favour one over the other, apart from personal preference.  I would say though, if going with the boolean approach please move the value in to a bools.xml file. 

re: custom roms which allow you to configure size/config - we shouldn't deal with this as a special case, the people who install those roms and configure them are aware that this could case apps to behave weirdly and will adjust accordingly.  I know - I'm one of them ;)

Comment 17

[Richard Newman [:rnewman]]

Also worth noting: anything that's referred to in code, such as a resource ID like R.id.has_two_panes, but only exists in a v11 resource file, will break when built for Gingerbread — the resource file will be stripped.

(Hoping to save you a backout!)

Comment 18

[Vivek Balakrishnan[:vivek]]

Attachment: part1.patch

I've split my changes into 5 sub patches.
Patch 1 : RemoteTabsExpandableListAdapter holder pattern changes

Comment 19

[Vivek Balakrishnan[:vivek]]

Attachment: part2.patch

Part2 : Extract common functionality from RemoteTabsELF to a base class

Comment 20

[Vivek Balakrishnan[:vivek]]

Attachment: part3.patch

Part 3: Remote tabs split plane fragment implementation.
long press context menu for tabs and clients list will be handled in subsequent patch.

Comment 21

[Vivek Balakrishnan[:vivek]]

Attachment: part4.patch

Part 4: context menu and selected client related changes.
Selected clients are maintained in ELStates. if selected client is not valid or none was selected then fallback is the most recent synced client.

Comment 22

[Vivek Balakrishnan[:vivek]]

Attachment: part5.patch

Part 5: Handle orientation changes in tablet

Comment 23

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8549983 [details] [diff] [review]
part1.patch

Review of attachment 8549983 [details] [diff] [review]:
-----------------------------------------------------------------

nits, nits, nits.  Looking good!

::: mobile/android/base/RemoteTabsExpandableListAdapter.java
@@ +33,5 @@
>      protected int groupLayoutId;
>      protected int childLayoutId;
>  
> +    public static class GroupViewHolder {
> +        final TextView nameView;

Add a comment explaining that these can be null in some situations.  In fact, I think they can no longer be null; see comment below.

@@ +113,5 @@
>              view = convertView;
>          } else {
>              final LayoutInflater inflater = LayoutInflater.from(context);
>              view = inflater.inflate(groupLayoutId, parent, false);
> +            GroupViewHolder holder = new GroupViewHolder(view);

nit: final.

@@ +123,5 @@
> +
> +        return view;
> +    }
> +
> +    public void updateClientsItemView(final boolean isExpanded, final Context context, final View view, final RemoteClient client) {

nit: should this be public?

Could this take the holder instead of the view itself?

@@ +125,5 @@
> +    }
> +
> +    public void updateClientsItemView(final boolean isExpanded, final Context context, final View view, final RemoteClient client) {
> +        GroupViewHolder holder = (GroupViewHolder) view.getTag();
> +        if (!showGroupIndicator) {

This seems odd.  Why are we setting a background color manually?

@@ +160,4 @@
>  
>          // These views exists only in some of our group views: they are present
>          // for the home panel groups and not for the tabs panel groups.
>          // Therefore, we must handle null.

I think this is no longer the case.  We don't use these views or this adapter anywhere but in this panel, now.  Another ticket, perhaps.

Comment 24

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8549984 [details] [diff] [review]
part2.patch

Review of attachment 8549984 [details] [diff] [review]:
-----------------------------------------------------------------

Straight rubber stamp.  This is why I wanted you to divide the patch series.  Bravo!

Comment 25

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8549986 [details] [diff] [review]
part3.patch

Review of attachment 8549986 [details] [diff] [review]:
-----------------------------------------------------------------

I have nits but you're right -- this is Awesome split plane for tablets :)  This is r+ but I want to see it once more before it lands.

::: mobile/android/base/home/RemoteTabsSplitPlaneFragment.java
@@ +28,5 @@
> +import java.util.EnumSet;
> +import java.util.List;
> +
> +/**
> + * Fragment that displays other devices and tabs from them in a separate <code>ListView<code>.

nit: ... in two separate ListView instances.

@@ +34,5 @@
> + * This is intended to be used in landscape mode on tablets.
> + */
> +public class RemoteTabsSplitPlaneFragment extends RemoteTabsBaseFragment {
> +    // Logging tag name.
> +    private static final String LOGTAG = "GeckoRemoteTabsSplitPlaneList";

Make sure this is less than... 23 characters? 5 + 6 + 4 + 5 + 5 + 4 = 29 is too long.  And it should be ...Fragment.  GeckoSplitTabsPane might be enough?

@@ +36,5 @@
> +public class RemoteTabsSplitPlaneFragment extends RemoteTabsBaseFragment {
> +    // Logging tag name.
> +    private static final String LOGTAG = "GeckoRemoteTabsSplitPlaneList";
> +
> +    private ArrayAdapter<RemoteTab> tabsAdapter;

The superclass uses the convention mMember, so follow suit throughout, please. mTabsAdapter, mClientsAdapter, etc.

@@ +39,5 @@
> +
> +    private ArrayAdapter<RemoteTab> tabsAdapter;
> +    private ArrayAdapter<RemoteClient> clientsAdapter;
> +
> +    // DatasetObserver for the expandable list adapter

nit: trailing period.

@@ +42,5 @@
> +
> +    // DatasetObserver for the expandable list adapter
> +    private DataSetObserver observer;
> +
> +    // The view shown by the fragment.

nit: views

@@ +89,5 @@
> +            public void onItemClick(AdapterView<?> adapter, View view, int position, long id) {
> +                final RemoteClient client = (RemoteClient) adapter.getItemAtPosition(position);
> +                if (client != null) {
> +                    // Mark the clients  as selected, without flipping its expanded state.
> +                    //sState.setClientAsSelected(client.guid);

What's happening here?

@@ +141,5 @@
> +        mAdapter.unregisterDataSetObserver(observer);
> +        observer = null;
> +        mAdapter = null;
> +
> +        if (mSyncStatusListener != null) {

This should be in the super, since the listener is owned by super.

@@ +151,5 @@
> +    @Override
> +    public void onActivityCreated(Bundle savedInstanceState) {
> +        super.onActivityCreated(savedInstanceState);
> +
> +        // There is an unfortunate interaction between ExpandableListViews and

These comments are *definitely* out of date, since there's no ELV here at all.

@@ +261,5 @@
> +            tabsAdapter.clear();
> +
> +            for (int i = 0; i < mAdapter.getGroupCount(); i++) {
> +                clientsAdapter.add((RemoteClient) mAdapter.getGroup(i));
> +            }

Explain why we're not doing something like notifyDataSetChanged, like below.  Based on http://developer.android.com/reference/android/widget/ArrayAdapter.html#setNotifyOnChange%28boolean%29, it looks like clear/add already do the notification for you, so you don't need the nDSC below.

@@ +322,5 @@
> +                view = convertView;
> +            } else {
> +                final LayoutInflater inflater = LayoutInflater.from(context);
> +                view = inflater.inflate(resource, parent, false);
> +                GroupViewHolder holder = new GroupViewHolder(view);

nit: final.

@@ +326,5 @@
> +                GroupViewHolder holder = new GroupViewHolder(view);
> +                view.setTag(holder);
> +            }
> +
> +            adapter.updateClientsItemView(false, context, view, getItem(position));

Ah, that's why you had the public method before.  Very good.

Comment 26

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8550001 [details] [diff] [review]
part4.patch

Review of attachment 8550001 [details] [diff] [review]:
-----------------------------------------------------------------

Update commit message:

"""
Bug 1063742 - Part 4: Implement clicking on items in split panes.

Also tracks last selected client.
"""

This looks good, I'll look again before testing locally.

::: mobile/android/base/home/HomeListView.java
@@ +90,5 @@
>  
>              mContextMenuInfo = mContextMenuInfoFactory.makeInfoForCursor(view, position, id, cursor);
>              return showContextMenuForChild(HomeListView.this);
>  
> +        } else if(mContextMenuInfoFactory instanceof HomeContextMenuInfo.ListFactory) {

nit: if(

@@ +93,5 @@
>  
> +        } else if(mContextMenuInfoFactory instanceof HomeContextMenuInfo.ListFactory) {
> +            mContextMenuInfo = ((HomeContextMenuInfo.ListFactory) mContextMenuInfoFactory).makeInfoForAdapter(view, position, id, getAdapter());
> +            return showContextMenuForChild(HomeListView.this);
> +        }else {

nit: } else {

::: mobile/android/base/home/RemoteTabsExpandableListState.java
@@ +43,5 @@
>      // "not present" means "shown".
>      // Only accessed from the UI thread.
>      protected final Set<String> hiddenClients;
>  
> +    // Synchronized by the state instance. The user selected client guid.

The *last* user selected client?

::: mobile/android/base/home/RemoteTabsSplitPlaneFragment.java
@@ +96,5 @@
>                          tabsAdapter.add(tab);
>                      }
> +
> +                    // Notify data has changed for both clients and tabs adapter, this will update selected client state and tabs list.
> +                    clientsAdapter.notifyDataSetChanged();

Clear and add already do this for tabs.  And clients hasn't changed, right?

@@ +187,5 @@
>          // Initialize adapter
>          mAdapter = new RemoteTabsExpandableListAdapter(R.layout.home_remote_tabs_group, R.layout.home_remote_tabs_child, null, false);
>  
>          tabsAdapter = new RemoteTabsAdapter(getActivity(), R.layout.home_remote_tabs_child);
> +        tabsAdapter.setNotifyOnChange(false);

Oh ho!  This is interesting, you're doing this manually.  I will test this carefully on the final pass.

@@ +270,2 @@
>              for (int i = 0; i < mAdapter.getGroupCount(); i++) {
> +                RemoteClient client = (RemoteClient) mAdapter.getGroup(i);

This is an awkward way to do this.  There's no need to copy the tab elements, is there?  Is it reasonable to just keep a reference, and if there are no clients at all, make a new empty list?

@@ +287,5 @@
> +            for (RemoteTab tab : visibleTabs) {
> +                tabsAdapter.add(tab);
> +            }
> +
> +            // Update the selected clients;

nit: client.

@@ -295,5 @@
>                  view = (TwoLinePageRow) inflater.inflate(resource, parent, false);
>              }
>  
>              final RemoteTab tab = getItem(position);
> -            view.setShowIcons(false);

Stuff like this freaks me out.  This controls the little "switch to tab icon" and the bookmark star icon.  Why don't we use this anymore?

@@ +354,5 @@
>                  GroupViewHolder holder = new GroupViewHolder(view);
>                  view.setTag(holder);
>              }
>  
> +            RemoteClient client = getItem(position);

nit: final

And maybe extract:

final boolean isSelected = client.guid.equals...

Comment 27

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8550002 [details] [diff] [review]
part5.patch

Review of attachment 8550002 [details] [diff] [review]:
-----------------------------------------------------------------

Bravo!

::: mobile/android/base/home/RemoteTabsPanel.java
@@ +205,5 @@
>              return mFallbackFragment;
>          }
>  
>          Fragment fragment = mFragmentCache.get(actionNeeded);
> +        if (fragment == null || (HardwareUtils.isTablet() && actionNeeded == Action.None)) {

A little comment explaining the case you're avoiding would be nice.

Comment 28

[Vivek Balakrishnan[:vivek]]

Attachment: part1.patch

New patch with nits addressed

Comment 29

[Vivek Balakrishnan[:vivek]]

Attachment: part2.patch

Part 2 of the patch for extracting ELA common functionality to a base class

Comment 30

[Vivek Balakrishnan[:vivek]]

Attachment: part3.patch

Remote tabs split plane fragment impl

I've have added comments to clearly identify place holder and todo that I'm addressing in successive patch.

Also, the adapter have setNotifyOnChange set to false and notifyDataSetChanged is called manually when required

Comment 31

[Vivek Balakrishnan[:vivek]]

Attachment: part4.patch

Same as the previous version with nits addressed and comments added

Comment 32

[Vivek Balakrishnan[:vivek]]

Attachment: part5.patch

Final part to handle orientation changes

Comment 33

[Vivek Balakrishnan[:vivek]]

Attachment: part3.patch

Missed the placeholder TODO: in the earlier version

Comment 34

[Vivek Balakrishnan[:vivek]]

Attachment: part4.patch

Rebased on top of latest part3.patch

Comment 35

[:Margaret Leibovic]

Comment on attachment 8552176 [details] [diff] [review]
part1.patch

Review of attachment 8552176 [details] [diff] [review]:
-----------------------------------------------------------------

I trust nalexander to cover the details here, but the direction looks good to me.

Comment 36

[:Margaret Leibovic]

Comment on attachment 8552177 [details] [diff] [review]
part2.patch

Review of attachment 8552177 [details] [diff] [review]:
-----------------------------------------------------------------

Nit: Every commit should have a description of what it's doing. That would make it easier to see what I'm reviewing :)

But as nalexander mentioned, splitting this mechanical refactor our into its own commit is extremely helpful!

Comment 37

[:Margaret Leibovic]

Comment on attachment 8552195 [details] [diff] [review]
part3.patch

Review of attachment 8552195 [details] [diff] [review]:
-----------------------------------------------------------------

Same story: the direction here looks good, I'll leave the details to nalexander :)

Comment 38

[:Margaret Leibovic]

Comment on attachment 8552180 [details] [diff] [review]
part5.patch

Review of attachment 8552180 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/RemoteTabsPanel.java
@@ +83,5 @@
> +            load();
> +            return;
> +        }
> +
> +        super.loadIfVisible();

It feels a bit fragile to me to be overriding loadIfVisible like this, since the super method will also call load(). I was going to suggest not calling the super method, but then we would never set the mIsLoaded member variable. 

I can't immediately think of a good solution here, but I do think we should avoid calling load() twice if we don't need to.

Comment 39

[Nick Alexander :nalexander [he/him]]

vivek: all of these patches look fine to me.  I do want to test them on my device, so I'm holding off setting the flags until I have run them on my tablets.  Thanks for your patience.  Good work!

Comment 40

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/145b1a56988e
https://hg.mozilla.org/integration/fx-team/rev/ae31ef6afa3f
https://hg.mozilla.org/integration/fx-team/rev/02cd1264274e
https://hg.mozilla.org/integration/fx-team/rev/70161fcd4087
https://hg.mozilla.org/integration/fx-team/rev/62e12e630f28

Comment 41

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #39)
> vivek: all of these patches look fine to me.  I do want to test them on my
> device, so I'm holding off setting the flags until I have run them on my
> tablets.  Thanks for your patience.  Good work!

vivek: bravo!  Landed as you left it.  I've filed some follow-ups and improvements, but this was a big ticket and you did a great job.  Let's see how it goes in the wild!

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/145b1a56988e
https://hg.mozilla.org/mozilla-central/rev/ae31ef6afa3f
https://hg.mozilla.org/mozilla-central/rev/02cd1264274e
https://hg.mozilla.org/mozilla-central/rev/70161fcd4087
https://hg.mozilla.org/mozilla-central/rev/62e12e630f28

Comment 43

[Aaron Train [:aaronmt]]

This has yielded a crasher in bug 1131635.

Comment 44

[Aaron Train [:aaronmt]]

Another crasher in bug 1132046.