Bug 796187 - Send tab: usability tweaks. r=nalexander

mobile/android/base/resources/layout/sync_list_item.xml

@@ -4,10 +4,11 @@
    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
 
 <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
-    android:orientation="horizontal"
+    android:gravity="center_vertical"
     android:layout_width="fill_parent"
     android:layout_height="wrap_content"
-    android:gravity="center_vertical"
+    android:minHeight="?android:attr/listPreferredItemHeight"
+    android:orientation="horizontal"
     android:padding="5dp" >
 
     <ImageView

mobile/android/base/resources/layout/sync_send_tab.xml

@@ -10,6 +10,26 @@
       style="@style/SyncTop"
       android:text="@string/sync_title_send_tab" />
 
+  <LinearLayout
+    android:layout_width="fill_parent"
+    android:layout_height="wrap_content"
+    android:orientation="vertical"
+    android:padding="@dimen/SyncSpace" >
+
+    <TextView
+      android:id="@+id/title"
+      android:layout_width="fill_parent"
+      android:layout_height="wrap_content"
+      android:textAppearance="?android:attr/textAppearanceMedium"
+      android:text="@string/sync_title_send_tab" />
+    <TextView
+      android:id="@+id/uri"
+      android:layout_width="fill_parent"
+      android:layout_height="wrap_content"
+      android:textAppearance="?android:attr/textAppearanceSmall"
+      android:text="@string/sync_title_send_tab" />
+  </LinearLayout>
+
   <ListView
     android:id="@+id/device_list"
     style="@style/SyncMiddle"

mobile/android/base/sync/setup/activities/ClientRecordArrayAdapter.java

@@ -1,6 +1,11 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
 package org.mozilla.gecko.sync.setup.activities;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 
 import org.mozilla.gecko.R;
@@ -15,19 +20,63 @@ import android.widget.CheckBox;
 import android.widget.ImageView;
 import android.widget.TextView;
 
