Merge pull request #160 from kruton/state-race

Fix race condition in updating the HostListActivity with current host statuses

4 files changed, 124 insertions, 43 deletions

diff --git a/app/src/androidTest/java/org/connectbot/StartupTest.java b/app/src/androidTest/java/org/connectbot/StartupTest.java
index ea5b584..0eb9c11 100644
--- a/app/src/androidTest/java/org/connectbot/StartupTest.java
+++ b/app/src/androidTest/java/org/connectbot/StartupTest.java
@@ -13,12 +13,10 @@ import org.junit.runner.RunWith;
 import android.content.Intent;
 import android.support.annotation.NonNull;
 import android.support.test.InstrumentationRegistry;
-import android.support.test.espresso.ViewAssertion;
 import android.support.test.espresso.intent.Intents;
 import android.support.test.espresso.matcher.BoundedMatcher;
 import android.support.test.rule.ActivityTestRule;
 import android.support.test.runner.AndroidJUnit4;
-import android.view.KeyEvent;
 import android.view.View;
 import android.widget.ImageView;
 
@@ -28,7 +26,7 @@ import static android.support.test.espresso.action.ViewActions.click;
 import static android.support.test.espresso.action.ViewActions.closeSoftKeyboard;
 import static android.support.test.espresso.action.ViewActions.longClick;
 import static android.support.test.espresso.action.ViewActions.pressBack;
-import static android.support.test.espresso.action.ViewActions.pressKey;
+import static android.support.test.espresso.action.ViewActions.pressImeActionButton;
 import static android.support.test.espresso.action.ViewActions.pressMenuKey;
 import static android.support.test.espresso.action.ViewActions.typeText;
 import static android.support.test.espresso.assertion.ViewAssertions.matches;
