Desktop: Introduce UI dependency #406
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Superseded by #422, but interested in pursuing the navigablecircuitcontent work in a new PR once that's in! |
ZacSweers
added a commit
that referenced
this pull request
Feb 2, 2023
This is a breaking API change to `Ui.Content()` to add a `Modifier`
parameter. There are breaking changes, but if using code gen it's fairly
minimal.
```diff
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 `ScreenUi` we'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 `CircuitConfig` using 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
- Realized our code gen artifact was sorta halfway multiplatform and
resulted in confusing KSP configuration errors. Cleaned that up and made
it just a standard JVM artifact.
- Reworked how we generate UI factories a bit in code gen to make it
simpler to maintain. Put a thorough diagram in for good measure.
- Removed redundant wrapper `Box`es that were shims for this
functionality missing.
- It's not possible in abstract composable functions to add a default
value. i.e. can't write `modifier: Modifier = Modifier` in the
`Ui.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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of using navigation on desktop requires
NavDecoration, which has a dependency on compose.uiModifier. This could also be achieved with another two mechanisms:Modifieras an argument forDecoratedContentModifierHowever, since
Modifieris such a fundamental part of Compose UI, I thought it may indeed make sense to introduce it as a core dependency. I would be happy for input.The changes here also adjust
runtime.saveableas an implementation dependency since it's not required as an API dependency, this was just a single line change, but I'd be happy to extract it to a new PR if desired.