Refactor: state-aware ViewModel and app flow #37
Conversation
|
Ci fails is my fault, hopefully improvements will come up in the next few days. This looks great and so much! I think we will benefit a lot from a more solid design here in the long term, and it will probably solve a lot of edge cases we have not even experienced yet! Thanks so much and looks forward to look into more detail and test it. |
|
Sorry for the closing/reopening, I tried to trigger the updated CI, but Github always runs the workflow in the branch, so you'd have to rebase with the new one to see it hopefully passing. Thanks for all the great work! |
…appstate. Add ConfigurationViewModel to manage appstate flow. Use ConfigurationViewModel in MainActivity and launch slideshow only if state is not adbConnected. Convert AdbViewModel to AdbManager class.
… for all permission pages (WifiPage, FinalPage, etc)
…onnectedFinishOnboarding state and move preferences checks to slideshowmanager
|
I noticed there's a regression in the pairing flow so I'm going to put this back in draft mode while I fix it :( It looks like I got a little greedy with the autoconnect part. |
rocodes
force-pushed
the
states
branch
2 times, most recently
from
868c03a to
dab2009
Compare
|
ok @TheZ3ro I think I'm ready for you now :) |
| AppState.NeedWirelessDebuggingAndPair, AppState.TryAutoConnect, AppState.AdbConnecting -> return SlideshowPageData(title = stringResource(R.string.slideshow_wireless_title), | ||
| description = stringResource(R.string.slideshow_wireless_description), | ||
| icon = Icons.Filled.Build, | ||
| buttonText = if (state == AppState.NeedWirelessDebuggingAndPair) stringResource(R.string.slideshow_wireless_button) else stringResource(R.string.notification_adb_pairing_working_title), |
There was a problem hiding this comment.
the notification_adb_pairing_working_title isn't perfect (it should be something like "trying to connect to adb"), but just used this for now to avoid having a ton of very similar strings to translate.
| text = when (appState.value) { | ||
| AppState.AdbScanning -> stringResource(R.string.home_scanning_button) | ||
| AppState.AdbConnected -> stringResource(R.string.home_scan_button) | ||
| AppState.TryAutoConnect, AppState.AdbConnecting -> stringResource(R.string.notification_adb_pairing_working_title) |
There was a problem hiding this comment.
(as with above, notification_adb_pairing_working_title could be improved on here to ~ "trying to connect to adb" but just tried to reuse existing strings and avoid more translation burden until things are more finalized)
|
Apparently I've entered a state where I successfully pair with ADB but the state is not updated thus it is asking to perform another pairing. This happens both on a phisical device Pixel 4a and an emulator. I'm trying to debug what is happening 
|
|
Ok wow I've understood the problem here. Basically, when you disable developer options "USB debugging" and "Wireless debugging" gets disabled. 
Thanks @rocodes for the awesome PR! |
|
I tested on both Android 11, 12 and 16 in the emulator and the behavior seems pretty solid and way more reliable than before! Thanks for all the effort in this. I didn't look at the code, but I was curious about this which seems a duplicate slide and that goes back automatically, it doesn't really affect the process but it seems like an extra click for the user |
|
Thank you both for your feedback :) @lsd-cat: Thanks for the video. It looks like it's trying to autoconnect first (which is why the button text says "Pairing with adb" and then switches to "Pair"), then it fails and asks you to re-pair. If you're starting fresh (no prior Bugbane connections stored in Wireless Debugging) it shouldn't happen, but maybe there's a case I didn't catch, I'll try to repro. @TheZ3ro: clarification about what you found, are you testing on a physical device by running the app while it's plugged and in usb debug mode, or are you seeing this issue even when you push the apk to the device via usb, then disconnect the phone, then run the app? I'll try to reproduce it as well, or if you have steps to reproduce the failed connection issue it would be helpful too. I think I know what the last couple issues are, thanks for the testing and I'll see what I can do. |
Add TryAutoConnect state to AppState and include adbManager logic to support it. Add additional transitional adb and app states. Improve wifi connectivity monitor checks. Use checkState in AdbManager to report connection.
…ebuggingAndPair from autoconnect states Check for wireless adb directly in configuration manager using content resolver. Include navigation to build number when launching settings. Refactor ConfigurationManager to use contentobservers and listen for and emit configuration changes. Refactor ConfigurationViewModel to combine stateflows of components and calculate state. Deprecate checkUpdateState since state is automatically emitted on value change. Refactor slideshow manager so that it emits updates.
slideshowactivity and check onResume() for updated Slideshow state. Keep ScanScreen in the backstack if permissions screen is re-launched from slideshow. Use renamed AppState AdbConnectedScanning. SlideshowActivity: add logging, use AppState.hiddenStates() to filter onboarding screens.
…ebugging AppState. Add button strings.
Simplify MainActivity slideshow vs acquisition screen selection logic.
rocodes
force-pushed
the
states
branch
2 times, most recently
from
50cad6e to
37b15ce
Compare
|
okay, back after a brief intermission :) I think I addressed all the review feedback:
also
|
| AppState.TryAutoConnect, AppState.AdbConnecting, -> return SlideshowPageData(title = stringResource(R.string.notification_channel_adb_pairing), //todo | ||
| description = stringResource(R.string.notification_adb_pairing_working_title), | ||
| icon = Icons.Filled.Build, | ||
| buttonText = "please wait", //todo |
There was a problem hiding this comment.
(I didn't know what the text should be here, so it's a placeholder - suggestions (+italian translation plz) welcome. The user may never see this screen, but it may show briefly depending how fast the user leaves the Settings menu after doing the wifi pairing, for example.)
There was a problem hiding this comment.
Thanks! I'd go for "Waiting for ADB pairing" and "In attesa dell'accoppiamento ADB" (@TheZ3ro yes Android uses "accoppiamento" a lot, see this).
I think I'd keep the button clickable though, for the following scenario: if the user exits the pairing process, or closes the application, than at the next start of the app it is still waiting for pairing, but the user has no way to get redirected again to the correct screen (though the app behavior seemed very solid, keeping the pairing service running or restarting it and allowing the user to complete the flow no matter in which state they interrupted).
There was a problem hiding this comment.
thank you :) added the text in 5f3f356, and also pushed a commit to make sure the pairing service notification is killed if the app is exited via the slideshow.
All that by way of saying, if you can get things into a state where the user is stuck on a screen that doesn't automatically resolve to a connection success or error (and then the pairing screen), that's a state machine bug that I should fix. The connecting / tryautoconnect states are always supposed to automatically resolve to a state where the user can take actions.
|
I did some tests on Android 11/12 and 15/16 and it seemed really solid to recover from weird conditions. I'll do a last round of tests today but it looks really good! |
wireless is enabled. Catch edge case where user has manually removed prior wireless pairing authorizations. Skip autoconnect if we received a pairing exception.
…ed to autoconnect
|
I think I've addressed everything :) For followup work in another pr, I'm suggesting:
|
| } | ||
| } | ||
|
|
||
| configViewModel.adbManager.watchCommandOutput().observe(this) { output -> |
There was a problem hiding this comment.
I think this can be removed since it's useless, wdyt?
There was a problem hiding this comment.
Yep I think it is a leftover from the libadb demo app we started with!
There was a problem hiding this comment.
sorry I was wrong, that is the listener on the output of the QuickForensics result.
maybe we can just clean that out
TheZ3ro
left a comment
TheZ3ro
left a comment
There was a problem hiding this comment.
This is much much more reliable and elegant than the old canSkip Slideshow, I really like this approach. LGTM
3 participants
Ready for testing / review (cc @TheZ3ro).
It's....kind of a lot?
getState(), the viewmodel can also observe other components and be responsive to their changes: right now, there is one for adb connectivity status and one rough one for wifi connectivity status -- so for example disabling wifi mid-flow doesn't require a check on every page, it will emit the NeedsWifi state and the app will show the slideshow starting at wifi (and skipping any states it doesn't need again).Still todo:
The autoconnect logic needs some further looking. If autoconnect fails the app requires re-pairing, which may be a regression compared to previous behaviour.Other notes:
Maybe controversially, I decided on the configViewModel as a singleton scoped to the application's lifetime. My reasoning is that we don't want multiple instances of it either (we want one adbManager / adb instance, one wifi connectivity listener, etc), and we don't actually want to bake in view-specific functions (i.e. it's not an
onButtonPressedhandler bound to any specific screen), and we want one source of truth for the overall appstate. I think this is fine for our purposes, but it does mean that if we wanted screen-specific viewmodels, they should be a separate viewmodel object, and there should be no references to short-term ui components held by the configurationviewmodel, to avoid memory leaks. This is documented in comments.Fixes #23
Fixes #11
Refs #12