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

Node mp #551

Merged
merged 103 commits into from
Aug 15, 2023
Merged

Node mp #551

merged 103 commits into from
Aug 15, 2023

Conversation

Ninjars
Copy link
Collaborator

@Ninjars Ninjars commented Jul 30, 2023

Description

Brings in the feature dev branch implementing multiplatform support for the navigation module.

Check list

  • I have updated CHANGELOG.md if required.
  • I have updated documentation if required.

Ninjars and others added 30 commits May 15, 2023 11:14
might have ramifications for testing where logcat shouldn't be referenced...
…om ParentNode

Multiplatform implementations pending...
@Ninjars Ninjars added this to the 2.0 milestone Jul 30, 2023
@Ninjars Ninjars self-assigned this Jul 30, 2023
@Ninjars Ninjars linked an issue Jul 30, 2023 that may be closed by this pull request
api(libs.kotlin.coroutines.android)
api(libs.androidx.lifecycle.common)
api(project(":appyx-navigation:appyx-navigation"))
runtimeOnly(libs.kotlin.coroutines.android)
Copy link
Collaborator

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?

Copy link
Collaborator Author

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?

Copy link
Collaborator

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

Copy link
Collaborator Author

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(
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

@@ -117,7 +113,6 @@ open class Node @VisibleForTesting internal constructor(
fun Compose(modifier: Modifier = Modifier) {
CompositionLocalProvider(
LocalNode provides this,
LocalLifecycleOwner provides this,
Copy link
Collaborator

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

Copy link
Collaborator Author

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it removed?

Copy link
Collaborator Author

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?

Copy link
Collaborator

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

Copy link
Collaborator Author

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(
Copy link
Collaborator

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(
Copy link
Collaborator

Choose a reason for hiding this comment

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

put it in common?

Copy link
Collaborator Author

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

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Supports Compose multiplatform. WebNodeHost and DesktopNodeHost provide convenience wrappers for
- Supports Compose Multiplatform. `WebNodeHost` and `DesktopNodeHost` provide convenience wrappers for

@zsoltk zsoltk merged commit 9f95179 into 2.x Aug 15, 2023
@Ninjars Ninjars deleted the node-mp branch August 15, 2023 13:05
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.

Integrate navigation multiplatform
3 participants