Introduce handling for identifiable onboarding views

[felixschlegel]

Introduce handling for identifiable onboarding views

:recycle: Current situation & Problem

Closes #43. Previously, the OnboardingStack identified views by their type name for ordered navigation operations. By introducing the protocol OnboardingIdentifiableView we require views to set an id: String allowing for multiple views of the same type to be part of an OnboardingStack.

:gear: Release Notes

• OnboardingIdentifiableView (new):
  • new protocol OnboardingIdentifiableView conforming to View and Identifiable
  • add default implementation for id that uses the view's type name as its identifier
• OnboardingNavigationPath (changed):
  • add new func append(identifiableView: any OnboardingIdentifiableView) that allows navigating to any view that conforms to the new OnboardingIdentifiableView protocol

Example Usage:

struct TestView1: OnboardingIdentifiableView {
    // id = "TestView1" if not explicitly specified

    var body: some View {...}
}

struct TestView2: OnboardingIdentifiableView {
    var id: String = "my-custom-identifier"

    var body: some View {...}
}

struct OnboardingFlow: View {
    var view: some View {
         OnboardingStack {
               TestView2()
               TestView1()
               TestView2(id: "second-test-view-identifier")
         }
    }
}

:books: Documentation

Documentation added to all new public interface members.

:white_check_mark: Testing

• add new UI Test testIdentifiableViews that clicks through a set of views with both the default view id and custom view identifiers

:pencil: Code of Conduct & Contributing Guidelines

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

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

[felixschlegel]

Regarding the ViewModifier (.onboardingIdentifier(_: String)) as proposed in #43 , here are my thoughts:

We both want the view's type name as a default value for id and a ViewModifier that allows setting the id property of the implementing view.

In the case that the implementing view does not specify var id: String, we default to OnboardingIdentifiableView's default implementation of id which is a computed property that returns the view's type name and hence the implementing view has no internal storage for the var id: String since it does not exist. This means that we cannot apply the onboardingIdentifier(_ id: String) to the implementing type since it does not have an id property.

extension OnboardingIdentifiableView {
    public func onboardingIdentifier(_ id: String) -> Self {
        var copy = self
        copy.id = id
        return copy
    }
}

At the moment I can imagine two ways out of this:

1. Omit ViewModifier onboardingIdentifier and force users to set id through the implementing view's initialiser / struct definition if var id: String is added to struct
2. Have ViewModifier but force all views that implement OnboardingIdentifiableView to have a var id: String property, omitting the default implementation that sets the id to the view's type name

[Supereg]

I just stumbled across the PR and had some random thoughts. I thought I just add them here. Hope this adds to some of the discussion points. If there are already other directions you went and discussed just ignore these points 🙈

Does it make senes to avoid implementing a custom OnboardingIdentifiableView protocol for that and instead just add an additional overload for a type that conforms to View & Identifiable. We just need to Hashable requirement of the ID associated type, right? And as the default value for the id property is entirely defined but the type itself, we could derive that information at runtime (e.g., have two overloads one for arbitrary, generic Views V where we just ruse "\(V.self)" as the identifier and one for views that conform to Identifiable)?

