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

Introduced RetainedInstanceStore to keep data during config change #361

Conversation

LachlanMcKee
Copy link
Collaborator

Description

Copied the RetainedInstanceStore concept from RIBs so that objects can be retained during a configuration change.

Also re-added the MVICore screens to the sandbox to demonstrate the behaviour

Check list

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

@LachlanMcKee LachlanMcKee force-pushed the add-ability-to-retain-on-configuration-change branch from dcf4973 to 626a79b Compare February 18, 2023 20:41
@LachlanMcKee LachlanMcKee force-pushed the add-ability-to-retain-on-configuration-change branch from 626a79b to af8cbce Compare February 18, 2023 22:47
@CherryPerry
Copy link
Collaborator

CherryPerry commented Feb 18, 2023

Originally I wanted to suggest to adopt #322 instead, but after I checked API I do not want anymore (ViewModels API is a hell, you can't just pass ViewModel factory as a lambda).

But my suggestion is still to apply its approach regarding ViewModelStoreOwner. I do not like that and how we moved ID management from Node itself to BuildParams. So instead, what do you think if we make it in the same way as ViewModelStoreOwner – provide RetainedInstanceStore API only directly from Node and not outside. We can capture all required parameters in Builder and pass them as a lamda to Node. From the first look it worse, but it is a good opportunity to 1) to do less potential API breaking changes now and in the future, we can always to go from inside-node approach to outside-node approach (but not other way) 2) check if direct dependency injection makes sense (partially dropping Builder and completly Interactor class as it won't have access to the feature anymore).

Would like to invite more people to discuss.

@LachlanMcKee LachlanMcKee force-pushed the add-ability-to-retain-on-configuration-change branch from af8cbce to 3685fa6 Compare February 18, 2023 22:56
@LachlanMcKee
Copy link
Collaborator Author

@CherryPerry I'm happy if we can come up with another solution. This PR allows Devs to retained objects within the builder which is useful of you're using MVICore.

The ViewModel approach in the other PR doesn't seem to have this functionality.

Perhaps we could support both? As devs using the builder/interactor approach need a solution as well

@LachlanMcKee LachlanMcKee force-pushed the add-ability-to-retain-on-configuration-change branch from 3685fa6 to 18fd6ae Compare February 19, 2023 13:20
@@ -22,6 +26,18 @@ data class BuildContext(
)
}

fun <T : NodeCustomisation> getOrDefault(defaultCustomisation: T) : T =
val identifier: String by lazy {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make this internal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add a comment at the very least saying to not use it, Incubating?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We cannot make this internal unfortunately. Not sure if I should mark this with a comment as this will be part of the 1.0 API

private val map: MutableMap<String, MutableMap<KClass<*>, ValueHolder<*>>> = HashMap()

@Suppress("UNCHECKED_CAST")
override fun <T : Any> get(nodeId: String, clazz: KClass<*>, disposer: (T) -> Unit, factory: () -> T): T =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use a string key instead of the class so that multiple instance of the same class can be used.

@LachlanMcKee LachlanMcKee force-pushed the add-ability-to-retain-on-configuration-change branch from eca3a97 to 5d629d2 Compare February 20, 2023 13:01
@LachlanMcKee LachlanMcKee force-pushed the add-ability-to-retain-on-configuration-change branch 2 times, most recently from eb7fd4d to 50d47f4 Compare February 20, 2023 14:14
@LachlanMcKee LachlanMcKee force-pushed the add-ability-to-retain-on-configuration-change branch from 50d47f4 to af69578 Compare February 20, 2023 14:24
@LachlanMcKee LachlanMcKee force-pushed the add-ability-to-retain-on-configuration-change branch from af69578 to fe8d961 Compare February 20, 2023 14:51
@LachlanMcKee LachlanMcKee merged commit 781d834 into bumble-tech:1.x Feb 20, 2023
@LachlanMcKee LachlanMcKee deleted the add-ability-to-retain-on-configuration-change branch February 20, 2023 15:13
@KovalevAndrey KovalevAndrey mentioned this pull request Mar 2, 2023
2 tasks
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.

3 participants