-public class ClientRecordArrayAdapter extends ArrayAdapter<Object> {
+public class ClientRecordArrayAdapter extends ArrayAdapter<ClientRecord> {
   public static final String LOG_TAG = "ClientRecArrayAdapter";
-  private ClientRecord[] clientRecordList;
+
   private boolean[] checkedItems;
   private int numCheckedGUIDs;
   private SendTabActivity sendTabActivity;
 
-  public ClientRecordArrayAdapter(Context context, int textViewResourceId,
-      ClientRecord[] clientRecordList) {
-    super(context, textViewResourceId, clientRecordList);
+  public ClientRecordArrayAdapter(Context context,
+                                  int textViewResourceId) {
+    super(context, textViewResourceId, new ArrayList<ClientRecord>());
+    this.checkedItems = new boolean[0];
     this.sendTabActivity = (SendTabActivity) context;
-    this.clientRecordList = clientRecordList;
-    this.checkedItems = new boolean[clientRecordList.length];
+  }
+
+  public synchronized void setClientRecordList(final Collection<ClientRecord> clientRecordList) {
+    this.clear();
+    this.checkedItems = new boolean[clientRecordList.size()];
+    this.numCheckedGUIDs = 0;
+    for (ClientRecord clientRecord : clientRecordList) {
+      this.add(clientRecord);
+    }
+    this.notifyDataSetChanged();
+  }
+
+  /**
+   * If we have only a single client record in the list, mark it as checked.
+   */
+  public synchronized void checkItem(final int position, boolean checked) throws ArrayIndexOutOfBoundsException {
+    if (position < 0 ||
+        position >= checkedItems.length) {
+      throw new ArrayIndexOutOfBoundsException(position);
+    }
+
+    if (setRowChecked(position, true)) {
+      this.notifyDataSetChanged();
+    }
+  }
+
+  /**
+   * Set the specified row to the specified checked state.
+   * @param position an index.
+   * @param checked whether the checkbox should be checked.
+   * @return <code>true</code> if the state changed, <code>false</code> if the
+   *         box was already in the requested state.
+   */
+  protected synchronized boolean setRowChecked(int position, boolean checked) {
+    boolean current = checkedItems[position];
+    if (current == checked) {
+      return false;
+    }
+
+    checkedItems[position] = checked;
+    numCheckedGUIDs += checked ? 1 : -1;
+    if (numCheckedGUIDs <= 0) {
+      sendTabActivity.enableSend(numCheckedGUIDs > 0);
+    }
+    return true;
   }
 
   @Override
@@ -42,9 +91,10 @@ public class ClientRecordArrayAdapter extends ArrayAdapter<Object> {
       row.setBackgroundResource(android.R.drawable.menuitem_background);
     }
 
-    final ClientRecord clientRecord = clientRecordList[position];
+    final ClientRecord clientRecord = this.getItem(position);
     ImageView clientType = (ImageView) row.findViewById(R.id.img);
     TextView clientName = (TextView) row.findViewById(R.id.client_name);
+
     // Set up checkbox and restore stored state.
     CheckBox checkbox = (CheckBox) row.findViewById(R.id.check);
     checkbox.setChecked(checkedItems[position]);
@@ -56,37 +106,29 @@ public class ClientRecordArrayAdapter extends ArrayAdapter<Object> {
     row.setOnClickListener(new OnClickListener() {
       @Override
       public void onClick(View view) {
-        CheckBox item = (CheckBox) view.findViewById(R.id.check);
+        final CheckBox item = (CheckBox) view.findViewById(R.id.check);
 
-        // Update the checked item, both in the UI and in the checkedItems array.
-        boolean newCheckedValue = !item.isChecked();
-        item.setChecked(newCheckedValue);
-        checkedItems[position] = newCheckedValue;
-
-        numCheckedGUIDs += newCheckedValue ? 1 : -1;
-        if (numCheckedGUIDs <= 0) {
-          sendTabActivity.enableSend(false);
-          return;
-        }
-        sendTabActivity.enableSend(true);
+        // Update the checked item, both in the UI and in our internal state.
+        final boolean checked = !item.isChecked();    // Because it hasn't happened yet.
+        item.setChecked(checked);
+        setRowChecked(position, checked);
       }
-
     });
 
     return row;
   }
 
-  public List<String> getCheckedGUIDs() {
+  public synchronized List<String> getCheckedGUIDs() {
     final List<String> guids = new ArrayList<String>();
     for (int i = 0; i < checkedItems.length; i++) {
       if (checkedItems[i]) {
-        guids.add(clientRecordList[i].guid);
+        guids.add(this.getItem(i).guid);
       }
     }
     return guids;
   }
 
-  public int getNumCheckedGUIDs() {
+  public synchronized int getNumCheckedGUIDs() {
     return numCheckedGUIDs;
   }
 

mobile/android/base/sync/setup/activities/SendTabActivity.java

@@ -4,15 +4,19 @@
 
 package org.mozilla.gecko.sync.setup.activities;
 
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
 
 import org.mozilla.gecko.R;
 import org.mozilla.gecko.sync.CommandProcessor;
 import org.mozilla.gecko.sync.CommandRunner;
-import org.mozilla.gecko.sync.SyncConstants;
 import org.mozilla.gecko.sync.GlobalSession;
 import org.mozilla.gecko.sync.Logger;
 import org.mozilla.gecko.sync.SyncConfiguration;
+import org.mozilla.gecko.sync.SyncConstants;
 import org.mozilla.gecko.sync.repositories.NullCursorException;
 import org.mozilla.gecko.sync.repositories.android.ClientsDatabaseAccessor;
 import org.mozilla.gecko.sync.repositories.domain.ClientRecord;
@@ -31,6 +35,7 @@ import android.os.AsyncTask;
 import android.os.Bundle;
 import android.view.View;
 import android.widget.ListView;
+import android.widget.TextView;
 import android.widget.Toast;
 
 public class SendTabActivity extends Activity {
@@ -38,10 +43,80 @@ public class SendTabActivity extends Activity {
   private ClientRecordArrayAdapter arrayAdapter;
   private AccountManager accountManager;
   private Account localAccount;
+  private SendTabData sendTabData;
 
   @Override
   public void onCreate(Bundle savedInstanceState) {
     super.onCreate(savedInstanceState);
+
+    Intent intent = getIntent();
+    if (intent == null) {
+      Logger.warn(LOG_TAG, "intent was null; aborting without sending tab.");
+      notifyAndFinish(false);
+      return;
+    }
+
+    Bundle extras = intent.getExtras();
+    if (extras == null) {
+      Logger.warn(LOG_TAG, "extras was null; aborting without sending tab.");
+      notifyAndFinish(false);
+      return;
+    }
+
+    sendTabData = SendTabData.fromBundle(extras);
+    if (sendTabData == null) {
+      Logger.warn(LOG_TAG, "send tab data was null; aborting without sending tab.");
+      notifyAndFinish(false);
+      return;
+    }
+
+    if (sendTabData.uri == null) {
+      Logger.warn(LOG_TAG, "uri was null; aborting without sending tab.");
+      notifyAndFinish(false);
+      return;
+    }
+
+    if (sendTabData.title == null) {
+      Logger.warn(LOG_TAG, "title was null; ignoring and sending tab anyway.");
+    }
+  }
+
+  /**
+   * Ensure that the view's list of clients is backed by a recently populated
+   * array adapter. But only once, so we don't end up blowing away your selections
+   * just because you got a text message.
+   */
+  protected synchronized void ensureClientList(final Context context,
+                                               final ListView listview) {
+    if (arrayAdapter != null) {
+      Logger.debug(LOG_TAG, "Already have an array adapter for client lists.");
+      listview.setAdapter(arrayAdapter);
+      return;
+    }
+
+    arrayAdapter = new ClientRecordArrayAdapter(context, R.layout.sync_list_item);
+    listview.setAdapter(arrayAdapter);
+
+    // Fetching the client list hits the clients database, so we spin this onto
+    // a background task.
+    new AsyncTask<Void, Void, Collection<ClientRecord>>() {
+
+      @Override
+      protected Collection<ClientRecord> doInBackground(Void... params) {
+        return getOtherClients();
+      }
+
+      @Override
+      protected void onPostExecute(final Collection<ClientRecord> clientArray) {
+        // We're allowed to update the UI from here.
+
+        Logger.debug(LOG_TAG, "Got " + clientArray.size() + " clients.");
+        arrayAdapter.setClientRecordList(clientArray);
+        if (clientArray.size() == 1) {
+          arrayAdapter.checkItem(0, true);
+        }
+      }
+    }.execute();
   }
 
   @Override
@@ -60,23 +135,13 @@ public class SendTabActivity extends Activity {
     listview.setChoiceMode(ListView.CHOICE_MODE_MULTIPLE);
     enableSend(false);
 
-    // Fetching the client list hits the clients database, so we spin this onto
-    // a background task.
-    final Context context = this;
-    new AsyncTask<Void, Void, ClientRecord[]>() {
+    ensureClientList(this, listview);
 
-      @Override
-      protected ClientRecord[] doInBackground(Void... params) {
-        return getClientArray();
-      }
+    TextView textView = (TextView) findViewById(R.id.title);
+    textView.setText(sendTabData.title);
 
-      @Override
-      protected void onPostExecute(final ClientRecord[] clientArray) {
-        // We're allowed to update the UI from here.
-        arrayAdapter = new ClientRecordArrayAdapter(context, R.layout.sync_list_item, clientArray);
-        listview.setAdapter(arrayAdapter);
-      }
-    }.execute();
+    textView = (TextView) findViewById(R.id.uri);
+    textView.setText(sendTabData.uri);
   }
 
   private static void registerDisplayURICommand() {
@@ -126,25 +191,6 @@ public class SendTabActivity extends Activity {
 
   public void sendClickHandler(View view) {
     Logger.info(LOG_TAG, "Send was clicked.");
-    Bundle extras = this.getIntent().getExtras();
-    if (extras == null) {
-      Logger.warn(LOG_TAG, "extras was null; aborting without sending tab.");
-      notifyAndFinish(false);
-      return;
-    }
-
-    final SendTabData sendTabData = SendTabData.fromBundle(extras);
-
-    if (sendTabData.title == null) {
-      Logger.warn(LOG_TAG, "title was null; ignoring and sending tab anyway.");
-    }
-
-    if (sendTabData.uri == null) {
-      Logger.warn(LOG_TAG, "uri was null; aborting without sending tab.");
-      notifyAndFinish(false);
-      return;
-    }
-
     final List<String> remoteClientGuids = arrayAdapter.getCheckedGUIDs();
 
     if (remoteClientGuids == null) {
@@ -214,11 +260,10 @@ public class SendTabActivity extends Activity {
     sendButton.setClickable(shouldEnable);
   }
 
-  protected ClientRecord[] getClientArray() {
+  protected Map<String, ClientRecord> getClients() {
     ClientsDatabaseAccessor db = new ClientsDatabaseAccessor(this.getApplicationContext());
-
     try {
-      return db.fetchAllClients().values().toArray(new ClientRecord[0]);
+      return db.fetchAllClients();
     } catch (NullCursorException e) {
       Logger.warn(LOG_TAG, "NullCursorException while populating device list.", e);
       return null;
@@ -226,4 +271,20 @@ public class SendTabActivity extends Activity {
       db.close();
     }
   }
+
+  /**
+   * @return a collection of client records, excluding our own.
+   */
+  protected Collection<ClientRecord> getOtherClients() {
+    final Map<String, ClientRecord> all = getClients();
+    final ArrayList<ClientRecord> out = new ArrayList<ClientRecord>(all.size());
+    final String ourGUID = getAccountGUID();
+    for (Entry<String, ClientRecord> entry : all.entrySet()) {
+      if (ourGUID.equals(entry.getKey())) {
+        continue;
+      }
+      out.add(entry.getValue());
+    }
+    return out;
+  }
 }