Fixes #67 (Gif duplication)

[charlesdurham]

It's not a bullet proof solution but it would work as a stand-in until bumptech/glide#2526 is fixed.

[charlesdurham]

#67

[ZacSweers]

It looks like it's fixed in Glide master now. Would it be better to just wait for the next release?

[charlesdurham]

I think the NonAutostarting targets or some simpler variant where the DribblerTarget doesn't call super onResourceReady might still be worthwhile to avoid immediately starting and stopping the gif.

[charlesdurham]

Immediately starting and stopping each gif adds the overhead of decoding and holding in memory a second bitmap.

[ZacSweers]

Fair point. Looks like 4.3 was released, do you want to include that update in this PR?

[charlesdurham]

Done, I saw bumptech/glide#2541 but wasn't able to reproduce.

[ZacSweers]

Awesome! Installing now, going to run for a day or so tomorrow to see if anything comes up

[ZacSweers]

So functionally everything seems fine, but one thing I wanted to check. I've noticed I get leak canary reports now originating from Glide. It does seem to be rooted in a custom request manager I wrote, but wanted to see if this looks related to your change:

[charlesdurham]

It looks like a change in Glide's RequsetManger between 4.2. and 4.3. In 4.3 RequestManger holds onto any context reference in it's constructor this.context = context.

I haven't quiet tracked down why/how the RequestManger is being kept around but either way I don't see any reason for that context reference not to be context.getBaseContext().

EDIT:

Thinking this may be using the wrong lifecycle callback, not super familiar with Conductor but I think the RequestManager needs to be destroyed and the lifecycle listener removed when the activity is destroyed and not when the controller is destroyed.

inline fun Controller.requestManager(): RequestManager {
  return activity?.let {
    Glide.with(activity).apply {
      addLifecycleListener(object : LifecycleListener() {
        override fun postCreateView(controller: Controller, view: View) {
          onStart()
        }

        override fun postDestroyView(controller: Controller) {
          onStop()
        }

        override fun preDestroy(controller: Controller) {
          try {
            onDestroy()
          } catch (e: IllegalStateException) {
            // silently ignore this, glide aggressively crashes if not fully registered yet
          }
          controller.removeLifecycleListener(this)
        }
      })
    }
  } ?: throw IllegalArgumentException(
      "You cannot start a load until the Controller has been bound to a Context.")
}

[ZacSweers]

Interesting. Want to make both of those changes in this PR?

[charlesdurham]

I did some more investigation and glide ties a RequestManager's start/stop/destroy to an internal fragment. You can see most of this in RequestManagerRetriever and RequestManagerFragment.

The simplest solution is to let glide do it's thing and not try to tie the request manager lifecycle to a controller. I think that's also the best solution here since it doesn't seem like that much is gained by tying the request manager's lifecycle to a controller.

That said; I also created a Glide.with() that ties a request manager to a controller's lifecycle (because why not): https://gist.github.com/charlesdurham/7ab78fb97038ec7d6fc7a3d2ebec23fb

I didn't add that gist to my PR but let me know if you want to go that route instead.

[ZacSweers]

Yep I'd agree with just removing it and letting Glide handling it. Can revisit your gist in the future if we decide there needs to be a reason to tie to the controller lifecycle. Want to just delete the requestmanager impl as part of this PR?

[charlesdurham]

Not sure what you mean by request manager impl, I didn't add that gist to the PR, was really just me messing around.

[ZacSweers]

As in remove my existing custom implementation too and just use vanilla glide

[charlesdurham]

Ah sorry, took me a bit to wrap my head around what's going on.

[charlesdurham]

I think all those sub-classes get generated from @GlideModule. I was hoping there would be some way to hook into glide's initialization and set the default request options without all of those classes being generated but I can't find one.

There's also a generated GlideApp which is already being used in the app and supports adding generated methods (right now it just mimics Glide's static APIs). Seems slightly better to use GlideApp.with() instead of Glide.with() so I made that minor change in my PR.

I feel like there's still a chance I totally missed what you were after so if that's the case sorry again.

[ZacSweers]

Ah I was specifically talking about removing this function while you're at it since I think it's unused now - https://github.com/hzsweers/CatchUp/blob/8861c3bfb96db6b0a6c80480803ece4caabce50e/app/src/main/kotlin/io/sweers/catchup/GlideConfiguration.kt#L60-L60

It looks like you've done that though, so I think this is good to go!

[ZacSweers]

Thanks again for this and bearing with me on code review cycles (been traveling a lot with 2 conferences in the last week, and one more this weekend x_x)

[ZacSweers]

I bet @nickbutcher would be interested in this fix in Plaid too if you're feeling contribute-y :)