-
Notifications
You must be signed in to change notification settings - Fork 61
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
Node mp #551
Conversation
…ble implementations
…d with new utilities module
Multiplatform utils
…upporting configuration
…breaks all android dependencies within
…ncerns to Android-only code
might have ramifications for testing where logcat shouldn't be referenced...
… Android lifecycle classes
… smattering of errors and warnings
…om ParentNode Multiplatform implementations pending...
…interface to compile
api(libs.kotlin.coroutines.android) | ||
api(libs.androidx.lifecycle.common) | ||
api(project(":appyx-navigation:appyx-navigation")) | ||
runtimeOnly(libs.kotlin.coroutines.android) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain this change, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a complaint raised by one of the build analysis tools. It wanted a change to the way the dependency was declared and it didn't appear to break anything, so who am I to question the computer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right, we have attachChild
, waitForChildAttached
API which use coroutines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, will revert
/** | ||
* Obtains or creates an instance of a class using the identifier from the BuildContext. | ||
*/ | ||
inline fun <reified T : Any> BuildContext.getRetainedInstance( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put the rest of ext functions to the common
module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deriving the key from the class name wasn't trivially possible in a multiplatform way, hence the functions that rely on that remaining in a jvm context.
appyx-navigation/common/src/commonMain/kotlin/com/bumble/appyx/navigation/node/Node.kt
Outdated
Show resolved
Hide resolved
@@ -117,7 +113,6 @@ open class Node @VisibleForTesting internal constructor( | |||
fun Compose(modifier: Modifier = Modifier) { | |||
CompositionLocalProvider( | |||
LocalNode provides this, | |||
LocalLifecycleOwner provides this, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's revet this and provide lifecycle so it's available in the view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. This is more than a reversion: we'll need to create a multiplatform concept of LocalLifecycleOwner and provide it, and also the lifecycle we're providing is CommonLifeycleOwner not the android lifecycle so we should probably avoid name-space collisions with Jetpack Compose's own LocalLifecycleOwner.
Perhaps by declaring this in the lifecycle folder?
val LocalCommonLifecycleOwner: ProvidableCompositionLocal<CommonLifecycleOwner?> = compositionLocalOf { null }```
private val buildContext: BuildContext, | ||
val view: NodeView = EmptyNodeView, | ||
private val retainedInstanceStore: RetainedInstanceStore, | ||
plugins: List<Plugin> = emptyList() | ||
) : NodeLifecycle, NodeView by view, RequestCodeClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because request code is an Android concept that doesn't need to be entangled with a multiplatform Node. Unless I'm off-base here, android client code can supply any RequestCodeClient impl needed at the activity level, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, RequestCodeClient
is needed on a per Node basis. We'll have to implement RequestCodeClient
in the client code for Android Nodes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in conversation with Zsolt about this there was a question as to whether we even needed this on Android for a good reason, so worth re-assessing perhaps?
/** | ||
* Obtains or creates an instance of a class using the identifier from the BuildContext. | ||
*/ | ||
inline fun <reified T : Any> BuildContext.getRetainedInstance( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one method is in common
module while other similar methods in commonAndroid
* Obtains or creates an instance of a class using the class name as the key. | ||
* If you need multiple instances of an object with the same key, do not use this extension. | ||
*/ | ||
inline fun <reified T : Any> RetainedInstanceStore.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put it in common?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T::class.java
isn't available outside the jvm build targets
appyx-navigation/web/src/jsMain/kotlin/com/bumble/navigation/integrationpoint/WebNodeHost.kt
Outdated
Show resolved
Hide resolved
...ion/common/src/commonMain/kotlin/com/bumble/appyx/navigation/node/container/ContainerNode.kt
Show resolved
Hide resolved
.../kotlin/com/bumble/appyx/navigation/node/spotlight/SpotlightObserveTransitionsExampleNode.kt
Show resolved
Hide resolved
…ommon navigation module, simplifying the dependency situation
@@ -15,7 +15,8 @@ The library is packaged as multiple artifacts. | |||
- Cares about the Node structure & Android-related functionality | |||
- Uses `:appyx-interactions` as a dependency to implement navigation using gestures and transitions | |||
- Android library | |||
- Compose multiplatform implementation is in progress, to be merged soon | |||
- Supports Compose multiplatform. WebNodeHost and DesktopNodeHost provide convenience wrappers for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Supports Compose multiplatform. WebNodeHost and DesktopNodeHost provide convenience wrappers for | |
- Supports Compose Multiplatform. `WebNodeHost` and `DesktopNodeHost` provide convenience wrappers for |
Description
Brings in the feature dev branch implementing multiplatform support for the navigation module.
Check list
CHANGELOG.md
if required.