Address review feedback for feature branch

briar-android/src/main/java/org/briarproject/briar/android/hotspot/ConditionManager29.java

@@ -61,15 +61,14 @@ class ConditionManager29 extends AbstractConditionManager {
 	}
 
 	private boolean areEssentialPermissionsGranted() {
+		boolean isWifiEnabled = wifiManager.isWifiEnabled();
 		if (LOG.isLoggable(INFO)) {
 			LOG.info(String.format("areEssentialPermissionsGranted(): " +
 							"locationPermission? %s, " +
 							"wifiManager.isWifiEnabled()? %b",
-					locationPermission,
-					wifiManager.isWifiEnabled()));
+					locationPermission, isWifiEnabled));
 		}
-		return locationPermission == Permission.GRANTED &&
-				wifiManager.isWifiEnabled();
+		return locationPermission == Permission.GRANTED && isWifiEnabled;
 	}
 
 	@Override

briar-android/src/main/java/org/briarproject/briar/android/hotspot/FallbackFragment.java

@@ -15,6 +15,7 @@ import org.briarproject.bramble.api.nullsafety.MethodsNotNullByDefault;
 import org.briarproject.bramble.api.nullsafety.ParametersNotNullByDefault;
 import org.briarproject.briar.R;
 import org.briarproject.briar.android.fragment.BaseFragment;
+import org.briarproject.briar.android.util.ActivityLaunchers.CreateDocumentAdvanced;
 
 import java.util.List;
 
@@ -22,7 +23,6 @@ import javax.inject.Inject;
 
 import androidx.activity.result.ActivityResultLauncher;
 import androidx.annotation.Nullable;
-import androidx.fragment.app.Fragment;
 import androidx.fragment.app.FragmentActivity;
 import androidx.lifecycle.ViewModelProvider;
 
@@ -33,12 +33,10 @@ import static android.content.pm.PackageManager.MATCH_DEFAULT_ONLY;
 import static android.os.Build.VERSION.SDK_INT;
 import static android.view.View.INVISIBLE;
 import static android.view.View.VISIBLE;
-import static androidx.activity.result.contract.ActivityResultContracts.CreateDocument;
 import static androidx.transition.TransitionManager.beginDelayedTransition;
 import static org.briarproject.briar.android.AppModule.getAndroidComponent;
 import static org.briarproject.briar.android.hotspot.HotspotViewModel.getApkFileName;
 
-
 @MethodsNotNullByDefault
 @ParametersNotNullByDefault
 public class FallbackFragment extends BaseFragment {
@@ -50,7 +48,7 @@ public class FallbackFragment extends BaseFragment {
 
 	private HotspotViewModel viewModel;
 	private final ActivityResultLauncher<String> launcher =
-			registerForActivityResult(new CreateDocument(),
+			registerForActivityResult(new CreateDocumentAdvanced(),
 					this::onDocumentCreated);
 	private Button fallbackButton;
 	private ProgressBar progressBar;
@@ -75,7 +73,7 @@ public class FallbackFragment extends BaseFragment {
 			@Nullable ViewGroup container,
 			@Nullable Bundle savedInstanceState) {
 		return inflater
-				.inflate(R.layout.fragment_hotspot_save_apk, container, false);
+				.inflate(R.layout.fragment_hotspot_fallback, container, false);
 	}
 
 	@Override
@@ -92,8 +90,7 @@ public class FallbackFragment extends BaseFragment {
 			if (SDK_INT >= 19) launcher.launch(getApkFileName());
 			else viewModel.exportApk();
 		});
-		viewModel.getSavedApkToUri()
-				.observeEvent(this, uri -> shareUri(this, uri));
+		viewModel.getSavedApkToUri().observeEvent(this, this::shareUri);
 	}
 
 	private void onDocumentCreated(@Nullable Uri uri) {
@@ -107,12 +104,12 @@ public class FallbackFragment extends BaseFragment {
 		progressBar.setVisibility(INVISIBLE);
 	}
 
-	static void shareUri(Fragment fragment, Uri uri) {
+	void shareUri(Uri uri) {
 		Intent i = new Intent(ACTION_SEND);
 		i.putExtra(EXTRA_STREAM, uri);
 		i.setType("*/*"); // gives us all sharing options
 		i.addFlags(FLAG_GRANT_READ_URI_PERMISSION);
-		Context ctx = fragment.requireContext();
+		Context ctx = requireContext();
 		if (SDK_INT <= 19) {
 			// Workaround for Android bug:
 			// ctx.grantUriPermission also needed for Android 4
@@ -124,7 +121,7 @@ public class FallbackFragment extends BaseFragment {
 						FLAG_GRANT_READ_URI_PERMISSION);
 			}
 		}
-		fragment.startActivity(Intent.createChooser(i, null));
+		startActivity(Intent.createChooser(i, null));
 	}
 
 }

briar-android/src/main/java/org/briarproject/briar/android/hotspot/HotspotActivity.java

@@ -59,12 +59,12 @@ public class HotspotActivity extends BriarActivity
 				// check if fragment is already added
 				// to not lose state on configuration changes
 				if (fm.findFragmentByTag(tag) == null) {
-					if (!started.consume()) {
+					if (started.wasNotYetConsumed()) {
 						showFragment(fm, new HotspotFragment(), tag);
 					}
 				}
 			} else if (hotspotState instanceof HotspotError) {
-				HotspotError error = ((HotspotError) hotspotState);
+				HotspotError error = (HotspotError) hotspotState;
 				showErrorFragment(error.getError());
 			}
 		});

