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

[feat]: add 'screen container' API #258

Merged
merged 3 commits into from
Jan 11, 2024
Merged

Conversation

jamieQ
Copy link
Contributor

@jamieQ jamieQ commented Dec 5, 2023

Motivation

in some circumstances it is helpful to know when a type is serving as a logical 'container' for a rendering, and in particular a container of a Screen. the types currently exposed by WorkflowUI which serve this purpose are the following:

  1. AnyScreen
  2. WorkflowHostingController<ScreenType, Output>
  3. ScreenViewController<ScreenType>

since both the hosting controller & screen view controller are generic, there is no way to dynamically check if an arbitrary type is an instance without also specifying the generic parameters. this makes certain types of dynamic introspection clunky or impossible depending on code structure. to illustrate the point, consider the following somewhat contrived example:

func makeUninspectableScreenVC() -> UIViewController {
    struct LocalScreen: Screen { ... } // `LocalScreen` symbol is only visible in this function
    return ScreenViewController(screen: LocalScreen(), environment: .empty)
}

let isSVC = makeUninspectableScreenVC() is ScreenViewController<???> // no generic can be specified here that makes this true

we'd like our logical 'Screen container' types to share an interface that allows access to the underlying screen information, which can help with debugging and other runtime data collection.

Changes

  • adds the ScreenContaining protocol. this has a single requirement that conformances must provide a containedScreen value, returning an any Screen type that is the semantically 'contained' screen
  • adds conformances to this protocol to the existing screen containers in WorkflowUI
  • adds a utility protocol extension to traverse a chain of containers to find the 'leaf' screen

Checklist

  • Unit Tests
  • UI Tests
  • Snapshot Tests (iOS only)
  • I have made corresponding changes to the documentation

/// This enables referring to `any ScreenViewController` which is not possible without a protocol
/// like this because `ScreenViewController` is generic over its screen type and it's not possible to check
/// `viewController is ScreenViewController` without also specifying a specific screen type.
public protocol ScreenContaining {
Copy link
Contributor

Choose a reason for hiding this comment

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

So I see this is still a draft, but how will this work for things like navigation controllers or tab bar controllers, where they have >= 1 inner screens?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I think you may want two properties, containedScreens : [any Screen] and visibleScreens: [any Screen]?)

Copy link
Member

Choose a reason for hiding this comment

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

Those only have one current screen so it's ok for those to simply return the current "top" screen for conforming to this protocol.

The harder example is something like a split screen. In that case we may have to opt for tagging that screen with its own identity rather than trying to dig out the two individual screens its composing, but not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think the main goals of this interface are to:

  1. add parent type to ScreenViewController so that its generic parameter can be ignored without losing access to its boxed Screen value
  2. provide a mechanism for 'traversing' nested containers

i agree there are some questions about particular types of containers that might be worth further consideration, but i'm not sure if we need to deal with those questions at the moment or can punt them if this simple model works for most existing use cases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering goal 2, I agree that it would be useful to provide some guidelines around the semantic meaning of containedScreen for more than trivial cases. I don't think it's well-defined in the context of modals, for example. But not a blocker for landing this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this spelling of the protocol should be SingleScreenContaining? That leaves the door open to a ScreenContaining that can handle multiple contained screens (where SingleScreenContaining could provide its own conformance to that protocol).

I do also somewhat like calling this visibleContainedScreen (or similar) to make it clear that, for example, we’re interested in the topmost screen in a navigation stack not all the obscured screens.

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 the feedback. i've made an update to rename to the protocol to SingleScreenContaining, and its requirement to primaryScreen to better communicate the intent. also had a go at improving the associated commentary.

i plan to work on the integration story with the intent to merge this early next week. if you have additional thoughts, do let me know!

@jamieQ jamieQ marked this pull request as ready for review December 11, 2023 16:33
@jamieQ jamieQ requested review from a team as code owners December 11, 2023 16:33
/// This enables referring to `any ScreenViewController` which is not possible without a protocol
/// like this because `ScreenViewController` is generic over its screen type and it's not possible to check
/// `viewController is ScreenViewController` without also specifying a specific screen type.
public protocol ScreenContaining {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering goal 2, I agree that it would be useful to provide some guidelines around the semantic meaning of containedScreen for more than trivial cases. I don't think it's well-defined in the context of modals, for example. But not a blocker for landing this.

@jamieQ jamieQ merged commit 1e9e2bb into main Jan 11, 2024
13 checks passed
@jamieQ jamieQ deleted the jquadri/screen-container-api branch January 11, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants