Commit a258da12 authored by Sebastian Kürten's avatar Sebastian Kürten
Browse files

Apply review feedback

* Improve HotspotManager logging (attempt count)
* Add layout margin for status text
* Fix wording in inner documentation
* Remove unneeded LOG field
* Remove flag 'startRequested'
* Move Javadocs within HotspotManager
parent abc356e9
......@@ -2,12 +2,9 @@ package org.briarproject.hotspot;
import android.net.wifi.WifiManager;
import java.util.logging.Logger;
import androidx.fragment.app.FragmentActivity;
import static android.content.Context.WIFI_SERVICE;
import static java.util.logging.Logger.getLogger;
/**
* Abstract base class for the ConditionManagers that ensure that the conditions
......@@ -20,9 +17,6 @@ abstract class ConditionManager {
UNKNOWN, GRANTED, SHOW_RATIONALE, PERMANENTLY_DENIED
}
private static final Logger LOG =
getLogger(ConditionManager.class.getName());
protected final Runnable permissionUpdateCallback;
protected FragmentActivity ctx;
protected WifiManager wifiManager;
......
......@@ -99,7 +99,12 @@ class ConditionManager29Impl extends ConditionManager {
showRationale(ctx, R.string.wifi_settings_title,
R.string.wifi_settings_request_enable_body,
this::requestEnableWiFi);
return false;
}
// we shouldn't usually reach this point, but if we do, return false
// anyway to force a recheck. Maybe some condition changed in the
// meantime.
return false;
}
......
......@@ -8,7 +8,6 @@ import java.util.logging.Logger;
import androidx.activity.result.ActivityResultCaller;
import androidx.activity.result.ActivityResultLauncher;
import androidx.activity.result.contract.ActivityResultContracts.StartActivityForResult;
import androidx.fragment.app.FragmentActivity;
import static java.util.logging.Level.INFO;
import static java.util.logging.Logger.getLogger;
......@@ -36,11 +35,6 @@ class ConditionManagerImpl extends ConditionManager {
result -> permissionUpdateCallback.run());
}
@Override
void init(FragmentActivity ctx) {
super.init(ctx);
}
@Override
void onStart() {
// nothing to do here
......
......@@ -33,20 +33,6 @@ public class HotspotFragment extends Fragment {
private ImageView qrCode;
private TextView ssidView, passwordView, statusView;
private Button button, serverButton;
/*
* We keep track of whether a start has been requested by tapping the button.
* The PermissionUpdateCallback we pass to the ConditionManager can receive
* an update even if we actually did not try to start the hotspot yet. If
* we did not check if the user actually requested to start the hotspot, we
* would start the hotspot as soon as all conditions are fulfilled. We don't
* want that behavior, instead we want the user to actively initiate that
* process.
*
* We set this variable to true when the button gets tapped and reset to
* false as soon as we receive any status update from the view model about
* the hotspot status.
*/
private boolean startRequested = false;
private boolean hotspotStarted = false;
private final ConditionManager conditionManager = SDK_INT < 29 ?
......@@ -81,7 +67,6 @@ public class HotspotFragment extends Fragment {
.setText(getString(R.string.wifi_5ghz_supported)));
viewModel.getStatus().observe(getViewLifecycleOwner(), state -> {
startRequested = false;
if (state instanceof StartingHotspot) {
statusView.setText(getString(R.string.starting_hotspot));
} else if (state instanceof HotspotStarted) {
......@@ -159,14 +144,14 @@ public class HotspotFragment extends Fragment {
} else {
// the hotspot is currently stopped → start it
button.setEnabled(false);
startRequested = true;
startWifiP2pHotspot();
}
}
private void startWifiP2pHotspot() {
if (startRequested && conditionManager.checkAndRequestConditions())
if (conditionManager.checkAndRequestConditions()) {
viewModel.startWifiP2pHotspot();
}
}
public void onServerButtonClick(View view) {
......
......@@ -60,25 +60,6 @@ class HotspotManager {
private final WifiP2pManager wifiP2pManager;
private final Handler handler;
private final String lockTag;
/**
* As soon as Wifi is enabled, we try starting the WifiP2p framework.
* If Wifi has just been enabled, it is possible that fails. If that happens
* we try again for MAX_FRAMEWORK_ATTEMPTS times after a delay of
* RETRY_DELAY_MILLIS after each attempt.
* <p>
* Rationale: it can take a few milliseconds for WifiP2p to become available
* after enabling Wifi. Depending on the API level it is possible to check this
* using {@link WifiP2pManager#requestP2pState} or register a BroadcastReceiver
* on the WIFI_P2P_STATE_CHANGED_ACTION to get notified when WifiP2p is really
* available. Trying to implement a solution that works reliably using these
* checks turned out to be a long rabbit-hole with lots of corner cases and
* workarounds for specific situations.
* Instead we now rely on this trial-and-error approach of just starting
* the framework and retrying if it fails.
* <p>
* We'll realize that the framework is busy when the ActionListener passed
* to {@link WifiP2pManager#createGroup} is called with onFailure(BUSY)
*/
@Nullable
// on API < 29 this is null because we cannot request a custom network name
......@@ -109,9 +90,30 @@ class HotspotManager {
startWifiP2pFramework(1);
}
/**
* As soon as Wifi is enabled, we try starting the WifiP2p framework.
* If Wifi has just been enabled, it is possible that will fail. If that
* happens we try again for MAX_FRAMEWORK_ATTEMPTS times after a delay of
* RETRY_DELAY_MILLIS after each attempt.
* <p>
* Rationale: it can take a few milliseconds for WifiP2p to become available
* after enabling Wifi. Depending on the API level it is possible to check this
* using {@link WifiP2pManager#requestP2pState} or register a BroadcastReceiver
* on the WIFI_P2P_STATE_CHANGED_ACTION to get notified when WifiP2p is really
* available. Trying to implement a solution that works reliably using these
* checks turned out to be a long rabbit-hole with lots of corner cases and
* workarounds for specific situations.
* Instead we now rely on this trial-and-error approach of just starting
* the framework and retrying if it fails.
* <p>
* We'll realize that the framework is busy when the ActionListener passed
* to {@link WifiP2pManager#createGroup} is called with onFailure(BUSY)
*/
void startWifiP2pFramework(int attempt) {
/**
* It is important that we call {@link WifiP2pManager#initialize} again
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,
* createGroup() will continue to fail with a BUSY state.
*/
......@@ -177,6 +179,7 @@ class HotspotManager {
}
private void restartWifiP2pFramework(int attempt) {
LOG.info("retrying to start WifiP2p framework");
if (attempt < MAX_FRAMEWORK_ATTEMPTS) {
handler.postDelayed(() -> startWifiP2pFramework(attempt + 1),
RETRY_DELAY_MILLIS);
......@@ -327,7 +330,7 @@ class HotspotManager {
}
private void retryRequestingGroupInfo(int attempt) {
LOG.info("retrying");
LOG.info("retrying to request group info");
// On some devices we need to wait for the group info to become available
if (attempt < MAX_GROUP_INFO_ATTEMPTS) {
handler.postDelayed(() -> requestGroupInfo(attempt + 1),
......
......@@ -45,7 +45,9 @@
android:id="@+id/status"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:textSize="16sp" />
android:layout_margin="16dp"
android:textSize="16sp"
tools:text="@string/stop_framework_busy" />
<Button
android:id="@+id/serverButton"
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment