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

Expose arguments provider as a parameter for customization #531

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

agologan
Copy link

This is a proposal for enabling users to pass in arguments during viewModel creation which may not originate from arguments/Mavericks.KEY_ARG
I realize this might not fit the current design direction but alternatively writing an extension that facilitates this requires @OptIn(InternalMavericksApi::class)

NOTE: This proposal is a breaking change as the trailing lambda has now changed from keyFactory to argsProvider

@gpeal
Copy link
Collaborator

gpeal commented Mar 19, 2021

@agologan Out of curiosity, what is the use case here?

@agologan
Copy link
Author

One use case for this is integrating Mavericks with 3rd party libraries which may create your fragment with specific arguments.

E.g. providing arguments to a fragment in a navigation graph.
To integrate with Mavericks now we could define:

 <fragment
        android:id="@+id/detailFragment"
        android:name="com.myapp.DetailFragment">
        <argument
            android:name="mavericks:arg"
            app:argType="com.myapp.DetailArgs" />
 </fragment>

So arguments get passed to your state's constructor, but this fails if you wanna use implicit deeplinks.

 <fragment
        android:id="@+id/detailFragment"
        android:name="com.myapp.DetailFragment">
        <argument
            android:name="id"
            app:argType="string" />
        <argument
            android:name="action"
            app:argType="string" />
        <deepLink app:uri="https://mydomain/detail/{id}?action={action}" />
 </fragment>

With the proposed change you could now do:

@Parcelize
data class DetailArgs(
    val id: String?,
    val action: String?
) : Parcelable {
    companion object {
        fun with(args: Bundle?): DetailArgs? =
            DetailArgs(
                args.getString("id", null),
                args.getString("action", null)
            )
    }
}

class DetailFragment: Fragment(), MavericksView {
  val viewModel: DetailViewModel by fragmentViewModel( argsProvider = { DetailArgs.with(arguments) })
}

Maybe there's a better way to approach this, looking forward to your feedback.

@elihart
Copy link
Contributor

elihart commented Mar 20, 2021

I think this is a reasonable use case and is a decent approach. Another way could be to have a companion factory method on the viewmodel, but I think doing this in the fragment is a better abstraction, since the viewmodel shouldn't really be aware of where the arguments come from.

I do wonder if there should also maybe be a global setting, perhaps in mavericks initialization, where you can set a default args provider so that if you want to use a special key everywhere in your app you don't need to do this override on every page

@agologan
Copy link
Author

I think the question is how we create the initialState from those arguments rather than where they come from.
The target here is enabling that creation without assuming a set structure facilitated by Mavericks.KEY_ARG.
So if we were to get a bundle in one of the companion factory's methods and then create args out of it would be just fine.

On that note I just realized an approach that works today is this:

companion object : MavericksViewModelFactory<DetailViewModel, DetailViewState> {
  override fun initialState(viewModelContext: ViewModelContext) =
    DetailViewState(DetailArgs.with((viewModelContext as FragmentViewModelContext).fragment.arguments))
}

Regarding the global setting if we let users customize their arguments provider they could define their own global implementation to reuse.

@elihart
Copy link
Contributor

elihart commented Mar 22, 2021

Yes, the initialState approach is what I was thinking of. Given that that works already do you still want to be able to customize the argsProvider to the viewmodel property delegate?

if possible, instead of changing the delegate I would be in favor of just adding the ability to set a global argsProvider for initial state, since each app likely has a single approach that it uses.

@agologan
Copy link
Author

This PR is just a proposal, whether it fits the current design it's up for you to decide, but having the argsProvider injectable just feels like it was meant to be 😏

The initialState solution by downcasting the context is a bit wonky, and feels hacky but it works. (Maybe it could be added to the docs)

The global provider doesn't work well in the navigation use-case because each destination has it's own set of separate arguments which would become a separate parcel. That's why you'd want to customize each of the "boxing" processes.

@elihart
Copy link
Contributor

elihart commented Mar 23, 2021

The global provider doesn't work well in the navigation use-case because each destination has it's own set of separate arguments which would become a separate parcel. That's why you'd want to customize each of the "boxing" processes.

I would be curious what we might be able to build to make it so this use case doesn't have to be customized for each type of arguments. There is probably something we could do with reflection like 1) get the argument type from the state constructor 2) look at argument constructors or companion functions that take a bundle 3) automatically create arguments from the bundle via this reflection path that is reusable for all argument types

Maybe this is a bit too much reflection magic, but having to manually specify argument creation on each fragment is a red flag of boiler plate to me

@agologan
Copy link
Author

Or we could just have a state constructor with bundle.
In this specific case having another parcel act as an intermediary is just unneeded complexity.

@elihart
Copy link
Contributor

elihart commented Mar 24, 2021

we could just have a state constructor with bundle.

yeah, that is a cool idea

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.

None yet

3 participants