square / cycler

Apache License 2.0
791 stars 27 forks source link

Compatibility with shared transitions #21

Open Rawa opened 4 years ago

Rawa commented 4 years ago

Hello,

First off thanks for a really nice library! I've been working with this library over that last few days and it's been a breeze. I currently hit a problem and would like to hear your point of view of it.

Imagine the following setup: Fragment A (with RecyclerView and Recycler obj) -> Fragment B (DetailFragment). Then using a shared element transition from Fragment A to Fragment B, for details how this is done see this article

Currently I create the Recycler object in onCreateView of Fragment A, because adopt need a RecyclerView instance. ( I hope this is correct? )

Once navigation from A -> B -> A happens the view of Fragment A will be destroyed and get created again when the user presses back. Along with the view the Recycler object will also be recreated due to it needing the newly created RecyclerView. This also means the adapter will be recreated but w/o it's old data. In order to make the transition to work one have to populate the Recycler object with the old data that was used before the view was destroyed. Normally the adapter would be created separately and this data would be retained inside and not recreated with the view.

How do we handle a new view being created with this library? Currently my workaround for this looks something along the ways like this:

    private lateinit var recycler: Recycler<DevicesRow>
    private var data = emptyList<ItemRow>().toDataSource()

    override fun onCreateView(
        inflater: LayoutInflater,
        container: ViewGroup?,
        savedInstanceState: Bundle?
    ): View {
        binding = AFragmentBinding.inflate(inflater)
        val recyclerView = binding.recyclerView
        recycler = Recycler.adopt(recyclerView) {
             ...
        }
        recycler.data = data
        postponeEnterTransition()
        rv.doOnPreDraw {
            startPostponedEnterTransition()
        }
    }

    override fun onDestroyView() {
        super.onDestroyView()
        data = recycler.data
    }

    override fun onResume() {
        // I'm starting to observe my presenter here
    }

    override fun onPause() {
        // Stopping to observe my presenter here
    }

Worth noting is that onResume() won't be called unless startPostponedEnterTransition() is called first in onCreateView() or onViewCreated(), if we want the animation to work the adapter also needs to have the data when it is called. The solution works for now, but it doesn't really feel like it is the optimal way to recreate the entire Recycler object along with the view?

helios175 commented 4 years ago

Hello! I've been a little offline since then. I'll look into this soon. I'm not terribly familiar with transitions but happy to learn. And thanks!

helios175 commented 4 years ago

Now I understand it a little better.

A big disclaimer: in Square we use square/workflow to orchestrate the transitions between screens so we don't have to deal with this AFAIK. Anyway I can offer you how I would approach this issue. Hopefully it can shed some light.

In general for views we have two steps: creation and update (bind). I found out that it's not only a concept applied to RecyclerViews but to everything UI really. We've been doing some internal work around declarative UIs and the concept proves useful once and again.

For what I understand, when you are going to perform a transition back to the screen you need it both created and updated, so when it animates you already see the data there. That's up to how you handle transitions, regardless of what you use for your UI, RecyclerView or not.

Now: how can we model creation vs update might influence how to implement the transition.

RecyclerView's have an intermediate object, the Adapter. You told me that beforehand you stored the Adapter somewhere and plugged it in back. Now you don't have that, and that's the problem. Right?

As I see it you have two options to make it fit into creation vs update.

RecyclerView is:

RecyclerView | Adapter | Data.

So far you were treating the adapter as data:

Creation: RecyclerView
Update: Adapter + Data

so you kept it. And now you cannot do it anymore.

Way 1: row configuration as part of the UI creation

You don't treat your adapter as data anymore, but as part of your UI creation/definition. In that case the data object you would store would be the datasource you plug in, or the data list (and optionally the extraItem).

That means that the creation code would adopt(...) { config rows } while the update would just plug the data, for instance:

row.update {
  data = myDataSource
}

That would work just fine.

This assumes the configuration of the RecyclerView – the row definitions, hence, the Adapter – is part of the UI and doesn't change with the data. That might represent a problem if you want to use the same UI/layout for different row declarations but it's not that common.

Way 2: row configuration as part of the UI data

Imagine you still want to treat your adapter as part of the data.

What Recycler.adopt(...) does is just create a specialized Adapter and plug it to a RecyclerView. You could still decide that adapter is part of the data and keep it in your view stack somewhere retrieving it later.

How would you do that if you need to call Recycler.adopt(..) { config... }?

Those nice braces are just Kotlin's syntactic sugar. What really happens is you are calling adopt with all the parameters, and that config block is just the last parameter. It's a lambda.

Recycler.adopt(recyclerView, block = { ... })

So you could – instead of storing the adapter – store that lambda with you:

val configLambda: Recycler.Config<ItemType>.() -> Unit

// create a new one
configLambda = { // this: Recycler.Config<ItemType>
  ...
}

And use it when you want to update the screen:

onCreate(...) { val recyclerView = ... Recycler.adopt(recyclerView, configLambda) }

That lambda is created the first time, and then stored and retrieved wherever you're storing your current Adapter.

Notice two things:

1) You shouldn't call adopt each time you want to update the recycler once the screen is created. That works, but it doesn't do any data diffing, it just recreates everything inside, and can be an issue. Anyway that's exactly what happens when you reassign RecyclerView.adapter. I'm just saying not to use this for standard / inside-the-current-screen data updates. Use recycler.update { data = ... }.

2) I don't remember if Recycler avoids re-setting the adapter reference once that's already set (there's a reason but that's another story). You can ensure it will be set if you previously set it to null and then call adopt. That set-only-if-null might go away, but this would be bullet-proof.

So your onCreate would look like:

onCreate { val recyclerView = ... recycler.adapter = null // just in case configLambda = / a new one, or the stored one if you are coming back / recycler = Recycler.adopt(recyclerView, configLambda) }

What I'm saying basically is: Adapter == configLambda. You can do with that lambda, what you did with the adapter.

Way 3 (it doesn't work but...)

I was thinking a third way is just to store and reassign the adapter. That would work (because effectively the config lambda becomes an adapter). But you would lose the ability to update the recycler, because you wouldn't have the reference to the Recycler object (the one returned by adopt).

So let's not go there.


What is my suggestion?

I'd go for solution (1) because I think that the row definition (how data is rendered) can be considered part of the screen definition –part of what it is, not part of what data it's showing. In the same way that a TextView decides that it will show one line of text with certain styling, and the data is just the text to show.

Anyway, if you plan to reuse that screen for different types of configurations, or it suits you better to keep the adapter/config-as-data approach, use the config as a lambda approach.

I can attest that it's a solid approach that we are even using in our incursions into declarative UI (when declaring a recycler we set two things: configLambda and data). I mean: you can do that and know it's not a hack but a well-supported thing that nobody plans to change.

Rawa commented 4 years ago

Thank you for your detailed reply!

Maybe it wasn't clear in my example but in OnResume I would update my data by starting my presenter and binding it to a function in the view, e.g render(viewState: ViewState), with some Rx or similar to do the updating of the view.

I also share the view of Way 1 being at the best way to set it up with Cycler. In normal RecyclerView the adapter would not be tied to the lifecycle of the view and thus internally keep the state (dataset). This would allow for simple shared transition by just attaching the adapter again if the fragment is paused and the view destroyed by calling RecyclerView.setAdapter() (Way 3)

In Cycler we would recreate the entire adapter along with the view each time. Thus also killing that stored/cached state(dataset). This is why, in the example above, I stored the dataSource in a class variable, the same would have to be done with the extraItem as well. From what I can tell the Recycler object is tied to the lifecycle of the view, there is no way to adopt another RecyclerView or create the Recycler object w/o providing a RecyclerView (=correct?).

Thus, from what I can tell, to allow for a shared transition one must manually store the last state in the Fragment as done in my first post so it could be set during the creation of the view.

Sorry if there is any bad formatting in my post above it is verylate here and I'm writing this on my cellphone :-) I'll look into this closer tomorrow, thanks yet again.

helios175 commented 4 years ago

Hello there!

Coming back to supporting the cycler library.

If you are still interesting in this: this are my current thoughts.

What you are trying to improve is:

Some assumptions and considerations (correct me if I'm wrong):

First I'd say that's not a big issue (unless I'm missing a problem here) because calling Recycler.adopt and the creation of the adapter done by the library is not expensive. The processing of the data happens anyway: when you set the adapter to a new RecyclerView it will need to recreate the visible views.

OTOH: creating a new adapter might mean that the cached views (the ones that are recycled) are lost. And if you just call Recycler.create or adopt it will creates its new adapter and start over.

So I propose the following:

I guess that would be nice and won't collision with anything else.

One question out of my lack of knowledge: those objects you are temporarily storing (the adapter you were storing) don't need to be serializable/parcelable right? If not, this should be straightforward.... but if yes this might need some minor adjustments (the Config lambda should be stored... or should be a parcelable object, the Recycler should be built with that, and I guess the adapter in case of serialization should be recreated).