Reusing controller after backpress

[alex-marochko]

Hi! Thanks for your great lib. Can't find out - is there any way to keep Controller un-destroyed after backpress? I'm getting java.lang.IllegalStateException: Trying to push a controller that has already been destroyed. when trying to use it again.

[shchurov]

@alex-marochko It's not possible atm afaik. We had the same issue, the workaround was just to keep that controller in a different router and show/hide its container.

Generally I think it's correct behavior: there's no way for Conductor to know if you want to use that controller again or it can be GC'ed. Also usually you want controller to stay "alive" just to keep its view state. In that case nothing prevents you from just hiding it, it will stay in the memory anyway.

[EricKuck]

Like @shchurov said, there's no way to reuse a destroyed controller. What's the use case for even wanting to do this?

[alex-marochko]

@EricKuck , I see that destroyed controller can't (and shouldn't) be reused. But maybe there's option not to destroy it on backpress?

I have a list of routes. Clicking at item opens 'expensive' controller containing SupportMapFragment. Then, user goes back to list and he can click another item. So, I'd prefer to keep controller with map for some time (still using normal backstack behavior), but backpress destroys it.

[Zhuinden]

setRetainViewMode?

[alex-marochko]

@Zhuinden , unfortunately, setRetainViewMode can keep view in controller when controller is detached, but it can't help when controller's gonna be destroyed by backpress.

[paramsen]

This seems like a point of no return! So using any of pop operations in the Router destroys the Controller immediately? Ugh! I have a couple of pretty heavyweight views so recreating them is a bad idea when the user navigates around (takes ~500ms so feels like a react native app or smth).. seems like the only alternative is to revert to Fragments again?

[sockeqwe]

Isnt it that the use case for setRetainViewMode?

Alternatively, you can create a pool for your heavy weighted views and in Controller.onCreateView() return one from pool (if available). Don't forget to clear the pool on orientation changes, otherwise you have memory leaks.

Pär Nils Amsen notifications@github.com schrieb am Fr., 24. Nov. 2017, 18:18:

This seems like a point of no return! So using any of pop operations in the Router destroys the Controller immediately? Ugh! I have a couple of pretty heavyweight views so recreating them is a bad idea when the user navigates around (takes ~500ms so feels like a react native app or smth).. seems like the only alternative is to revert to Fragments again?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bluelinelabs/Conductor/issues/233#issuecomment-346879200, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrqMblIiT9i1z3WvGwgmSnjKYP2kyks5s5whbgaJpZM4MEOsE .

[paramsen]

@sockeqwe Good call! I have RETAIN_DETACH, but the controller gets destroyed on pop anyway. I'll give it a shot with a "pool" instead, guess it makes sense to use a WeakReference in this case and recreate the hierarchy should it be GC:ed :) Thx!

[paramsen]

@sockeqwe Aight I came up with a quite elegant solution, just gonna drop the idea here in case someone need it:

val cache: MutableMap<Int, WeakReference<View>>

onCreateView(/*...*/) {
    if(retainViewMode == RetainViewMode.RETAIN_DETACH) {
        val cached = cache[layoutRes]?.get()

        if(cached != null) {
            containerView = cached
        } else {
            containerView = inflater.inflate(layoutRes, container, false)
            cache[layoutRes] = WeakReference(containerView)
        }
    } else {
        containerView = inflater.inflate(layoutRes, container, false)
    }
    //...
}

[sockeqwe]

Using WeakReferences is a bad idea because just because a screen orientation happens doesn't mean that gc run and weak reference is released. With your implementation it is very likely that you have a memory leak. Example:

