Split ConditionManager into version for pre 29 and 29+
Then on pre 29, don't ask for the location permission.
Also, wait for Wi-Fi p2p broadcast before creating group.
Merge request reports
Activity
added 1 commit
- 456e0a33 - Split ConditionManager into version for pre 29 and 29+
added 1 commit
- 4c9b9b8c - Split ConditionManager into version for pre 29 and 29+
As the process for managing the conditions is pretty different pre 29 and 29+, I thought it makes sense to detangle this and move the logic into separate classes to make it simpler to understand what's going on. I hope that helps not only me. We still have API level branching in
HotspotFragmentbut that is difficult to avoid because theActivityResultLaunchers need to stay inside the fragment and cannot move into the respectiveConditionManager.I think we can make the pre 29 version even simpler, because as far as I understand now, on those API levels, we don't need any permission to enable the Wifi, but can just enable it. Did you encounter devices < 29 where enabling via
wifiManager.setWifiEnabled(true);did not work?
As the process for managing the conditions is pretty different pre 29 and 29+, I thought it makes sense to detangle this and move the logic into separate classes to make it simpler to understand what's going on.
Yeah sounds reasonable.
I think we can make the pre 29 version even simpler, because as far as I understand now, on those API levels, we don't need any permission to enable the Wifi, but can just enable it.
Yes, I think it could be simpler, maybe even to a point where they don't need a common abstract base class.
Did you encounter devices < 29 where enabling via wifiManager.setWifiEnabled(true); did not work?
No, but I think we didn't cover this in our test survey.
added 6 commits
-
ff7674b7 - 1 commit from branch
master - f3785e01 - On API < 29 wait for Wi-Fi p2p broadcast before creating group
- 4c9b9b8c - Split ConditionManager into version for pre 29 and 29+
- 12bd2b86 - Check that intent action matches requested events
- 022174b6 - Add UiThread annotations in HotspotManager
- 44f55191 - Keep flag whether receiver is registered
Toggle commit list-
ff7674b7 - 1 commit from branch
added 5 commits
- 535735a5 - On API < 29 wait for Wi-Fi p2p broadcast before creating group
- a7123be1 - Check that intent action matches requested events
- 5997327a - Add UiThread annotations in HotspotManager
- f52b1dfc - Keep flag whether receiver is registered
- 86f8da0e - Split ConditionManager into version for pre 29 and 29+
Toggle commit listadded 1 commit
- ef253650 - Split ConditionManager into version for pre 29 and 29+
added 1 commit
- 49952b28 - Further detangle HotspotFragment and ConditionManager implementations
added 1 commit
- d2d3c3f2 - Reduce method visibility in ConditionManagers
-
I have worked quite a bit more on this and am very content with the result. As a last step, I also tried moving the
ActivityResultLauncherinto theConditionManagerimplementations, which makes it possible to really encapsulate the whole condition management in there. We need to take care not to callregisterForActivityResult()in an inappropriate lifecycle state. In order to do that I now initialize the ConditionManager during construction of theHotspotFragment, where I cannot yet pass the activity as the context, which is why I added another method calledinit()which is called duringonCreateView()and is responsible for context-specific initialization work. removed review request for @grote
requested review from @grote
12 13 import static android.content.Context.WIFI_SERVICE; 14 import static org.briarproject.hotspot.UiUtils.showDenialDialog; 15 import static org.briarproject.hotspot.UiUtils.showRationale; 16 17 /** 18 * This class ensures that the conditions to open a hotspot are fulfilled on 19 * API levels < 29. 20 * <p> 21 * Be sure to call and {@link #onRequestWifiEnabledResult()} when you get the 22 * {@link ActivityResult}. 23 * <p> 24 * As soon as {@link #checkAndRequestConditions()} returns true, 25 * all conditions are fulfilled. 26 */ 27 public class ConditionManager28 implements ConditionManager { -
I'm definitely not very glad with the 28 and 29 suffixes as they are inconsistent. I'm not aware of any good convention. On cs.android.com, I can find some examples where thinks are called
SomethingEclairandSomethingHoneycomb.We could do something similar but that would lead us to
ConditionManagerJellyBeanas JellyBean is our minimum supported version and - unfortunately as the code names for Android version got very boring recently,ConditionManagerAndroid10 changed this line in version 19 of the diff
142 130 statusView.setText(state.getError()); 143 131 } 144 132 145 @Override 146 public void onStart() { 147 super.onStart(); 148 conditionManager.resetPermissions(); 149 } 150 -
I tried to simplify the permissions logic as much as possible such that the user of the
ConditionManageronly needs to callcheckAndRequestConditions()and make that method really do what it name suggests, check the permissions and if some are missing request them. I also thought it makes sense if it's safe to call this as often as one likes without causing problems or using up extra resources.So when the user clicks the button, the internal state of the ConditionManager gets reset. Then when the fragment first calls
checkAndRequestConditions(), the location permission will definitely be checked using thelocationRequest. It can bring up the dialog if the permission has really been lost in the meantime and trigger the callback once it has been granted, otherwise it will trigger the callback right away, which isHotspotFragment'sstartWifiP2pHotspot()which will then in turn callcheckAndRequestConditions(). Assuming that the location permission is then granted the next check is for the wifi "permission", but it doesn't check thewifiSettingfield first, instead it really checks ifwifiManager.isWifiEnabled()returns true. Only if that returns false we'll examine the internal state with thewifiSettingfield. -
I tried to simplify the permissions logic as much as possible
We use the approach of resetting permission state in two other permission controllers in the code, so when deviating here, we need to be sure not to introduce regressions.
So when the user clicks the button, the internal state of the ConditionManager gets reset.
Why do we then even need to keep internal state?
-
We use the approach of resetting permission state in two other permission controllers in the code, so when deviating here, we need to be sure not to introduce regressions.
Hmm, I find fearing regressions a bit extreme in this case, as in OK, there are other places in the codebase that are doing similar things, but we are not changing those here, so we cannot break the existing permission managers by introducing different behavior here. The situation would be different I we were working on shared code between the permission managers, but we are not.
Still, I can relate to trying to be consistent with how to do things across the codebase.
I think an important thing to point out is that resetting the permissions on button tap is stricter than checking only during
onStart(). I think erring on being too strict here is better erring on the opposite as I think checking once too often is preferable than checking once too seldom (because checking too seldom can result in a crash, checking once too often however doesn't cause any severe issues the way the condition manager is designed). I think I added the reset of state on click when I noticed that I can disable the Wifi without havingonStart()reset the permissions (by dragging down the system bar from the top and disabling wifi there). So when you start the hotspot, stop it and then disable Wifi using that method, the internal state would still be stuck atwifiSetting = Permission.GRANTEDwhich would then not be checked once more if tapping the button again.However since then I've also changed the internal
ConditionManagerbehavior in a way such that we're actually no longer relying onwifiSetting = Permission.GRANTEDin order to proceed, but checkwifiManager.isWifiEnabled()every timecheckAndRequestConditions()is called viaareEssentialPermissionsGranted(). So I think we can actually try going back to resetting permissions duringonStart()in order to be consistent and it shouldn't be a problem. I'll try that now and test if it works without issues. changed this line in version 14 of the diff
-
Hmm, I find fearing regressions a bit extreme in this case, as in OK, there are other places in the codebase that are doing similar things, but we are not changing those here, so we cannot break the existing permission managers by introducing different behavior here.
Sure, I am not talking about breaking other managers, but our ConditionManager here.
The permission logic is quite tricky and not very well documented upstream. It took some time to get it right, especially when to show the rationale and the denial dialog. I am just worried that it will break here when changing what we had before and was known to work.
I think an important thing to point out is that resetting the permissions on button tap is stricter than checking only during onStart(). I think erring on being too strict here is better erring on the opposite
Yeah I am especially worried about resetting it too often, especially when it comes to detection of whether the permission was finally denied by the user or if we have a chance to trigger the dialog once more.
as I think checking once too often is preferable than checking once too seldom (because checking too seldom can result in a crash, checking once too often however doesn't cause any severe issues the way the condition manager is designed).
How can permissions change in practice without going through
onStart()though? If there is an issue with that, we need to fix it in other places as well.I think I added the reset of state on click when I noticed that I can disable the Wifi without having onStart() reset the permissions (by dragging down the system bar from the top and disabling wifi there).
Yeah that's a good point, because Wi-Fi is not actually a permission, but we treated it as such.
So I think we can actually try going back to resetting permissions during onStart() in order to be consistent and it shouldn't be a problem. I'll try that now and test if it works without issues.
Thanks!