feat(run-v2): link installation #90
Conversation
composeApp/src/androidMain/kotlin/org/ooni/probe/MainActivity.kt
Outdated
Show resolved
Hide resolved
| android:scheme="https" /> | ||
| </intent-filter> | ||
|
|
||
| <intent-filter android:autoVerify="true"> |
There was a problem hiding this comment.
I'm getting a Missing required elements/attributes for Android App Links warning here.
There was a problem hiding this comment.
android wants the scheme below to be http/https
There was a problem hiding this comment.
Then maybe we don't need the autoVerify here? Or we can still verify the ooni scheme?
composeApp/src/commonMain/kotlin/org/ooni/probe/domain/BootstrapTestDescriptors.kt
Outdated
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/ooni/probe/domain/RunNetTest.kt
Outdated
Show resolved
Hide resolved
composeApp/src/commonMain/kotlin/org/ooni/probe/ui/descriptor/AddDescriptorScreen.kt
Outdated
Show resolved
Hide resolved
| ) | ||
| } | ||
| }.onFailure { | ||
| onError() |
There was a problem hiding this comment.
You can also tell the Screen to display the error, and use the onBack here instead. Snackbars are tied to the Activity, so they survive if you switch screens.
There was a problem hiding this comment.
onError shows an error message before going back
| onEvent(AddDescriptorViewModel.Event.AutoRunChanged(!state.allTestsSelected())) | ||
| }, | ||
| ) { | ||
| Checkbox( |
There was a problem hiding this comment.
I found out there's a TriStateCheckbox that I used on my run tests PR.
| onBack() | ||
| }, | ||
| fetchDescriptor = { | ||
| withContext(backgroundDispatcher) { |
There was a problem hiding this comment.
You should hide this detail inside the fetchDescriptor, but the engine.httpDo call is already running on the backgroundDispatcher.
| saveTestDescriptors = { | ||
| saveTestDescriptors.invoke(it) | ||
| // TODO(aanorbel): show toast/snackbar | ||
| onBack() |
There was a problem hiding this comment.
I would prefer if we don't have logic in the Dependencies class where it's hard to find. Why not call onBack inside of the viewModel?
Co-authored-by: Sérgio Santos <me@sergiosantos.info>
aanorbel
changed the title
| android:scheme="https" /> | ||
| </intent-filter> | ||
|
|
||
| <intent-filter android:autoVerify="true"> |
There was a problem hiding this comment.
Then maybe we don't need the autoVerify here? Or we can still verify the ooni scheme?
| onCancel = { | ||
| snackbarHostState?.let { | ||
| showSnackbar( | ||
| scope = scope, | ||
| snackbarHostState = it, | ||
| message = cancelMessage, | ||
| ) | ||
| } | ||
| onBack() | ||
| }, | ||
| onError = { | ||
| snackbarHostState?.let { | ||
| showSnackbar( | ||
| scope = scope, | ||
| snackbarHostState = it, | ||
| message = errorMessage, | ||
| ) | ||
| } | ||
| onBack() | ||
| }, | ||
| saveTestDescriptors = { | ||
| saveTestDescriptors.invoke(it) | ||
| snackbarHostState?.let { | ||
| showSnackbar( | ||
| scope = scope, | ||
| snackbarHostState = it, | ||
| message = installCompleteMessage, | ||
| ) | ||
| } | ||
| onBack() | ||
| }, | ||
| fetchDescriptor = { | ||
| fetchDescriptor(descriptorId) | ||
| }, |
There was a problem hiding this comment.
I feel it's not a good practice to have any sort of logic in the Dependencies class. We're not expecting to find it there, it's not testable, and it will make this file to be bloated very quickly.
Ideally this logic should be inside the ViewModel. Even the fetchDescriptor argument should just be something like fetchDescriptor = fetchDescriptor::invoke.
| snackbarHostState?.let { | ||
| showSnackbar( | ||
| scope = scope, | ||
| snackbarHostState = it, | ||
| message = cancelMessage, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Do we need to show a snackbar saying the installation was cancelled? It felt a bit obvious to the user.
There was a problem hiding this comment.
I dont think we need it but i am trying to do backwards compatibility at once
| ) | ||
| } | ||
| }.onFailure { | ||
| onError() |
There was a problem hiding this comment.
This failure is not being logged, it might be useful.
There was a problem hiding this comment.
Yeah. I am working on the update as we speak.
| .onOpenURL { url in | ||
| handleDeepLink(url: url) | ||
| } |
| ) = ComposeUIViewController { | ||
| App( | ||
| dependencies = dependencies, | ||
| deepLink = deepLinkFlow.collectAsState(null).value, |
There was a problem hiding this comment.
This will only get a a deepLink once. Or does opening a deep link in iOS restart the whole app?
There was a problem hiding this comment.
Having some difficulties with this.
| val snackbarHostState = LocalSnackbarHostState.current | ||
| val cancelMessage = stringResource(Res.string.LoadingScreen_Runv2_Canceled) | ||
| val errorMessage = stringResource(Res.string.LoadingScreen_Runv2_Failure) | ||
| val installCompleteMessage = stringResource(Res.string.AddDescriptor_Toasts_Installed) |
There was a problem hiding this comment.
If the ViewModel ends up with the code to trigger a snackbar, the actual code that fetches the strings and the SnackbarHostState should be in the View.
Fixes: #52