Introduce compose.ui dependency and add Modifier to Ui.Content() #422
Conversation
If people want the old error behavior, they can make a composable that just errors instead of drawing
|
|
||
| /* | ||
| Diagram of what goes into generating a function! | ||
| - State parameter is _optional_ and can be omitted if it's static state. |
There was a problem hiding this comment.
note for the future: this is matching the previous behavior, but also... does this kind of situation even need to use circuit vs just being a regular composable function? I think it only matters if you have a presenter with no state but can receive UI events, which we currently don't support doing without a state.
| ui<StateType> { state, modifier -> Function(state = state, modifier = modifier, <assisted params>) } | ||
| ──────────────────────────────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| Diagram generated with asciiflow. You can make new ones or edit with this link. |
There was a problem hiding this comment.
open to good url shorteners that aren't ephemeral!
| private val MODIFIER = ClassName("androidx.compose.ui", "Modifier") | ||
| private val CIRCUIT_INJECT_ANNOTATION = | ||
| ClassName("com.slack.circuit.codegen.annotations", "CircuitInject") | ||
| private val CIRCUIT_PRESENTER = ClassName("com.slack.circuit", "Presenter") | ||
| private val CIRCUIT_PRESENTER_FACTORY = CIRCUIT_PRESENTER.nestedClass("Factory") | ||
| private val CIRCUIT_UI = ClassName("com.slack.circuit", "Ui") | ||
| private val CIRCUIT_UI_FACTORY = CIRCUIT_UI.nestedClass("Factory") | ||
| private val CIRCUIT_UI_STATE = ClassName("com.slack.circuit", "CircuitUiState") | ||
| private val SCREEN = ClassName("com.slack.circuit", "Screen") | ||
| private val NAVIGATOR = ClassName("com.slack.circuit", "Navigator") | ||
| private val CIRCUIT_CONTEXT = ClassName("com.slack.circuit", "CircuitContext") |
There was a problem hiding this comment.
nit: the IDE is complaining that all of these either don't match a regex or contain an "_" in the name. FACTORY is ok which I assume is because it's a const. Can we suppress or rename these to address the warnings?
There was a problem hiding this comment.
What's the warning? Mine is fine with it
circuit-codegen/src/main/kotlin/com/slack/circuit/codegen/CircuitSymbolProcessorProvider.kt
Outdated
Show resolved
Hide resolved
circuit-codegen/src/main/kotlin/com/slack/circuit/codegen/CircuitSymbolProcessorProvider.kt
Outdated
Show resolved
Hide resolved
| Diagram of what goes into generating a function! | ||
| - State parameter is _optional_ and can be omitted if it's static state. | ||
| - When omitted, the argument becomes _ and the param is omitted entirely. | ||
| - <StateType> is either the State or CircuitUiState if no state param is used. |
There was a problem hiding this comment.
Thought: I could see someone reading the generated code and getting confused by a param of type CircuitUiState. I'm wondering if it might make sense to create object NoState : CircuitUiState and use it here instead.
There was a problem hiding this comment.
unless we promote NoState to be a public and reusable type, I don't think it makes much of a difference. I have wondered if theres room for a class StaticState<UiEvent : CircuitUiEvent>(val eventSink: (UiEvent) -> Unit) that could be useful
| ) | ||
|
|
||
| /** Computes the data needed to generate a factory. */ | ||
| // Detekt and ktfmt don't agree on whether or not the rectangle rule makes for readable code. | ||
| @Suppress("ComplexMethod", "LongMethod") | ||
| @Suppress("ComplexMethod", "LongMethod", "ReturnCount") |
There was a problem hiding this comment.
Looking ahead: I think we should refactor this method (and maybe others in this file). It's pretty long and unwieldy right now :-\
circuit-codegen/src/main/kotlin/com/slack/circuit/codegen/CircuitSymbolProcessorProvider.kt
Outdated
Show resolved
Hide resolved
This is a breaking API change to
Ui.Content()to add aModifierparameter. There are breaking changes, but if using code gen it's fairly minimal.public interface Ui<UiState : CircuitUiState> { - @Composable public fun Content(state: UiState) + @Composable public fun Content(state: UiState, modifier: Modifier) }We've been getting stuck on this for awhile now when trying to bridge to UI land and finally pulling the trigger on this to ease a bunch of things.
At a high level, this replaces the toe-hold
ScreenUiwe've been keeping around as modifiers give us a means of plumbing down extra UI metadata, so I've removed it from the API.We can also offer a default onUnavailableContent API now, so I've made it non-nullable in
CircuitConfigusing that default. If people want the previous erroring behavior, they can still achieve this with a custom function of their own.One place this hits friction is UI libraries that don't use Modifiers (i.e. Mosaic), but it's easy enough to just ignore that parameter coming from
ui { state, _ -> ... }.Other things
Boxes that were shims for this functionality missing.modifier: Modifier = Modifierin theUi.Content(...)signature.This does add the compose UI dependency to circuit core, but after a lot of thought I think this is ok. Our android artifact already imposed it for all intents and purposes, this just drops that facade and embraces the fact that this is a UI architecture and not just a business logic layer using compose runtime only.
Supersedes #406.
Helps #50 too.
Eases state sharing via modifiers. It would be easy to build modifiers that build upon each other with this and we no longer have a missing UI link.