SpeziOnboarding

[pauljohanneskraft]

SpeziOnboarding

♻️ Current situation & Problem

Link any open issues or pull requests (PRs) related to this PR. Please ensure that all non-trivial PRs are first tracked and discussed in an existing GitHub issue or discussion.

⚙️ Release Notes

Add a bullet point list summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.
Include code snippets that provide examples of the feature implemented or links to the documentation if it appends or changes the public interface.

📚 Documentation

Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.
You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using in-line documentation.

✅ Testing

Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.
This section describes important information about the tests and why some elements might not be testable.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

• I agree to follow the Code of Conduct and Contributing Guidelines.

[codecov]

Codecov Report

Attention: Patch coverage is 33.97341% with 447 lines in your changes missing coverage. Please review.

Project coverage is 39.67%. Comparing base (74cc37c) to head (4eb35c8).

Files with missing lines	Patch %	Lines
...module/onboarding/spezi/consent/ConsentDocument.kt	0.00%	92 Missing ⚠️
...rding/spezi/consent/OnboardingConsentComposable.kt	0.00%	85 Missing ⚠️
...zi/module/onboarding/spezi/flow/OnboardingStack.kt	32.31%	35 Missing and 9 partials ⚠️
...onboarding/spezi/SequentialOnboardingComposable.kt	64.14%	24 Missing and 9 partials ⚠️
...nboarding/spezi/OnboardingComposableInformation.kt	21.63%	24 Missing and 5 partials ⚠️
...le/onboarding/spezi/OnboardingComposableBuilder.kt	0.00%	21 Missing ⚠️
...i/module/onboarding/spezi/OnboardingInformation.kt	64.41%	18 Missing and 3 partials ⚠️
...spezi/module/onboarding/spezi/OnboardingActions.kt	50.00%	16 Missing and 3 partials ⚠️
...pezi/consent/ConsentDocumentExportConfiguration.kt	0.00%	19 Missing ⚠️
...ezi/module/onboarding/consent/ConsentPdfService.kt	0.00%	12 Missing ⚠️
... and 10 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #129      +/-   ##
============================================
- Coverage     39.96%   39.67%   -0.29%     
- Complexity      834      844      +10     
============================================
  Files           296      311      +15     
  Lines         11442    12010     +568     
  Branches       1718     1830     +112     
============================================
+ Hits           4572     4764     +192     
- Misses         6400     6733     +333     
- Partials        470      513      +43     