As you explored the onboardingIdentifier approach. Is there an ability to use mechanisms like SwiftUI Preferences or even Environment observables (which are class based) to register or collect all identifiers from a given subview? This would feel very native to SwiftUI (e.g., similar to the tag modifier). However, this would probably require some reengineering as the information would only be available once the views are getting rendered (if I'm not wrong).

Otherwise, a different approach could be to use the modifier(_:) to implement a ViewModifier that itself conforms to Identifiable. This way you could add an overload to the result builder like buildExpression<V: View>(_ expression: ModifiedContent<YourModifier, V>) -> ... that would allow to extract the identifier (YourModifier being either a specific modifier implementation or just any generic ViewModifier that conforms to Identifiable). Limitation here would be that we only allow the identifier to the placed in the outermost level (random note at the end: maybe having a return type for that modifier that doesn't conform to View and is only accepted by the OnboardingStack view builder might be a nice restriction as well to enforce proper usage of the modifier?).

[philippzagar]

To add my thoughts to @Supereg's answer (thanks a lot for your input!):

I definitely agree with the typealias for View & Identifiable, no need to create a separate protocol for it.

I briefly explored using the SwiftUI-native .tag() and .id() ViewModifiers when writing #43. Sadly, there seems to be no way to build on top of the SwiftUI infrastructure here, at least not one that I could find. Probably, SwiftUI Preference Keys would be the way to go then, I would guess that this wouldn't require that much of a reengineering effort (in contrast to what @Supereg mentioned). I guess the information becomes available as soon as the OnboardingStack itself is rendered, which is early enough to continue with the current structure of the OnboardingNavigationPath. Or am I missing something? As a small side note: As we may generalize the OnboardingStack sooner or later into the SpeziViews repo, we should think about nested Views within the OnboardingStack and how to best support them. Just as a thought-provker, didn't think it through.

[Supereg]

I definitely agree with the typealias for View & Identifiable, no need to create a separate protocol for it.

Wouldn't even bother to create a type alias for that. Just View and Identifiable conformance that's it.

[felixschlegel]

Hello @Supereg and @philippzagar,

Thank you for your thorough review!

I replaced the OnboardingIdentifiableView protocol by making implementing Views simply conform to View & Identifiable.

Furthermore, I created a new ViewModifier .id(_:) wrapping any given View in a new struct OnboardingIdentifiableView that automatically conforms to View & Identifiable and contains an id: String property set by the .id(_:) modifier.

Please let me know what you think!

[Supereg]

@PSchmiedmayer I tried to workaround our issues with codecov by adding a repository upload token and updating the pull request action. This doesn't seem to work (maybe as this PR comes from an external repository). Not sure how to best resolve this issue?

[PSchmiedmayer]

@Supereg Thank you for the review; great input!

Regarding the code coverage issue: Tokenless uploads should be supported for forks when creating PRs for public repos: https://docs.codecov.com/docs/codecov-uploader#supporting-tokenless-uploads . Might very well be that codecov is broken here, we are having the exact same flow as described in the docs.

[felixschlegel]

Hey @felixschlegel,

thanks for all the improvements here. I think the current approach turned out great and we are on a good way forward 🚀

I had a few, smaller comments here in there that would add some refinements. Only one bigger comment, where I'm currently not sure if the implementation of the id(_:) modifier actually works, but I might just be wrong and missing something.

Reach out if there are any questions or if you already discussed different solutions with Paul.

Hey @Supereg ,

Thank you so much for this great review! I left some comments inline and overhauled the entire code.

I had a few, smaller comments here in there that would add some refinements. Only one bigger comment, where I'm currently not sure if the implementation of the id(_:) modifier actually works, but I might just be wrong and missing something.

Technically, .id(_:) works but shadows the default SwiftUI .id(_:) modifier which is not good, so thanks for pointing that out! Now I renamed it to .onboardingIdentifier(_:).

Best wishes, Felix

[PSchmiedmayer]

Thank you for the detailed review @Supereg 🚀

[Supereg]

Just added a small comment on how we can fully test the new behavior: https://github.com/StanfordSpezi/SpeziOnboarding/pull/45#discussion_r1577458150

Feel free to mark all comments resolved that you addressed. Then feel free to merge 🚀

[felixschlegel]

Just added a small comment on how we can fully test the new behavior: #45 (comment)

Feel free to mark all comments resolved that you addressed. Then feel free to merge 🚀

Hey @Supereg , thanks for giving this so much thought! I implemented your suggested change so we should be good to go @PSchmiedmayer 🚀

[felixschlegel]

@Supereg @PSchmiedmayer I cannot merge since codecov fails

[PSchmiedmayer]

@felixschlegel Seems like this issue is stemming from a codecov rate limit as we are using a fork and this uses the upload API without a token. I will merge the PR despite this error, it shouldn't affect a build on our main branch or if you use a feature branch in this repo.

Thank you for all the work here and thank you for the reviews @Supereg!