Merge branch 'cleanup-swap-stuff' into 'master'

Cleanup swap stuff, make more robust

While we're discussing the merits or otherwise of !208, here is some fixes to the swap workflow cherry-picked (and cleaned up) from that branch.

Although not directly reproducible, I'm confident this should fix #556 and #557.

The original assumption was that we can start the wifi local repo server, then tell the user it is connected while JmDNS is starting. The plan was for it to be very snappy, so the user could continue using the UI while bonjour was doing its stuff. However, in realtity this results in the possibility of turning swap on and off again while bonjour is still getting ready.

This now makes the user wait both when starting swap, and also when stopping swap. It will provide proper feedback to the user, do it on a background thread (properly) and update the UI when done.

Added some other misc cleanups while there.

See merge request !213

F-Droid/res/values/strings.xml

@@ -322,6 +322,7 @@
     <string name="swap_not_visible_bluetooth">Not visible via Bluetooth</string>
     <string name="swap_visible_wifi">Visible via Wi-Fi</string>
     <string name="swap_setting_up_wifi">Setting up Wi-Fi…</string>
+    <string name="swap_stopping_wifi">Stopping Wi-Fi…</string>
     <string name="swap_not_visible_wifi">Not visible via Wi-Fi</string>
     <string name="swap_wifi_device_name">Device Name</string>
     <string name="swap_cant_find_peers">Can\'t find who you\'re looking for?</string>

F-Droid/src/org/fdroid/fdroid/localrepo/SwapService.java