Flag	Coverage Δ
uitests	36.79% <32.80%> (+0.25%)	⬆️
unittests	29.61% <1.19%> (-1.81%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...n/kotlin/edu/stanford/bdh/engagehf/MainActivity.kt	0.00% <ø> (ø)
...lin/edu/stanford/bdh/engagehf/navigation/Routes.kt	36.37% <ø> (+3.04%)	⬆️
...anford/bdh/engagehf/onboarding/OnboardingModule.kt	0.00% <ø> (ø)
...ezi/module/onboarding/OnboardingNavigationEvent.kt	0.00% <ø> (ø)
...le/onboarding/spezi/flow/OnboardingStackBuilder.kt	100.00% <100.00%> (ø)
...odule/onboarding/spezi/consent/ConsentViewState.kt	50.00% <50.00%> (ø)
...ule/onboarding/spezi/flow/IllegalOnboardingStep.kt	0.00% <0.00%> (ø)
.../spezi/module/onboarding/consent/ConsentUiState.kt	60.00% <42.86%> (-21.25%)	⬇️
.../onboarding/spezi/consent/ConsentDocumentExport.kt	0.00% <0.00%> (ø)
...ezi/module/onboarding/core/OnboardingComposable.kt	69.57% <69.57%> (ø)
... and 15 more

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74cc37c...4eb35c8. Read the comment docs.

[eldcn]

Let's please start creating correct modules as part of this PR already, you can use the script that I have shared. In iOS this is defined under Spezi package. Create a module under modules:spezi and define this interface there, and add modules:spezi dependency where you need this

[pauljohanneskraft]

I would like to wait for this until SpeziAccount is merged, hope that's fine. Especially, I would want them to be self-contained (if this makes sense), i.e. to have no dependencies outside of their own package ecosystem and that will most likely mean that we will need to put some stuff into the packages that do not exist in the iOS version and that are Android-specific (e.g. SpeziTheme, etc).

[pauljohanneskraft]

What I can do, is simply creating a single package for this interface, so that it isn't part of core:utils, if that's good enough

[eldcn]

Updating the usage to PersonNameComponents implies that we should at least make use of some of its capabilities, e.g. formatting, or usage of NameTextField. We are doing none of those here, and I personally see no need to replace.

---

I hope that you are also able to see some of the concerns that I raised in the SpeziViews PR in terms of usability of mutating the fields of person name components builder directly inside compose. Even if you would use a NameTextField, updates of the fields would not cause a recomposition of the SignatureCanvas for instance. The approach that we followed there just covers a single case of simply updating the fields, however real use cases as this one here, require updates of other components as well once for instance the name changed.

[pauljohanneskraft]

Fine with reverting this to be two strings, instead? i.e. givenName and lastName - but only since they do not appear in the public API.

[eldcn]

This speaks for a downside of passing a mutable reference of the name components in NameTextField, updates of the fields would not cause a recomposition of the canvas

[pauljohanneskraft]

see above - recomposition would work though, since the onAction updates the state with a new PersonNameComponents instance every time, since PersonNameComponents only contains constants.

[eldcn]

this is a business logic and should not get decided in the ui, please leave the consent action as it was and define the id and export configuration in the view model

[pauljohanneskraft]

Let's talk about this, since it would make sense to stay aligned with the iOS interface here, and this information would need to be injected into the viewModel in some way

[eldcn]

There is no need to create so many wrapper layouts inside. We can add every content as an item in the lazy column:

@Composable
fun OnboardingComposable(
    modifier: Modifier = Modifier,
    title: @Composable () -> Unit = {},
    content: @Composable () -> Unit,
    action: (@Composable () -> Unit)? = null,
) {
    LazyColumn(
        modifier = modifier
            .padding(Spacings.medium),
        horizontalAlignment = Alignment.CenterHorizontally
    ) {
        item { title() }
        item { content() }

        action?.let {
            item {
                VerticalSpacer(height = Spacings.small)
                it()
            }
        }
    }
}

[pauljohanneskraft]

I will need to check whether this is actually equivalent in appearance and would need to check how to possibly adapt it, but I have changed it for now to this as it seems more reasonable.

[eldcn]

This is not being used. Please follow all the usages where we pass this and remove from the whole chain of calls

[pauljohanneskraft]

You are right, we should make sure it is visualized.

[eldcn]

This is also just being created and not being used

[pauljohanneskraft]

This is created inside ConsentDataSource and would usually be used inside a ConsentContraint-conforming Standard.

[eldcn]

this can be private, and other properties immutable vals

[pauljohanneskraft]

You are right about the navController to be changed to private, but the other properties may be mutated by use of the LaunchedEffect inside the OnboardingStack composable.

[eldcn]

why do we attempt to navigate when appending? This won't work if the route hasn't been registered to the graph first, same for the append method below

[pauljohanneskraft]

That's just how that component is meant to work... otherwise, I'm not sure, if I understand. One creates an OnboardingStack and then contained composables have access to this OnboardingNavigationPath, with which they can navigate to another item in the original definition of the OnboardingStack without knowing which composable is hidden behind it.

If a step id is unknown, that's not supposed to happen and should be avoided by a user (i.e. developer using this). Since we check for an existing step in the array and make sure to create the NavGraph ourself, we actually already throw an error in line 45.

[eldcn]

Would propose the following here and usage of rembemberSaveable as that is what also rember nav controller useses:

@Composable
fun OnboardingStack(
    modifier: Modifier = Modifier,
    onComplete: () -> Unit = {},
    startAtStep: String? = null,
    content: OnboardingStackBuilder.() -> Unit,
) {
    val navController = rememberNavController()
    val navigationPath = rememberSaveable(navController, startAtStep, content) {
        OnboardingNavigationPath(navController, startAtStep, content)
    }

    navigationPath.NavHost(modifier = modifier)

    LaunchedEffect(navController) {
        navController.currentBackStackEntryFlow.collect { entry ->
            if (entry.destination.route == null) {
                onComplete()
            }
        }
    }
}

As per navigation path:

class OnboardingNavigationPath internal constructor(
    private val navController: NavHostController,
    startDestination: String?,
    builder: OnboardingStackBuilder.() -> Unit,
) {
    private val steps = OnboardingStackBuilder().apply(builder).steps.also {
        if (it.isEmpty()) error("No steps provided")
    }
    private val startDestination = startDestination ?: steps.first().id
    // rest of the content
    
    // nav host
    @Composable
    fun NavHost(modifier: Modifier) {
        CompositionLocalProvider(LocalOnboardingNavigationPath provides this) {
            NavHost(
                modifier = modifier,
                navController = navController,
                startDestination = startDestination,
            ) {
                composable(
                    route = customRoute("{id}"),
                    arguments = listOf(navArgument("id") { type = NavType.StringType })
                ) { backStackEntry ->
                    val step = backStackEntry.arguments?.getString("id")?.let { stepId ->
                        customSteps.firstOrNull { it.id == stepId }
                    }
                    step?.content?.let { it() } ?: IllegalOnboardingStep()
                }

                for (step in steps) {
                    composable(step.id) { step.content() }
                }
            }
        }
    }

I just replicated your logic in the custom route, but I personally see that logic not being needed

[pauljohanneskraft]

The logic for custom steps is used in composables that are exclusively used inside OnboardingStacks and that want to push new composables onto the stack that aren't exposed to a developer and therefore cannot be exposed in that OnboardingStack definition. Just gives more flexibility than the being strictly limited to the composables defined inside the OnboardingStack.