1. Activity starts in portrait. FooViewController is shown.
2. screen orientation change (gc doesn't run) --> FooViewController puts his View in the "cache" (I would rather call it pool than cache).
3. Activity is recreated in landscape . FooViewController uses View from cache, BUT the cached view has been instantiated in portrait and holds a reference to "PortraitActivity". Then you use this View within "LandscapeActivity" --> Memory leak: portrait activity can't be garbage collected because FooViewController uses cached View, so holds a reference to cached View and cached View itself holds a reference to "PortraitActivity".

So what I would do is something like that:

typealias LayoutRes = Int // like R.layout.foo
typealias Pool = Set<View> 

class MainAcrivity() : Activity() {
  val pools : Map<LayoutRed, Pool>()  = ... 

  ...
  override fun onDestroy (){
    pools.clear() // release pooled views by removing strong references
  }
}

And then in your controller:

class FooController : Controller {

   override fun onCreateView(...) : View{
       val cachedView = activity?.pools[R.layout.foo]?.firstOrNull()
   return if (cachedView != null)
            cachedView
        else
           inflater.inflate(R.layout.foo, ...)
   }

  override fun onDestroyView () {
     activity?.pool[R.layout.foo].put(view)
  }
}

Some thoughts: I would write my own data structure Pools rather than Map<LayoutRes, Pool> with a nice API (internally uses ArrayMap and some other optimized data structures). Also you could use an AsyncLayoutInflater (i.e. in Activity.onCreate()) and put it the view into the pool if you know beforehand that the user reaches very likely the FooViewController with your expensive View (expensive in terms of layout inflation takes long).

[paramsen]

I'd argue that using WeakRefs is perfect in this case since the cache/pool is owned by the current Activity, so the refs are released on orientation change (or at least not referenced anymore). So for each orientation change/recreation of the activity, the pool gets reinstantiated properly. I've been profiling it for a while (messing around, changing orientation ~30 times), and the heap/allocs keep stable throughout! Maybe I misunderstood you, but there shouldn't be any leaks as long as the cache isn't persisted between layout changes/activity recreations?

The idea of using an AsyncLayoutInflater is spot on, I haven't seen that class around actually. It would be cool to "precache" some of the view hierarchies that are animated in later, because Android really doesn't mix inflation and animations well. Been looking for something like that for a while now!

fre 24 nov. 2017 kl 23:56 skrev Hannes Dorfmann notifications@github.com:

Using WeakReferences is a bad idea because just because a screen orientation happens doesn't mean that gc run and weak reference is released. With your implementation it is very likely that you have a memory leak. Example:

1. Activity starts in portrait. FooViewController is shown.
2. screen orientation change (gc doesn't run) --> FooViewController puts his View in the "cache" (I would rather call it pool than cache).
3. Activity is recreated in landscape . FooViewController uses View from cache, BUT the cached view has been instantiated in portrait and holds a reference to "PortraitActivity". Then you use this View within "LandscapeActivity" --> Memory leak: portrait activity can't be garbage collected because FooViewController uses cached View, so holds a reference to cached View and cached View itself holds a reference to "PortraitActivity".

So what I would do is something like that:

typealias LayoutRes = Int // like R.layout.footypealias Pool = Set

class MainAcrivity() : Activity() { val pools : Map<LayoutRed, Pool>() = ...

... override fun onDestroy (){ pools.clear() // release pooled views by removing strong references } }

And then in your controller:

class FooController : Controller {

override fun onCreateView(...) : View{ val cachedView = activity?.pools[R.layout.foo]?.firstOrNull() return if (cachedView != null) cachedView else inflater.inflate(R.layout.foo, ...) }

override fun onDestroyView () { activity?.pool[R.layout.foo].put(view) } }

Some thoughts: I would write my own data structure Pools rather than Map<LayoutRes, Pool> with a nice API (internally uses ArrayMap and some other optimized data structures). Also you could use an AsyncLayoutInflater (i.e. in Activity.onCreate()) and put it the view into the pool if you know beforehand that the user reaches very likely the FooViewController with your expensive View (expensive in terms of layout inflation takes long).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bluelinelabs/Conductor/issues/233#issuecomment-346904199, or mute the thread https://github.com/notifications/unsubscribe-auth/AOhw0jsps94Q-9OvlnVT3W_oPLhc_3aAks5s50mRgaJpZM4MEOsE .

--

paramsen.com | Pär Amsen | +46 765601973 | Sweden

[sockeqwe]

I guess I have misunderstood your code snipped above, wasn't

val cache: MutableMap<Int, WeakReference>

part of a ViewController?

If no (i.e. cache is part of activity, not a viewcontroller) you are right, WeakReference works.

Pär Nils Amsen notifications@github.com schrieb am Sa., 25. Nov. 2017, 00:05:

I'd argue that using WeakRefs is perfect in this case since the cache/pool is owned by the current Activity, so the refs are released on orientation change (or at least not referenced anymore). So for each orientation change/recreation of the activity, the pool gets reinstantiated properly. I've been profiling it for a while (messing around, changing orientation ~30 times), and the heap/allocs keep stable throughout! Maybe I misunderstood you, but there shouldn't be any leaks as long as the cache isn't persisted between layout changes/activity recreations?

The idea of using an AsyncLayoutInflater is spot on, I haven't seen that class around actually. It would be cool to "precache" some of the view hierarchies that are animated in later, because Android really doesn't mix inflation and animations well. Been looking for something like that for a while now!

fre 24 nov. 2017 kl 23:56 skrev Hannes Dorfmann <notifications@github.com

:

Using WeakReferences is a bad idea because just because a screen orientation happens doesn't mean that gc run and weak reference is released. With your implementation it is very likely that you have a memory leak. Example:

1. Activity starts in portrait. FooViewController is shown.
2. screen orientation change (gc doesn't run) --> FooViewController puts his View in the "cache" (I would rather call it pool than cache).
3. Activity is recreated in landscape . FooViewController uses View from cache, BUT the cached view has been instantiated in portrait and holds a reference to "PortraitActivity". Then you use this View within "LandscapeActivity" --> Memory leak: portrait activity can't be garbage collected because FooViewController uses cached View, so holds a reference to cached View and cached View itself holds a reference to "PortraitActivity".

So what I would do is something like that:

typealias LayoutRes = Int // like R.layout.footypealias Pool = Set

class MainAcrivity() : Activity() { val pools : Map<LayoutRed, Pool>() = ...

... override fun onDestroy (){ pools.clear() // release pooled views by removing strong references } }

And then in your controller:

class FooController : Controller {

override fun onCreateView(...) : View{ val cachedView = activity?.pools[R.layout.foo]?.firstOrNull() return if (cachedView != null) cachedView else inflater.inflate(R.layout.foo, ...) }

override fun onDestroyView () { activity?.pool[R.layout.foo].put(view) } }

Some thoughts: I would write my own data structure Pools rather than Map<LayoutRes, Pool> with a nice API (internally uses ArrayMap and some other optimized data structures). Also you could use an AsyncLayoutInflater (i.e. in Activity.onCreate()) and put it the view into the pool if you know beforehand that the user reaches very likely the FooViewController with your expensive View (expensive in terms of layout inflation takes long).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub < https://github.com/bluelinelabs/Conductor/issues/233#issuecomment-346904199 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AOhw0jsps94Q-9OvlnVT3W_oPLhc_3aAks5s50mRgaJpZM4MEOsE

.

--

paramsen.com | Pär Amsen | +46 765601973 | Sweden

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/bluelinelabs/Conductor/issues/233#issuecomment-346907900, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnrpMYo7fx_gwJyW-4LW1eLCR3hSIjks5s51m1gaJpZM4MEOsE .

[paramsen]

Aah, yes I left that out in my snippet hoping it would be simpler to understand - it does make a big difference though! The cache instance is injected into the Controller, but instantiated in the Activity, like this (pseudo code):

class MyActivity: Activity { val kodein = MyApplication.baseKodein.extend { provider { cache } } val cache: MutableMap<Int, WeakReference> = mutableMapOf() //... } class MyController: Controller { val childKodein = (activity as MyActivity).kodein val cache by childKodein.instance() override fun onCreateView() { //read/write to view cache etc. } }

I believe that it is sufficient enough, and that it shouldn't hinder the views from being GC:ed when the Activity is released. Wrapping that MutableMap up in a more intuitive api might be a good idea as you mentioned!

lör 25 nov. 2017 kl 04:06 skrev Hannes Dorfmann notifications@github.com:

I guess I have misunderstood your code snipped above, wasn't

val cache: MutableMap<Int, WeakReference>

part of a ViewController?

If no (i.e. cache is part of activity, not a viewcontroller) you are right, WeakReference works.

Pär Nils Amsen notifications@github.com schrieb am Sa., 25. Nov. 2017, 00:05:

I'd argue that using WeakRefs is perfect in this case since the cache/pool is owned by the current Activity, so the refs are released on orientation change (or at least not referenced anymore). So for each orientation change/recreation of the activity, the pool gets reinstantiated properly. I've been profiling it for a while (messing around, changing orientation ~30 times), and the heap/allocs keep stable throughout! Maybe I misunderstood you, but there shouldn't be any leaks as long as the cache isn't persisted between layout changes/activity recreations?

The idea of using an AsyncLayoutInflater is spot on, I haven't seen that class around actually. It would be cool to "precache" some of the view hierarchies that are animated in later, because Android really doesn't mix inflation and animations well. Been looking for something like that for a while now!

fre 24 nov. 2017 kl 23:56 skrev Hannes Dorfmann < notifications@github.com

:

Using WeakReferences is a bad idea because just because a screen orientation happens doesn't mean that gc run and weak reference is released. With your implementation it is very likely that you have a memory leak. Example:

1. Activity starts in portrait. FooViewController is shown.
2. screen orientation change (gc doesn't run) --> FooViewController puts his View in the "cache" (I would rather call it pool than cache).
3. Activity is recreated in landscape . FooViewController uses View from cache, BUT the cached view has been instantiated in portrait and holds a reference to "PortraitActivity". Then you use this View within "LandscapeActivity" --> Memory leak: portrait activity can't be garbage collected because FooViewController uses cached View, so holds a reference to cached View and cached View itself holds a reference to "PortraitActivity".

So what I would do is something like that:

typealias LayoutRes = Int // like R.layout.footypealias Pool = Set

class MainAcrivity() : Activity() { val pools : Map<LayoutRed, Pool>() = ...

... override fun onDestroy (){ pools.clear() // release pooled views by removing strong references } }

And then in your controller:

class FooController : Controller {

override fun onCreateView(...) : View{ val cachedView = activity?.pools[R.layout.foo]?.firstOrNull() return if (cachedView != null) cachedView else inflater.inflate(R.layout.foo, ...) }

override fun onDestroyView () { activity?.pool[R.layout.foo].put(view) } }

Some thoughts: I would write my own data structure Pools rather than Map<LayoutRes, Pool> with a nice API (internally uses ArrayMap and some other optimized data structures). Also you could use an AsyncLayoutInflater (i.e. in Activity.onCreate()) and put it the view into the pool if you know beforehand that the user reaches very likely the FooViewController with your expensive View (expensive in terms of layout inflation takes long).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <

https://github.com/bluelinelabs/Conductor/issues/233#issuecomment-346904199

, or mute the thread <

https://github.com/notifications/unsubscribe-auth/AOhw0jsps94Q-9OvlnVT3W_oPLhc_3aAks5s50mRgaJpZM4MEOsE

.

--

paramsen.com | Pär Amsen | +46 765601973 <+46%2076%20560%2019%2073> | Sweden

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub < https://github.com/bluelinelabs/Conductor/issues/233#issuecomment-346907900 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AAjnrpMYo7fx_gwJyW-4LW1eLCR3hSIjks5s51m1gaJpZM4MEOsE

.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bluelinelabs/Conductor/issues/233#issuecomment-346915207, or mute the thread https://github.com/notifications/unsubscribe-auth/AOhw0uXq1AWbb9uxblChEGPyablfc7fkks5s54RLgaJpZM4MEOsE .

--

paramsen.com | Pär Amsen | +46 765601973 | Sweden

[sockeqwe]

Hm, so I don't know much about scoping in Kodien but if I understand it correctly, a new instance of val cache: MutableMap<Int, WeakReference<View>> is created on each screen orientation change and therefore WeakReference is senseless, isn't it?

Nevertheless, I would like to leave a WARNING here for all people that blindly copy your code:

If MutableMap<Int, WeakReference<View>> is a singleton or part of app wide scope or ViewController holds a strong reference to it and use always the same reference (i.e. reference is a field in your Viewcontroller class) in onCreateView() , you may have a memory leak and WeakReference is not preventing this. It may works 30 times on your test device, but there is no guarantee that gc runs on screen orientation change so that WeakReference.get() returns null. You have no control over gc nor you can make assumptions about it. gc may work different on dalvik compared to art and might change in the future.

Again, just a warning for fellow conductor users.