@@ -441,14 +441,18 @@ public class SwapService extends Service {
     /**
      * Handles checking if the {@link SwapService} is running, and only restarts it if it was running.
      */
-    public void restartWifiIfEnabled() {
+    public void stopWifiIfEnabled(final boolean restartAfterStopping) {
         if (wifiSwap.isConnected()) {
             new AsyncTask<Void, Void, Void>() {
                 @Override
                 protected Void doInBackground(Void... params) {
-                    Utils.debugLog(TAG, "Restarting WiFi swap service");
+                    Utils.debugLog(TAG, "Stopping the currently running WiFi swap service (on background thread)");
                     wifiSwap.stop();
-                    wifiSwap.start();
+
+                    if (restartAfterStopping) {
+                        Utils.debugLog(TAG, "Restarting WiFi swap service after stopping (still on background thread)");
+                        wifiSwap.start();
+                    }
                     return null;
                 }
             }.execute();
@@ -483,6 +487,7 @@ public class SwapService extends Service {
     public static final String WIFI_STATE_CHANGE = "org.fdroid.fdroid.WIFI_STATE_CHANGE";
     public static final String EXTRA_STARTING = "STARTING";
     public static final String EXTRA_STARTED = "STARTED";
+    public static final String EXTRA_STOPPING = "STOPPING";
     public static final String EXTRA_STOPPED = "STOPPED";
 
     private static final int NOTIFICATION = 1;
@@ -632,7 +637,7 @@ public class SwapService extends Service {
         @Override
         public void onPreferenceChange() {
             Log.i(TAG, "Swap over HTTPS preference changed.");
-            restartWifiIfEnabled();
+            stopWifiIfEnabled(true);
         }
     };
 
@@ -640,7 +645,8 @@ public class SwapService extends Service {
     private final BroadcastReceiver onWifiChange = new BroadcastReceiver() {
         @Override
         public void onReceive(Context context, Intent i) {
-            restartWifiIfEnabled();
+            boolean hasIp = FDroidApp.ipAddressString != null;
+            stopWifiIfEnabled(hasIp);
         }
     };
 

F-Droid/src/org/fdroid/fdroid/localrepo/type/BonjourBroadcast.java

@@ -1,6 +1,7 @@
 package org.fdroid.fdroid.localrepo.type;
 
 import android.content.Context;
+import android.support.annotation.Nullable;
 import android.util.Log;
 
 import org.fdroid.fdroid.FDroidApp;
@@ -10,6 +11,7 @@ import org.fdroid.fdroid.localrepo.SwapService;
 
 import java.io.IOException;
 import java.net.InetAddress;
+import java.net.UnknownHostException;
 import java.util.HashMap;
 
 import javax.jmdns.JmDNS;
@@ -31,10 +33,15 @@ public class BonjourBroadcast extends SwapType {
 
     @Override
     public void start() {
-
         Utils.debugLog(TAG, "Preparing to start Bonjour service.");
         sendBroadcast(SwapService.EXTRA_STARTING);
 
+        InetAddress address = getDeviceAddress();
+        if (address == null) {
+            Log.e(TAG, "Starting Bonjour service, but couldn't ascertain IP address. Seems we are not connected to a network.");
+            return;
+        }
+
         /*
          * a ServiceInfo can only be registered with a single instance
          * of JmDNS, and there is only ever a single LocalHTTPD port to
@@ -43,6 +50,7 @@ public class BonjourBroadcast extends SwapType {
         if (pairService != null || jmdns != null) {
             clearCurrentMDNSService();
         }
+
         String repoName = Preferences.get().getLocalRepoName();
         HashMap<String, String> values = new HashMap<>();
         values.put("path", "/fdroid/repo");
@@ -59,7 +67,7 @@ public class BonjourBroadcast extends SwapType {
         try {
             Utils.debugLog(TAG, "Starting bonjour service...");
             pairService = ServiceInfo.create(type, repoName, FDroidApp.port, 0, 0, values);
-            jmdns = JmDNS.create(InetAddress.getByName(FDroidApp.ipAddressString));
+            jmdns = JmDNS.create(address);
             jmdns.registerService(pairService);
             setConnected(true);
             Utils.debugLog(TAG, "... Bounjour service started.");
@@ -78,12 +86,9 @@ public class BonjourBroadcast extends SwapType {
 
     private void clearCurrentMDNSService() {
         if (jmdns != null) {
-            if (pairService != null) {
-                jmdns.unregisterService(pairService);
-                pairService = null;
-            }
             jmdns.unregisterAllServices();
             Utils.closeQuietly(jmdns);
+            pairService = null;
             jmdns = null;
         }
     }
@@ -93,4 +98,15 @@ public class BonjourBroadcast extends SwapType {
         return SwapService.BONJOUR_STATE_CHANGE;
     }
 
+    @Nullable
+    private InetAddress getDeviceAddress() {
+        if (FDroidApp.ipAddressString != null) {
+            try {
+                return InetAddress.getByName(FDroidApp.ipAddressString);
+            } catch (UnknownHostException e) { }
+        }
+
+        return null;
+    }
+
 }

F-Droid/src/org/fdroid/fdroid/localrepo/type/SwapType.java

@@ -99,7 +99,7 @@ public abstract class SwapType {
             public void run() {
                 SwapType.this.stop();
             }
-        }.run();
+        }.start();
     }
 
 }

F-Droid/src/org/fdroid/fdroid/localrepo/type/WifiSwap.java

@@ -19,6 +19,13 @@ import java.io.IOException;
 import java.net.BindException;
 import java.util.Random;
 
+import rx.Single;
+import rx.SingleSubscriber;
+import rx.android.schedulers.AndroidSchedulers;
+import rx.functions.Action1;
+import rx.functions.Func2;
+import rx.schedulers.Schedulers;
+
 public class WifiSwap extends SwapType {
 
     private static final String TAG = "WifiSwap";
@@ -42,55 +49,115 @@ public class WifiSwap extends SwapType {
 
     @Override
     public void start() {
-
         Utils.debugLog(TAG, "Preparing swap webserver.");
         sendBroadcast(SwapService.EXTRA_STARTING);
 
-        Runnable webServer = new Runnable() {
+        if (FDroidApp.ipAddressString == null) {
-            // Tell Eclipse this is not a leak because of Looper use.
+            Log.e(TAG, "Not starting swap webserver, because we don't seem to be connected to a network.");
-            @SuppressLint("HandlerLeak")
+            setConnected(false);
-            @Override
+        }
-            public void run() {
-                localHttpd = new LocalHTTPD(
-                        context,
-                        FDroidApp.ipAddressString,
-                        FDroidApp.port,
-                        context.getFilesDir(),
-                        Preferences.get().isLocalRepoHttpsEnabled());
 
-                Looper.prepare(); // must be run before creating a Handler
+        Single.zip(
-                webServerThreadHandler = new Handler() {
+                Single.create(getWebServerTask()),
+                Single.create(getBonjourTask()),
+                new Func2<Boolean, Boolean, Boolean>() {
                     @Override
-                    public void handleMessage(Message msg) {
+                    public Boolean call(Boolean webServerTask, Boolean bonjourServiceTask) {
-                        Log.i(TAG, "we've been asked to stop the webserver: " + msg.obj);
+                        return bonjourServiceTask && webServerTask;
-                        setConnected(false);
-                        localHttpd.stop();
                     }
-                };
+                })
-                try {
+                .observeOn(AndroidSchedulers.mainThread())
-                    Utils.debugLog(TAG, "Starting swap webserver...");
+                .subscribeOn(Schedulers.newThread())
-                    localHttpd.start();
+                .subscribe(new Action1<Boolean>() {
-                    setConnected(true);
+                        @Override
-                    Utils.debugLog(TAG, "Swap webserver started.");
+                        public void call(Boolean success) {
-                } catch (BindException e) {
+                            setConnected(success);
-                    int prev = FDroidApp.port;
+                        }
-                    FDroidApp.port = FDroidApp.port + new Random().nextInt(1111);
+                    },
-                    setConnected(false);
+                    new Action1<Throwable>() {
-                    Log.w(TAG, "port " + prev + " occupied, trying on " + FDroidApp.port + "!");
+                        @Override
-                    context.startService(new Intent(context, WifiStateChangeService.class));
+                        public void call(Throwable throwable) {
-                } catch (IOException e) {
+                            setConnected(false);
-                    setConnected(false);
+                        }
-                    Log.e(TAG, "Could not start local repo HTTP server", e);
+                    });
-                }
+    }
-                Looper.loop(); // start the message receiving loop
+
+    /**
+     * A task which starts the {@link WifiSwap#bonjourBroadcast} and then emits a `true` value at
+     * the end.
+     */
+    private Single.OnSubscribe<Boolean> getBonjourTask() {
+        return new Single.OnSubscribe<Boolean>() {
+            @Override
+            public void call(SingleSubscriber<? super Boolean> singleSubscriber) {
+                bonjourBroadcast.start();
+
+                // TODO: Be more intelligent about failures here so that we can invoke
+                // singleSubscriber.onError() in the appropriate circumstances.
+                singleSubscriber.onSuccess(true);
+            }
+        };
+    }
+
+    /**
+     * Constructs a new {@link Thread} for the webserver to run on. If successful, it will also
+     * populate the webServerThreadHandler property and bind it to that particular thread. This
+     * allows messages to be sent to the webserver thread by posting messages to that handler.
+     */
+    private Single.OnSubscribe<Boolean> getWebServerTask() {
+        return new Single.OnSubscribe<Boolean>() {
+            @Override
+            public void call(final SingleSubscriber<? super Boolean> singleSubscriber) {
+                new Thread(new Runnable() {
+                    // Tell Eclipse this is not a leak because of Looper use.
+                    @SuppressLint("HandlerLeak")
+                    @Override
+                    public void run() {
+                        localHttpd = new LocalHTTPD(
+                            context,
+                            FDroidApp.ipAddressString,
+                            FDroidApp.port,
+                            context.getFilesDir(),
+                            Preferences.get().isLocalRepoHttpsEnabled());
+
+                        Looper.prepare(); // must be run before creating a Handler
+                        webServerThreadHandler = new Handler() {
+                            @Override
+                            public void handleMessage(Message msg) {
+                                Log.i(TAG, "we've been asked to stop the webserver: " + msg.obj);
+                                localHttpd.stop();
+                                Looper looper = Looper.myLooper();
+                                if (looper == null) {
+                                    Log.e(TAG, "Looper.myLooper() was null for sum reason while shutting down the swap webserver.");
+                                } else {
+                                    looper.quit();
+                                }
+                            }
+                        };
+                        try {
+                            Utils.debugLog(TAG, "Starting swap webserver...");
+                            localHttpd.start();
+                            Utils.debugLog(TAG, "Swap webserver started.");
+                            singleSubscriber.onSuccess(true);
+                        } catch (BindException e) {
+                            int prev = FDroidApp.port;
+                            FDroidApp.port = FDroidApp.port + new Random().nextInt(1111);
+                            context.startService(new Intent(context, WifiStateChangeService.class));
+                            singleSubscriber.onError(new Exception("port " + prev + " occupied, trying on " + FDroidApp.port + "!"));
+                        } catch (IOException e) {
+                            Log.e(TAG, "Could not start local repo HTTP server", e);
+                            singleSubscriber.onError(e);
+                        }
+                        Looper.loop(); // start the message receiving loop
+                    }
+                }).start();
             }
         };
-        new Thread(webServer).start();
-        bonjourBroadcast.start();
     }
 
     @Override
     public void stop() {
+        sendBroadcast(SwapService.EXTRA_STOPPING);
         if (webServerThreadHandler == null) {
             Log.i(TAG, "null handler in stopWebServer");
         } else {
@@ -99,7 +166,13 @@ public class WifiSwap extends SwapType {
             msg.obj = webServerThreadHandler.getLooper().getThread().getName() + " says stop";
             webServerThreadHandler.sendMessage(msg);
         }
+
+        // Stop the Bonjour stuff after asking the webserver to stop. This is not required in this
+        // order, but it helps. In practice, the Bonjour stuff takes a second or two to stop. This
+        // should give enough time for the message we posted above to reach the web server thread
+        // and for the webserver to thus be stopped.
         bonjourBroadcast.stop();
+        setConnected(false);
     }
 
 }

F-Droid/src/org/fdroid/fdroid/net/WifiStateChangeService.java

@@ -181,7 +181,7 @@ public class WifiStateChangeService extends Service {
             getApplicationContext().bindService(swapService, new ServiceConnection() {
                 @Override
                 public void onServiceConnected(ComponentName name, IBinder service) {
-                    ((SwapService.Binder) service).getService().restartWifiIfEnabled();
+                    ((SwapService.Binder) service).getService().stopWifiIfEnabled(true);
                     getApplicationContext().unbindService(this);
                 }
 

F-Droid/src/org/fdroid/fdroid/views/swap/StartSwapView.java

@@ -302,9 +302,8 @@ public class StartSwapView extends ScrollView implements SwapWorkflowActivity.In
                     Utils.debugLog(TAG, "Received onCheckChanged(true) for WiFi swap, asking in background thread to ensure WiFi swap is running.");
                     getManager().getWifiSwap().ensureRunningInBackground();
                 } else {
-                    Utils.debugLog(TAG, "Received onCheckChanged(false) for WiFi swap, disabling WiFi swap.");
+                    Utils.debugLog(TAG, "Received onCheckChanged(false) for WiFi swap, disabling WiFi swap in background thread.");
-                    getManager().getWifiSwap().stop();
+                    getManager().getWifiSwap().stopInBackground();
-                    Utils.debugLog(TAG, "Received onCheckChanged(false) for WiFi swap, WiFi swap disabled successfully.");
                 }
                 uiUpdateWifiNetwork();
             }
@@ -323,10 +322,15 @@ public class StartSwapView extends ScrollView implements SwapWorkflowActivity.In
             @Override
             public void onReceive(Context context, Intent intent) {
                 if (intent.hasExtra(SwapService.EXTRA_STARTING)) {
-                    Utils.debugLog(TAG, "WiFi service is starting (setting toggle to visible, but disabled).");
+                    Utils.debugLog(TAG, "WiFi service is starting (setting toggle to checked, but disabled).");
                     textWifiVisible.setText(R.string.swap_setting_up_wifi);
                     wifiSwitch.setEnabled(false);
                     wifiSwitch.setChecked(true);
+                } else if (intent.hasExtra(SwapService.EXTRA_STOPPING)) {
+                    Utils.debugLog(TAG, "WiFi service is stopping (setting toggle to unchecked and disabled).");
+                    textWifiVisible.setText(R.string.swap_stopping_wifi);
+                    wifiSwitch.setEnabled(false);
+                    wifiSwitch.setChecked(false);
                 } else {
                     wifiSwitch.setEnabled(true);
                     if (intent.hasExtra(SwapService.EXTRA_STARTED)) {