@@ -64,7 +62,7 @@ public class StartupTest {
 
 		// Make sure we're still connected.
 		onData(withHostNickname("Local")).inAdapterView(withId(android.R.id.list))
-				.check(hostConnected())
+				.check(matches(hostConnected()))
 				.perform(longClick());
 
 		// Click on the disconnect context menu item.
@@ -72,7 +70,7 @@ public class StartupTest {
 
 		// Now make sure we're disconnected.
 		onData(withHostNickname("Local")).inAdapterView(withId(android.R.id.list))
-				.check(hostDisconnected());
+				.check(matches(hostDisconnected()));
 	}
 
 	@Test
@@ -86,7 +84,7 @@ public class StartupTest {
 
 		// Now make sure we're disconnected.
 		onData(withHostNickname("Local")).inAdapterView(withId(android.R.id.list))
-				.check(hostDisconnected());
+				.check(matches(hostDisconnected()));
 	}
 
 	private void startNewLocalConnection() {
@@ -95,9 +93,12 @@ public class StartupTest {
 		onView(withId(R.id.front_quickconnect)).perform(typeText("Local"));
 
 		Intents.init();
-		onView(withId(R.id.front_quickconnect)).perform(pressKey(KeyEvent.KEYCODE_ENTER));
-		intended(hasComponent(ConsoleActivity.class.getName()));
-		Intents.release();
+		try {
+			onView(withId(R.id.front_quickconnect)).perform(pressImeActionButton());
+			intended(hasComponent(ConsoleActivity.class.getName()));
+		} finally {
+			Intents.release();
+		}
 
 		onView(withId(R.id.console_flip)).check(matches(
 				hasDescendant(allOf(isDisplayed(), withId(R.id.terminal_view)))));
@@ -148,14 +149,14 @@ public class StartupTest {
 	}
 
 	@NonNull
-	private ViewAssertion hostDisconnected() {
-		return matches(hasDescendant(allOf(withId(android.R.id.icon),
-				withDrawableState(android.R.attr.state_expanded))));
+	private Matcher<View> hostDisconnected() {
+		return hasDescendant(allOf(withId(android.R.id.icon),
+				withDrawableState(android.R.attr.state_expanded)));
 	}
 
 	@NonNull
-	private ViewAssertion hostConnected() {
-		return matches(hasDescendant(allOf(withId(android.R.id.icon),
-				withDrawableState(android.R.attr.state_checked))));
+	private Matcher<View> hostConnected() {
+		return hasDescendant(allOf(withId(android.R.id.icon),
+				withDrawableState(android.R.attr.state_checked)));
 	}
 }
diff --git a/app/src/main/java/org/connectbot/HostListActivity.java b/app/src/main/java/org/connectbot/HostListActivity.java
index 2d8e882..1fe634e 100644
--- a/app/src/main/java/org/connectbot/HostListActivity.java
+++ b/app/src/main/java/org/connectbot/HostListActivity.java
@@ -20,6 +20,7 @@ package org.connectbot;
 import java.util.List;
 
 import org.connectbot.bean.HostBean;
+import org.connectbot.service.OnHostStatusChangedListener;
 import org.connectbot.service.TerminalBridge;
 import org.connectbot.service.TerminalManager;
 import org.connectbot.transport.TransportFactory;
@@ -32,17 +33,15 @@ import android.content.ComponentName;
 import android.content.Context;
 import android.content.DialogInterface;
 import android.content.Intent;
+import android.content.Intent.ShortcutIconResource;
 import android.content.ServiceConnection;
 import android.content.SharedPreferences;
-import android.content.Intent.ShortcutIconResource;
 import android.content.SharedPreferences.Editor;
 import android.content.res.ColorStateList;
 import android.net.Uri;
 import android.os.Build;
 import android.os.Bundle;
-import android.os.Handler;
 import android.os.IBinder;
-import android.os.Message;
 import android.preference.PreferenceManager;
 import android.text.format.DateUtils;
 import android.util.Log;
@@ -51,19 +50,20 @@ import android.view.KeyEvent;
 import android.view.LayoutInflater;
 import android.view.Menu;
 import android.view.MenuItem;
-import android.view.View;
-import android.view.ViewGroup;
 import android.view.MenuItem.OnMenuItemClickListener;
+import android.view.View;
 import android.view.View.OnKeyListener;
+import android.view.ViewGroup;
 import android.widget.AdapterView;
+import android.widget.AdapterView.OnItemClickListener;
 import android.widget.ArrayAdapter;
+import android.widget.BaseAdapter;
 import android.widget.ImageView;
 import android.widget.ListView;
 import android.widget.Spinner;
 import android.widget.TextView;
-import android.widget.AdapterView.OnItemClickListener;
 
-public class HostListActivity extends ListActivity {
+public class HostListActivity extends ListActivity implements OnHostStatusChangedListener {
 	public final static String TAG = "CB.HostListActivity";
 	public static final String DISCONNECT_ACTION = "org.connectbot.action.DISCONNECT";
 
@@ -96,13 +96,6 @@ public class HostListActivity extends ListActivity {
 	 */
 	private boolean closeOnDisconnectAll = true;
 
-	protected Handler updateHandler = new Handler() {
-		@Override
-		public void handleMessage(Message msg) {
-			HostListActivity.this.updateList();
-		}
-	};
-
 	private ServiceConnection connection = new ServiceConnection() {
 		public void onServiceConnected(ComponentName className, IBinder service) {
 			bound = ((TerminalManager.TerminalBinder) service).getService();
@@ -110,12 +103,16 @@ public class HostListActivity extends ListActivity {
 			// update our listview binder to find the service
 			HostListActivity.this.updateList();
 
+			bound.registerOnHostStatusChangedListener(HostListActivity.this);
+
 			if (waitingForDisconnectAll) {
 				disconnectAll();
 			}
 		}
 
 		public void onServiceDisconnected(ComponentName className) {
+			bound.unregisterOnHostStatusChangedListener(HostListActivity.this);
+
 			bound = null;
 			HostListActivity.this.updateList();
 		}
@@ -161,9 +158,6 @@ public class HostListActivity extends ListActivity {
 		closeOnDisconnectAll = waitingForDisconnectAll && closeOnDisconnectAll;
 	}
 
-	/* (non-Javadoc)
-	 * @see android.app.Activity#onNewIntent(android.content.Intent)
-	 */
 	@Override
 	protected void onNewIntent(Intent intent) {
 		super.onNewIntent(intent);
@@ -370,7 +364,6 @@ public class HostListActivity extends ListActivity {
 		connect.setOnMenuItemClickListener(new OnMenuItemClickListener() {
 			public boolean onMenuItemClick(MenuItem item) {
 				bridge.dispatchDisconnect(true);
-				updateHandler.sendEmptyMessage(-1);
 				return true;
 			}
 		});
@@ -410,7 +403,7 @@ public class HostListActivity extends ListActivity {
 									bridge.dispatchDisconnect(true);
 
 								hostdb.deleteHost(host);
-								updateHandler.sendEmptyMessage(-1);
+								updateList();
 							}
 						})
 						.setNegativeButton(R.string.delete_neg, null).create().show();
@@ -433,7 +426,6 @@ public class HostListActivity extends ListActivity {
 			.setPositiveButton(R.string.disconnect_all_pos, new DialogInterface.OnClickListener() {
 				public void onClick(DialogInterface dialog, int which) {
 					bound.disconnectAll(true, false);
-					updateHandler.sendEmptyMessage(-1);
 					waitingForDisconnectAll = false;
 
 					// Clear the intent so that the activity can be relaunched without closing.
@@ -511,8 +503,14 @@ public class HostListActivity extends ListActivity {
 		this.setListAdapter(adapter);
 	}
 
-	class HostAdapter extends ArrayAdapter<HostBean> {
-		private List<HostBean> hosts;
+	@Override
+	public void onHostStatusChanged() {
+		updateList();
+	}
+
+	static class HostAdapter extends BaseAdapter {
+		private final LayoutInflater inflater;
+		private final List<HostBean> hosts;
 		private final TerminalManager manager;
 		private final ColorStateList red, green, blue;
 
@@ -525,8 +523,7 @@ public class HostListActivity extends ListActivity {
 		}
 
 		public HostAdapter(Context context, List<HostBean> hosts, TerminalManager manager) {
-			super(context, R.layout.item_host, hosts);
-
+			this.inflater = LayoutInflater.from(context);
 			this.hosts = hosts;
 			this.manager = manager;
 
@@ -539,7 +536,7 @@ public class HostListActivity extends ListActivity {
 		 * Check if we're connected to a terminal with the given host.
 		 */
 		private int getConnectedState(HostBean host) {
-			// always disconnected if we dont have backend service
+			// always disconnected if we don't have backend service
 			if (this.manager == null)
 				return STATE_UNKNOWN;
 
@@ -553,6 +550,32 @@ public class HostListActivity extends ListActivity {
 		}
 
 		@Override
+		public int getCount() {
+			return hosts.size();
+		}
+
+		@Override
+		public Object getItem(int position) {
+			return hosts.get(position);
+		}
+
+		/**
+		 * Use the database's IDs for the host.
+		 */
+		@Override
+		public long getItemId(int position) {
+			return hosts.get(position).getId();
+		}
+
+		/**
+		 * Since we're using the database's IDs, they're unchanging.
+		 */
+		@Override
+		public boolean hasStableIds() {
+			return true;
+		}
+
+		@Override
 		public View getView(int position, View convertView, ViewGroup parent) {
 			ViewHolder holder;
 
@@ -566,8 +589,9 @@ public class HostListActivity extends ListActivity {
 				holder.icon = (ImageView) convertView.findViewById(android.R.id.icon);
 
 				convertView.setTag(holder);
-			} else
+			} else {
 				holder = (ViewHolder) convertView.getTag();
+			}
 
 			HostBean host = hosts.get(position);
 			if (host == null) {
@@ -609,8 +633,8 @@ public class HostListActivity extends ListActivity {
 				holder.caption.setTextColor(chosen);
 			} else {
 				// selected, so revert back to default black text
-				holder.nickname.setTextAppearance(context, android.R.attr.textAppearanceLarge);
-				holder.caption.setTextAppearance(context, android.R.attr.textAppearanceSmall);
+				holder.nickname.setTextAppearance(context, android.R.style.TextAppearance_Large);
+				holder.caption.setTextAppearance(context, android.R.style.TextAppearance_Small);
 			}
 
 			CharSequence nice = context.getString(R.string.bind_never);
diff --git a/app/src/main/java/org/connectbot/service/OnHostStatusChangedListener.java b/app/src/main/java/org/connectbot/service/OnHostStatusChangedListener.java
new file mode 100644
index 0000000..0d6ced7
--- /dev/null
+++ b/app/src/main/java/org/connectbot/service/OnHostStatusChangedListener.java
@@ -0,0 +1,26 @@
+/*
+ * ConnectBot: simple, powerful, open-source SSH client for Android
+ * Copyright 2015 Kenny Root, Jeffrey Sharkey
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.connectbot.service;
+
+/**
+ * Used to notify interested parties when a {@link TerminalBridge} has changed materially
+ * changed status (e.g., connected, disconnected, name changed, etc).
+ */
+public interface OnHostStatusChangedListener {
+	public void onHostStatusChanged();
+}
diff --git a/app/src/main/java/org/connectbot/service/TerminalManager.java b/app/src/main/java/org/connectbot/service/TerminalManager.java
index a15dff0..5416ddb 100644
--- a/app/src/main/java/org/connectbot/service/TerminalManager.java
+++ b/app/src/main/java/org/connectbot/service/TerminalManager.java
@@ -81,6 +81,8 @@ public class TerminalManager extends Service implements BridgeDisconnectedListen
 
 	public BridgeDisconnectedListener disconnectListener = null;
 
+	private final ArrayList<OnHostStatusChangedListener> hostStatusChangedListeners = new ArrayList<>();
+
 	public Map<String, KeyHolder> loadedKeypairs = new HashMap<String, KeyHolder>();
 
 	public Resources res;
@@ -252,6 +254,8 @@ public class TerminalManager extends Service implements BridgeDisconnectedListen
 		// also update database with new connected time
 		touchHost(host);
 
+		notifyHostStatusChanged();
+
 		return bridge;
 	}
 
@@ -354,6 +358,8 @@ public class TerminalManager extends Service implements BridgeDisconnectedListen
 			disconnected.add(bridge.host);
 		}
 
+		notifyHostStatusChanged();
+
 		if (shouldHideRunningNotification) {
 			ConnectionNotifier.getInstance().hideRunningNotification(this);
 		}
@@ -719,4 +725,28 @@ public class TerminalManager extends Service implements BridgeDisconnectedListen
 			mPendingReconnect.clear();
 		}
 	}
+
+	/**
+	 * Register a {@code listener} that wants to know when a host's status materially changes.
+	 * @see #hostStatusChangedListeners
+	 */
+	public void registerOnHostStatusChangedListener(OnHostStatusChangedListener listener) {
+		if (!hostStatusChangedListeners.contains(listener)) {
+			hostStatusChangedListeners.add(listener);
+		}
+	}
+
+	/**
+	 * Unregister a {@code listener} that wants to know when a host's status materially changes.
+	 * @see #hostStatusChangedListeners
+	 */
+	public void unregisterOnHostStatusChangedListener(OnHostStatusChangedListener listener) {
+		hostStatusChangedListeners.remove(listener);
+	}
+
+	private void notifyHostStatusChanged() {
+		for (OnHostStatusChangedListener listener : hostStatusChangedListeners) {
+			listener.onHostStatusChanged();
+		}
+	}
 }