briar-android/src/main/java/org/briarproject/briar/android/hotspot/HotspotFragment.java

@@ -2,13 +2,11 @@ package org.briarproject.briar.android.hotspot;
 
 import android.os.Bundle;
 import android.view.View;
-
-import com.google.android.material.snackbar.Snackbar;
+import android.widget.Toast;
 
 import org.briarproject.bramble.api.nullsafety.MethodsNotNullByDefault;
 import org.briarproject.bramble.api.nullsafety.ParametersNotNullByDefault;
 import org.briarproject.briar.R;
-import org.briarproject.briar.android.util.BriarSnackbarBuilder;
 
 import androidx.annotation.Nullable;
 import androidx.fragment.app.Fragment;
@@ -41,13 +39,8 @@ public class HotspotFragment extends AbstractTabsFragment {
 
 	private void onPeerConnected(boolean connected) {
 		if (!connected) return;
-		new BriarSnackbarBuilder()
-				.setAction(R.string.hotspot_peer_connected_action, v ->
-						showNextFragment())
-				.make(connectedButton, R.string.hotspot_peer_connected,
-						Snackbar.LENGTH_LONG)
-				.setAnchorView(connectedButton)
-				.show();
+		Toast.makeText(requireContext(), R.string.hotspot_peer_connected,
+				Toast.LENGTH_LONG).show();
 	}
 
 	private void showNextFragment() {

briar-android/src/main/java/org/briarproject/briar/android/hotspot/HotspotManager.java

@@ -48,7 +48,9 @@ import static android.net.wifi.p2p.WifiP2pManager.NO_SERVICE_REQUESTS;
 import static android.net.wifi.p2p.WifiP2pManager.P2P_UNSUPPORTED;
 import static android.os.Build.VERSION.SDK_INT;
 import static android.os.PowerManager.FULL_WAKE_LOCK;
+import static java.util.Objects.requireNonNull;
 import static java.util.logging.Level.INFO;
+import static java.util.logging.Level.WARNING;
 import static java.util.logging.Logger.getLogger;
 import static org.briarproject.briar.android.util.UiUtils.handleException;
 
@@ -57,6 +59,7 @@ import static org.briarproject.briar.android.util.UiUtils.handleException;
 class HotspotManager {
 
 	interface HotspotListener {
+		@UiThread
 		void onStartingHotspot();
 
 		@IoExecutor
@@ -65,8 +68,7 @@ class HotspotManager {
 		@UiThread
 		void onDeviceConnected();
 
-		void onHotspotStopped();
-
+		@UiThread
 		void onHotspotError(String error);
 	}
 
@@ -97,8 +99,9 @@ class HotspotManager {
 	private WifiManager.WifiLock wifiLock;
 	private PowerManager.WakeLock wakeLock;
 	private WifiP2pManager.Channel channel;
+	@Nullable
 	@RequiresApi(29)
-	private volatile NetworkConfig savedNetworkConfig;
+	private volatile NetworkConfig savedNetworkConfig = null;
 
 	@Inject
 	HotspotManager(Application ctx,
@@ -157,9 +160,11 @@ class HotspotManager {
 	 * We'll realize that the framework is busy when the ActionListener passed
 	 * to {@link WifiP2pManager#createGroup} is called with onFailure(BUSY)
 	 */
+	@UiThread
 	private void startWifiP2pFramework(int attempt) {
-		if (LOG.isLoggable(INFO))
+		if (LOG.isLoggable(INFO)) {
 			LOG.info("startWifiP2pFramework attempt: " + attempt);
+		}
 		/*
 		 * It is important that we call WifiP2pManager#initialize again
 		 * for every attempt to starting the framework because otherwise,
@@ -167,13 +172,12 @@ class HotspotManager {
 		 */
 		channel = wifiP2pManager.initialize(ctx, ctx.getMainLooper(), null);
 		if (channel == null) {
-			listener.onHotspotError(
+			releaseHotspotWithError(
 					ctx.getString(R.string.hotspot_error_no_wifi_direct));
 			return;
 		}
 
 		ActionListener listener = new ActionListener() {
-
 			@Override
 			// Callback for wifiP2pManager#createGroup() during startWifiP2pHotspot()
 			public void onSuccess() {
@@ -183,7 +187,9 @@ class HotspotManager {
 			@Override
 			// Callback for wifiP2pManager#createGroup() during startWifiP2pHotspot()
 			public void onFailure(int reason) {
-				LOG.info("onFailure: " + reason);
+				if (LOG.isLoggable(INFO)) {
+					LOG.info("onFailure: " + reason);
+				}
 				if (reason == BUSY) {
 					// WifiP2p not ready yet or hotspot already running
 					restartWifiP2pFramework(attempt);
@@ -210,21 +216,26 @@ class HotspotManager {
 
 		try {
 			if (SDK_INT >= 29) {
-				dbExecutor.execute(() -> {
+				Runnable createGroup = () -> {
+					NetworkConfig c = requireNonNull(savedNetworkConfig);
+					WifiP2pConfig config = new WifiP2pConfig.Builder()
+							.setGroupOperatingBand(GROUP_OWNER_BAND_2GHZ)
+							.setNetworkName(c.ssid)
+							.setPassphrase(c.password)
+							.build();
+					wifiP2pManager.createGroup(channel, config, listener);
+				};
+				if (savedNetworkConfig == null) {
 					// load savedNetworkConfig before starting hotspot
-					loadSavedNetworkConfig();
-					androidExecutor.runOnUiThread(() -> {
-						WifiP2pConfig config = new WifiP2pConfig.Builder()
-								.setGroupOperatingBand(GROUP_OWNER_BAND_2GHZ)
-								.setNetworkName(savedNetworkConfig.ssid)
-								.setPassphrase(savedNetworkConfig.password)
-								.build();
-						acquireLocks();
-						wifiP2pManager.createGroup(channel, config, listener);
+					dbExecutor.execute(() -> {
+						loadSavedNetworkConfig();
+						androidExecutor.runOnUiThread(createGroup);
 					});
-				});
+				} else {
+					// savedNetworkConfig was already loaded, create group now
+					createGroup.run();
+				}
 			} else {
-				acquireLocks();
 				wifiP2pManager.createGroup(channel, listener);
 			}
 		} catch (SecurityException e) {
@@ -233,9 +244,11 @@ class HotspotManager {
 		}
 	}
 
+	@UiThread
 	private void restartWifiP2pFramework(int attempt) {
 		LOG.info("retrying to start WifiP2p framework");
 		if (attempt < MAX_FRAMEWORK_ATTEMPTS) {
+			if (SDK_INT >= 27 && channel != null) channel.close();
 			handler.postDelayed(() -> startWifiP2pFramework(attempt + 1),
 					RETRY_DELAY_MILLIS);
 		} else {
@@ -250,19 +263,23 @@ class HotspotManager {
 		wifiP2pManager.removeGroup(channel, new ActionListener() {
 			@Override
 			public void onSuccess() {
-				releaseHotspot();
+				closeChannelAndReleaseLocks();
 			}
 
 			@Override
 			public void onFailure(int reason) {
 				// not propagating back error
-				releaseHotspot();
+				if (LOG.isLoggable(WARNING)) {
+					LOG.warning("Error removing Wifi P2P group: " + reason);
+				}
+				closeChannelAndReleaseLocks();
 			}
 		});
 	}
 
 	@SuppressLint("WakelockTimeout")
 	private void acquireLocks() {
+		// FLAG_KEEP_SCREEN_ON is not respected on some Huawei devices.
 		wakeLock = powerManager.newWakeLock(FULL_WAKE_LOCK, lockTag);
 		wakeLock.acquire();
 		// WIFI_MODE_FULL has no effect on API >= 29
@@ -272,23 +289,21 @@ class HotspotManager {
 		wifiLock.acquire();
 	}
 
-	private void releaseHotspot() {
-		listener.onHotspotStopped();
-		closeChannelAndReleaseLocks();
-	}
-
+	@UiThread
 	private void releaseHotspotWithError(String error) {
 		listener.onHotspotError(error);
 		closeChannelAndReleaseLocks();
 	}
 
+	@UiThread
 	private void closeChannelAndReleaseLocks() {
-		if (SDK_INT >= 27) channel.close();
+		if (SDK_INT >= 27 && channel != null) channel.close();
 		channel = null;
-		wakeLock.release();
-		wifiLock.release();
+		if (wakeLock.isHeld()) wakeLock.release();
+		if (wifiLock.isHeld()) wifiLock.release();
 	}
 
+	@UiThread
 	private void requestGroupInfo(int attempt) {
 		if (LOG.isLoggable(INFO)) {
 			LOG.info("requestGroupInfo attempt: " + attempt);
@@ -331,25 +346,20 @@ class HotspotManager {
 			LOG.info("group is null");
 			return false;
 		} else if (!group.getNetworkName().startsWith("DIRECT-")) {
-			if (LOG.isLoggable(INFO)) {
-				LOG.info("received networkName without prefix 'DIRECT-': " +
-						group.getNetworkName());
-			}
+			LOG.info("received networkName without prefix 'DIRECT-'");
 			return false;
 		} else if (SDK_INT >= 29) {
 			// if we get here, the savedNetworkConfig must have a value
-			String networkName = savedNetworkConfig.ssid;
+			String networkName = requireNonNull(savedNetworkConfig).ssid;
 			if (!networkName.equals(group.getNetworkName())) {
-				if (LOG.isLoggable(INFO)) {
-					LOG.info("expected networkName: " + networkName);
-					LOG.info("received networkName: " + group.getNetworkName());
-				}
+				LOG.info("expected networkName does not match received one");
 				return false;
 			}
 		}
 		return true;
 	}
 
+	@UiThread
 	private void retryRequestingGroupInfo(int attempt) {
 		LOG.info("retrying to request group info");
 		// On some devices we need to wait for the group info to become available
@@ -364,17 +374,12 @@ class HotspotManager {
 
 	@UiThread
 	private void requestGroupInfoForConnection() {
-		if (LOG.isLoggable(INFO)) {
-			LOG.info("requestGroupInfo for connection");
-		}
+		LOG.info("requestGroupInfo for connection");
 		GroupInfoListener groupListener = group -> {
 			if (group == null || group.getClientList().isEmpty()) {
 				handler.postDelayed(this::requestGroupInfoForConnection,
 						RETRY_DELAY_MILLIS);
 			} else {
-				if (LOG.isLoggable(INFO)) {
-					LOG.info("client list " + group.getClientList());
-				}
 				listener.onDeviceConnected();
 			}
 		};
@@ -433,26 +438,15 @@ class HotspotManager {
 	}
 
 	// exclude chars that are easy to confuse: 0 O, 5 S, 1 l I
-	private static final String digits = "2346789";
-	private static final String letters = "abcdefghijkmnopqrstuvwxyz";
-	private static final String LETTERS = "ABCDEFGHJKLMNPQRTUVWXYZ";
+	private static final String chars =
+			"2346789ABCDEFGHJKLMNPQRTUVWXYZabcdefghijkmnopqrstuvwxyz";
 
 	private String getRandomString(int length) {
 		char[] c = new char[length];
 		for (int i = 0; i < length; i++) {
-			if (random.nextBoolean()) {
-				c[i] = random(digits);
-			} else if (random.nextBoolean()) {
-				c[i] = random(letters);
-			} else {
-				c[i] = random(LETTERS);
-			}
+			c[i] = chars.charAt(random.nextInt(chars.length()));
 		}
 		return new String(c);
 	}
 
-	private char random(String universe) {
-		return universe.charAt(random.nextInt(universe.length()));
-	}
-
 }

briar-android/src/main/java/org/briarproject/briar/android/hotspot/HotspotState.java

@@ -63,10 +63,10 @@ abstract class HotspotState {
 		 * to not repeat actions such as showing fragments on rotation changes.
 		 */
 		@UiThread
-		boolean consume() {
+		boolean wasNotYetConsumed() {
 			boolean old = consumed;
 			consumed = true;
-			return old;
+			return !old;
 		}
 	}
 

briar-android/src/main/java/org/briarproject/briar/android/hotspot/HotspotViewModel.java

@@ -141,12 +141,6 @@ class HotspotViewModel extends DbViewModel
 		peerConnected.setEvent(true);
 	}
 
-	@Override
-	public void onHotspotStopped() {
-		LOG.info("stopping webserver");
-		ioExecutor.execute(webServerManager::stopWebServer);
-	}
-
 	@Override
 	public void onHotspotError(String error) {
 		if (LOG.isLoggable(WARNING)) {
@@ -170,7 +164,7 @@ class HotspotViewModel extends DbViewModel
 	public void onWebServerError() {
 		state.postValue(new HotspotError(getApplication()
 				.getString(R.string.hotspot_error_web_server_start)));
-		hotspotManager.stopWifiP2pHotspot();
+		stopHotspot();
 	}
 
 	void exportApk(Uri uri) {

briar-android/src/main/java/org/briarproject/briar/android/hotspot/QrHotspotFragment.java

@@ -1,12 +1,14 @@
 package org.briarproject.briar.android.hotspot;
 
 import android.content.Context;
+import android.graphics.Bitmap;
 import android.os.Bundle;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
 import android.widget.ImageView;
 import android.widget.TextView;
+import android.widget.Toast;
 
 import org.briarproject.bramble.api.nullsafety.MethodsNotNullByDefault;
 import org.briarproject.bramble.api.nullsafety.ParametersNotNullByDefault;
@@ -16,7 +18,6 @@ import org.briarproject.briar.android.hotspot.HotspotState.HotspotStarted;
 import javax.inject.Inject;
 
 import androidx.annotation.Nullable;
-import androidx.core.util.Consumer;
 import androidx.fragment.app.Fragment;
 import androidx.lifecycle.ViewModelProvider;
 
@@ -60,19 +61,23 @@ public class QrHotspotFragment extends Fragment {
 		TextView qrIntroView = v.findViewById(R.id.qrIntroView);
 		ImageView qrCodeView = v.findViewById(R.id.qrCodeView);
 
-		Consumer<HotspotStarted> consumer;
-		if (requireArguments().getBoolean(ARG_FOR_WIFI_CONNECT)) {
-			qrIntroView.setText(R.string.hotspot_qr_wifi);
-			consumer = state ->
-					qrCodeView.setImageBitmap(state.getNetworkConfig().qrCode);
-		} else {
-			qrIntroView.setText(R.string.hotspot_qr_site);
-			consumer = state ->
-					qrCodeView.setImageBitmap(state.getWebsiteConfig().qrCode);
-		}
+		boolean forWifi = requireArguments().getBoolean(ARG_FOR_WIFI_CONNECT);
+
+		qrIntroView.setText(forWifi ? R.string.hotspot_qr_wifi :
+				R.string.hotspot_qr_site);
+
 		viewModel.getState().observe(getViewLifecycleOwner(), state -> {
 			if (state instanceof HotspotStarted) {
-				consumer.accept((HotspotStarted) state);
+				HotspotStarted s = (HotspotStarted) state;
+				Bitmap qrCode = forWifi ? s.getNetworkConfig().qrCode :
+						s.getWebsiteConfig().qrCode;
+				if (qrCode == null) {
+					Toast.makeText(requireContext(), R.string.error,
+							Toast.LENGTH_SHORT).show();
+					qrCodeView.setImageResource(R.drawable.ic_image_broken);
+				} else {
+					qrCodeView.setImageBitmap(qrCode);
+				}
 			}
 		});
 		return v;

briar-android/src/main/java/org/briarproject/briar/android/hotspot/WebServerManager.java

@@ -22,6 +22,7 @@ import java.util.logging.Logger;
 import javax.inject.Inject;
 
 import androidx.annotation.Nullable;
+import androidx.annotation.UiThread;
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.list;
@@ -49,7 +50,7 @@ class WebServerManager {
 	private final WebServer webServer;
 	private final DisplayMetrics dm;
 
-	private WebServerListener listener;
+	private volatile WebServerListener listener;
 
 	@Inject
 	WebServerManager(Application ctx) {
@@ -57,6 +58,7 @@ class WebServerManager {
 		dm = ctx.getResources().getDisplayMetrics();
 	}
 
+	@UiThread
 	void setListener(WebServerListener listener) {
 		this.listener = listener;
 	}