Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce handling for identifiable onboarding views #45

Merged

Conversation

felixschlegel
Copy link
Contributor

Introduce handling for identifiable onboarding views

♻️ 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.

⚙️ 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")
         }
    }
}

📚 Documentation

Documentation added to all new public interface members.

✅ Testing

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

📝 Code of Conduct & Contributing Guidelines

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

@felixschlegel
Copy link
Contributor Author

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
Copy link
Member

Supereg commented Mar 25, 2024

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 philippzagar added the enhancement New feature or request label Apr 3, 2024
@philippzagar
Copy link
Member

philippzagar commented Apr 3, 2024

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
Copy link
Member

Supereg commented Apr 3, 2024

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 felixschlegel marked this pull request as ready for review April 12, 2024 17:44
@felixschlegel
Copy link
Contributor Author

felixschlegel commented Apr 12, 2024

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!

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

import SwiftUI

/// Wrap `Content` `View` in an `Identifiable` `View`.
public struct OnboardingIdentifiableView<Content>: View, Identifiable where Content: View {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this view private/internal as it doesn't seems like this should be used directly?
Otherwise, if that doesn't work, it might make sense to prefix it with _ (e.g., _OnboardingIdentifiableView) such that it doesn't show up in the Xcode autocompletion and documentation?

/// Unique identifier of the wrapped `View`.
public let id: String
/// Wrapped `View`.
public var body: Content
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be let probably?

/// When applying this modifier repeatedly, the outermost ``id(_:)`` counts.
/// - Parameters:
/// - identifier: The `String` identifier given to the view.
public func id(_ identifier: String) -> some View {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it have to be a String? Can't we just use an arbitrary Hashable identifier (as the Identifiable protocol requires it)?

Also, isn't the current naming conflicting with SwiftUI's id(_:) modifier or how do we ensure disambiguity?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is new public interface, make sure to build the DocC bundle once and see if we can highlight this modifier somewhere in the main topics section (or on the topics section of a nested documentation type) to ensure exploitability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a code example to the DocC documentation of OnboardingStack

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you 👍


extension View {
/// `ViewModifier` assigning an identifier to the `View` it is applied to.
/// When applying this modifier repeatedly, the outermost ``id(_:)`` counts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sene to add a Note bubble to the documentation, that this modifier only has an effect if the view modifier is directly used in the OnboardingStack view builder (maybe even demonstrate this with a short code example)?

public var body: Content
}

struct OnboardingIdentifiableViewModifier: ViewModifier {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can also be private

/// - Parameters:
/// - identifier: The `String` identifier given to the view.
public func id(_ identifier: String) -> some View {
modifier(OnboardingIdentifiableViewModifier(identifier: identifier))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my understanding this modifier is currently not tested and I have the suspicion that this doesn't work as this has a return type of type ModifiedContent<Self, OnboardingIdentifiableViewModifier> which does conform to View, but doesn't conform to Identifiable. We might need to add a conditional conformance to ModifiedContent if Content` is Identifiable, so we can detect a modified, identifiable view (e.g., also if there are other modifiers used with a normal view etc).

Also, do we ever consider Identifiable views in the @OnboardingViewBuilder, doesn't currently seem like it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out, it was naive of me to not have a test for this!
The reason why we cannot use Content: Identifiable as a type constraint here is that Content refers to the View before the modifier is applied, thus the View that we want to make Identifiable.
Therefore I extended ModifiedContent to be Identifiable when its Modifier is Identifiable (see code).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what exact reason do you want to consider Identifiable views in the OnboardingViewBuilder?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore I extended ModifiedContent to be Identifiable when its Modifier is Identifiable (see code).

Sounds great. Yes, I confused the Content associated type. Making the modifier itself conform to Identifiable is the way to go. Thank you.

For what exact reason do you want to consider Identifiable views in the OnboardingViewBuilder?

My approach would have been to construct OnboardingStepIdentifiers already within the OnboardingViewBuilder. This would eliminate the dynamic type check within the OnboardingStepIdentifier, though would also require to create a dedicated component type that stores this within the collection, instead of just using [any View].

I'm not sure if it is just me learning Swift at a time where dynamic type checks using protocols that contained associated type requirements weren't possible 🙊.

Just to illustrate my approach, you would have added expression builders as below. No need to implement, just to illustrate my train of thought here.

public static func buildExpression<Content: View & Identifiable>(_ expression: Content) -> [OnboardingView]

// bonus, would eliminate the Identifiable extension on ModifiedContent.
public static func buildExpression<Content: View, Modifier: Identifiable>(_ expression: ModifiedContent<Content, Modifier>) -> [OnboardingView]

public static func buildExpression<Content: View>(_ expression: Content) -> [OnboardingView]

/// It isn't required to declare this view within the ``OnboardingStack``.
public func append<V: View & Identifiable>(customView: V) {
let customOnboardingStepIdentifier = OnboardingStepIdentifier(
onboardingStepType: String(describing: customView.id),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we turning the Hashable id into a String here? There is no guarantee that the String representation of two Hashables that have the same hash is the same.
I would propose to change the implementation of OnboardingStepIdentifier. Making onboardingStepType to store the hash of the id of a given onboarding view (maybe renaming the property would also make sense to reflect the new semantics).

Refer to the documentation of Hasher on how to calculate the hash of a Hashable.

@Supereg
Copy link
Member

Supereg commented Apr 13, 2024

@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
Copy link
Member

@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
Copy link
Contributor Author

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

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the hard work here. 🚀 Looks good from my side 🥳

Last few comments were mainly about some minor improvements regarding documentation and some unused code we can eliminate 👍

///
/// ### Identifying Onboarding Views
///
/// Apply the `onboardingIdentifier(_:)` modifier to clearly identify a view in the `OnboardingStack`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this a DocC symbol to properly link to the modifier.

Suggested change
/// Apply the `onboardingIdentifier(_:)` modifier to clearly identify a view in the `OnboardingStack`.
/// Apply the ``SwiftUI/View/onboardingIdentifier(_:)`` modifier to clearly identify a view in the `OnboardingStack`.

You might need to verify that this is spelled correctly.

/// }
/// ```
///
/// - Note: When the `onboardingIdentifier(_:)` modifier is applied multiple times to the same view, the most recently applied identifier takes precedence.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Suggested change
/// - Note: When the `onboardingIdentifier(_:)` modifier is applied multiple times to the same view, the most recently applied identifier takes precedence.
/// - Note: When the ``SwiftUI/View/onboardingIdentifier(_:)`` modifier is applied multiple times to the same view, the outermost identifier takes precedence.

Comment on lines 23 to 26
OnboardingIdentifiableView(
id: self.id,
body: content
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I noticed just now, I think it isn't necessary to wrap this into a OnboardingIdentifiableView anymore and we can remove the view above, as the ViewModifier now conforms to Identifiable.

/// ```swift
/// struct Onboarding: View {
/// @AppStorage(StorageKeys.onboardingFlowComplete) var completedOnboardingFlow = false
///
/// var body: some View {
/// OnboardingStack(onboardingFlowComplete: $completedOnboardingFlow) {
/// MyOwnView().onboardingIdentifier("my-own-view-1")
/// MyOwnView().onboardingIdentifier("my-own-view-2")
/// }
/// }
/// }
/// ```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the example to the modifier docs. This looks great 🚀

Comment on lines 31 to 32
/// `ViewModifier` assigning an identifier to the `View` it is applied to.
/// When applying this modifier repeatedly, the outermost ``onboardingIdentifier(_:)`` counts.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DocC uses the first line of a documentation as a brief summary and embeds that e.g. into the topics section at the bottom a documentation page to provide some details of a reference type. Any text following the first empty line is put into the overview section of a symbol (see Writing symbol documentation in your source files).

I would suggest to put a single, short and descriptive sentence into the first line of a type or symbol.
Feel free to adapt to your liking.

Suggested change
/// `ViewModifier` assigning an identifier to the `View` it is applied to.
/// When applying this modifier repeatedly, the outermost ``onboardingIdentifier(_:)`` counts.
/// Assign a unique identifier to a `View` appearing in an `OnboardingStack`.
///
/// A `ViewModifier` assigning an identifier to the `View` it is applied to.
/// When applying this modifier repeatedly, the outermost ``onboardingIdentifier(_:)`` counts.

Comment on lines +28 to +29
OnboardingTestViewNotIdentifiable(text: "Leland").onboardingIdentifier("a")
OnboardingTestViewNotIdentifiable(text: "Stanford").onboardingIdentifier("b")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding tests for the modifier 🚀

Comment on lines 76 to 83
XCTAssert(app.staticTexts["Leland"].waitForExistence(timeout: 2))
XCTAssert(app.buttons["Next"].waitForExistence(timeout: 2))
app.buttons["Next"].tap()

XCTAssert(app.staticTexts["Stanford"].waitForExistence(timeout: 2))
XCTAssert(app.buttons["Next"].waitForExistence(timeout: 2))
app.buttons["Next"].tap()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We assert that these views are inserted in the right order. But are we asserting that their identifiers are set as expected within those tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment we are not, we cannot test the internal state from a UI Test so the only possibility here would be to assert the value of a TextView which would have to access the id from its wrapping OnboardingIdentifiableViewModifier which is all too much of an effort for such a basic assertion imo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would highly recommend that we add a test for that. It's very implicit that this works currently (we rely on the Identifiable extension of ModifiedContent) and it might easily break without someone noticing with future changes.

I agree, that it is pretty clumsy to do this using a UI test. I would propose the following unit test that would verify this behavior and prevent future regressions. Just add it as part of the unit tests of the swift package.
I would argue that we can make the firstOnboardingStepIdentifier property internal (currently it is private) to support this unit test.

    @MainActor
    func testOnboardingIdentifierModifier() throws {
        let stack = OnboardingStack {
            Text("Hello World")
                .onboardingIdentifier("Custom Identifier")
        }

        let identifier = try XCTUnwrap(stack.onboardingNavigationPath.firstOnboardingStepIdentifier)

        var hasher = Hasher()
        hasher.combine("Custom Identifier")
        let final = hasher.finalize()
        XCTAssertEqual(identifier.identifierHash, final)
    }

@PSchmiedmayer
Copy link
Member

Thank you for the detailed review @Supereg 🚀

Motivation:

Closes StanfordSpezi#43.

Modifications:

* introduce `OnboardingIdentifiableView` `protocol` requiring `id` property
* use view's type name if `id` not explicitly specified
* create new UI Test for the new idenfitiable onboarding views
Modifications:

* replace `protocol` `OnboardingIdentifiableView` with simple
  conformance to `View & Identifiable`
* create a new view modifier `.id(_:)` wrapping any `View` in a `struct
  OnboardingIdenfiableView` which is `View & Identifiable` and contains
  the specified `id` as a property
* hoist `OnboardingStepIdentifier` overloaded intializers into different
  `OnboardingNavigationPath.append()` methods
* remove `OnboardingIdentifiableTestViewDefault`
Modifications:

* identify `OnboardingStack` views through `id: any Hashable` instead of
  `id: String`
* rename `.id(_:)` view modifier to `.onboardingIdentifier(_:)` to
  disambiguate from SwiftUI's own `.id(_:)` view modifier
* refactor `OnboardingStepIdentifier` to store a custom hash of the
  given view / view id
* add `extension ModifiedContent` to conditionally make it
  `Identifiable`
* adjust UITest to verify that the `.onboardingIdentifier(_:)` works
* add `.onboardingIdentifier(_:)` example in `OnboardingStack` DocC
  documentation
* make sure to only expose API that has to be exposed
Modifications:

* add links in DocC
* remove superflous type `OnboardingIdentifiableView`
@Supereg
Copy link
Member

Supereg commented Apr 24, 2024

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 🚀

Modifications:

* change `OnboardingNavigationPath.firstOnboardingStepIdentifier`
  access level from `private` to `internal`
@felixschlegel
Copy link
Contributor Author

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
Copy link
Contributor Author

@Supereg @PSchmiedmayer I cannot merge since codecov fails

@PSchmiedmayer
Copy link
Member

@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!

@PSchmiedmayer PSchmiedmayer merged commit 8d6dda3 into StanfordSpezi:main Apr 30, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Customize identifier of View within the OnboardingStack
4 participants