Migrate to Circuit #1288
Migrate to Circuit #1288
Conversation
chrisbanes
commented
May 26, 2023
•
chrisbanes
commented
- Fix navigation to settings
This won't work when we move to KMP, but necessary for now.
Enables the Compose Compiler to treat it as stable in lambda captures. See: https://issuetracker.google.com/issues/256100927
| val configuration = LocalConfiguration.current | ||
| val useBottomNavigation = configuration.smallestScreenWidthDp < 600 | ||
|
|
||
| val rootScreen by remember { | ||
| derivedStateOf { backstack.last().screen } |
| // Launch an effect to track changes to the current back stack entry, and push them | ||
| // as a screen views to analytics | ||
| LaunchedEffect(backstack.topRecord) { | ||
| val topScreen = backstack.topRecord?.screen as? TiviScreen | ||
| analytics.trackScreenView( | ||
| name = topScreen?.name ?: "unknown screen", | ||
| arguments = topScreen?.arguments, | ||
| ) | ||
| } |
There was a problem hiding this comment.
We do this with a delegating navigator inside of slack. This works too though! The delegating navigator made it easy for us to actually set this up with tracing, so we get spans as screens are pushed and popped off too
| import kotlinx.coroutines.launch | ||
|
|
||
| @OptIn(ExperimentalMaterial3Api::class) | ||
| class BottomSheetOverlay<Model : Any, Result : Any>( |
There was a problem hiding this comment.
Do you think we should offer a batteries-included component like this? Definitely know there's like 4 different implementations of this same idea floating around different repos at the moment heh
There was a problem hiding this comment.
Yeah I was thinking the same thing. Probably worth keeping as a separate artifact. It also gets tricky with Jetpack Compose vs Compose Multiplatform.
| } | ||
| } | ||
|
|
||
| suspend fun OverlayHost.showInBottomSheet( |
| object UpNextScreen : TiviScreen(name = "UpNext()") | ||
|
|
||
| abstract class TiviScreen(val name: String) : Screen { | ||
| open val arguments: Map<String, *>? = null |
There was a problem hiding this comment.
I'd be curious to learn more about this!
There was a problem hiding this comment.
These are solely there for analytics. These gets pushed up to Crashlytics along with screen views.
| @IntoSet | ||
| @Provides | ||
| @ApplicationScope | ||
| fun bindAccountPresenterFactory(factory: AccountUiPresenterFactory): Presenter.Factory = factory | ||
|
|
||
| @IntoSet | ||
| @Provides | ||
| @ApplicationScope | ||
| fun bindAccountUiFactory(factory: AccountUiFactory): Ui.Factory = factory |
There was a problem hiding this comment.
Do you think there's anything we could do to make these kind of stuff easier to use in other DI systems like kotlin-inject? We built anvil support mostly because that's just what we use, but open to supporting other stuff if there's stuff you think is reusable
There was a problem hiding this comment.
Another option is you could group these together into one composite factory, and possibly even cover multiple screens within it
There was a problem hiding this comment.
Personally I didn't have a problem with writing these.
kotlin-inject makes creating the assisted injection factory super simple, which helped a lot.
I'm not sure how much of this stuff could be transferable across DI systems tbh.
| class AccountUiPresenterFactory( | ||
| private val presenterFactory: (Navigator) -> AccountPresenter, | ||
| ) : Presenter.Factory { | ||
| override fun create(screen: Screen, navigator: Navigator, context: CircuitContext): Presenter<*>? { | ||
| return when (screen) { | ||
| is AccountScreen -> presenterFactory(navigator) | ||
| else -> null | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
For cases not using a code gen tool, anything we could do to make these simpler? Maybe a ScreenPresenterFactory(Screen,Function) API that dries some of this up?
There was a problem hiding this comment.
I thought about writing exactly that, but the only thing you're really saving is the when.
You still need a class somewhere to inject the assisted factory. I guess you could move that to the component/module instead, and just call a ScreenPresenterFactory(). I prefer the explicitness here though.
| override fun create( | ||
| screen: Screen, | ||
| navigator: Navigator, | ||
| context: CircuitContext, | ||
| ): Presenter<*>? = when (screen) { |
There was a problem hiding this comment.
Side not - these are fun interface so could it be simpler to use the function syntax?
There was a problem hiding this comment.
ehh nevermind I guess it's not possible to use class delegation with a function input. Tried it locally without success
# Conflicts: # android-app/app/src/main/java/app/tivi/AppNavigation.kt # android-app/app/src/main/java/app/tivi/ComposeScreens.kt # android-app/app/src/main/java/app/tivi/NavigationUtils.kt # android-app/app/src/main/java/app/tivi/inject/ApiComponent.kt # common/ui/compose/src/main/java/app/tivi/common/compose/ViewModel.kt # common/ui/screens/src/androidMain/kotlin/app/tivi/screens/Platform.kt # common/ui/screens/src/jvmMain/kotlin/app/tivi/screens/Platform.kt # ui/account/src/main/java/app/tivi/account/AccountUi.kt # ui/discover/src/main/java/app/tivi/home/discover/Discover.kt # ui/library/src/main/java/app/tivi/home/library/Library.kt # ui/library/src/main/java/app/tivi/home/library/LibraryUiState.kt # ui/popular/src/main/java/app/tivi/home/popular/Popular.kt # ui/recommended/src/main/java/app/tivi/home/recommended/Recommended.kt # ui/search/src/main/java/app/tivi/home/search/Search.kt # ui/search/src/main/java/app/tivi/home/search/SearchUiState.kt # ui/show/details/src/main/java/app/tivi/showdetails/details/ShowDetails.kt # ui/show/seasons/src/main/java/app/tivi/showdetails/seasons/ShowSeasons.kt # ui/trending/src/main/java/app/tivi/home/trending/Trending.kt
Yes it's hacky.
