Add Glide.setModulesEnabled API to allow apps to disable Manifest parsing #1753
Conversation
There was a problem hiding this comment.
The testing approach is indirect, but I think it's OK. Well documented!
I think you made the PR against the wrong branch though. You said you don't want to wait till v4, but master is v4. You want the 3.0 branch. (That said we may keep this as well just to have feature parity between v3 and v4, @sjudd?)
| @@ -547,6 +552,44 @@ public void testClone() throws IOException { | |||
| verify(secondTarget).onResourceReady(notNull(Drawable.class), isA(Transition.class)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testSetModulesEnabledTrue() throws Exception { | |||
| // teaDown glide instance first so we have a clean slate and not the Glide.get() call in setUp | |||
|
|
||
| @Test | ||
| public void testSetModulesEnabledFalse() throws Exception { | ||
| // teaDown glide instance first so we have a clean slate and not the Glide.get() call in setUp |
| assertEquals(1, modelLoaders.size()); | ||
| assertTrue(modelLoaders.get(0) instanceof HttpGlideUrlLoader); | ||
| } finally { | ||
| Glide.setModulesEnabled(true); |
There was a problem hiding this comment.
I think this should be part of Glide.tearDown, it resets Glide to pre-initialized state, which IMO includes both static variables. (It should probably even reset ViewTarget.tagId)
|
Oops, yes. I want this on the v3 branch. Want a separate PR or should we land on master and then cherry-pick? |
| @@ -132,6 +137,7 @@ public Boolean answer(InvocationOnMock invocation) throws Throwable { | |||
| @After | |||
| public void tearDown() { | |||
| Glide.tearDown(); | |||
| Glide.setModulesEnabled(true); | |||
There was a problem hiding this comment.
I meant Glide.tearDown not the @After, though I guess it doesn't matter much.
There was a problem hiding this comment.
It's fine to submit this for just v3 or for v3 and v4. The changes will probably make cherry picking a bit complicated, but you can try it.
I'm completely fine with this approach, but I'd be curious to get your thoughts on #1742. That would remove the need for manifest parsing entirely.
| * Must be called before accessing the Glide singleton; otherwise, has no effect. | ||
| */ | ||
| public static void setModulesEnabled(boolean enabled) { | ||
| synchronized (Glide.class) { |
There was a problem hiding this comment.
I'd probably add a check here to make sure that the Glide singleton hasn't already been instantiated.
|
OK I just made #1754 against the 3.0 branch. Turns out a lot has changed around the test code, so that part is pretty different. RE #1742. as I read it, it still parses the manifest every time, so it wouldn't fix #684 even if we were able to ship Glide 4.0 right now. Maybe 4.0 needs its own implementation of this same method? |
|
Yup that's true, but we'd probably enable a parameter similar to the one @TWiStErRob proposed for excluding modules to exclude parsing the manifest entirely. It's mostly still there to not instantly break everything. |
|
OK great. How about I drop this PR, and you add a parameter to your PR? |
|
Yep! I'll add a comment over there just to make sure this doesn't get lost. |
Description
Adding a static flag to Glide to disable parsing the manifest to find configured modules.
Added unit tests.
Fixes #684
Motivation and Context
The Dropbox app has been crashing on launch approximately 9000 times a day due to
RuntimeExceptions thrown byPackageManager. We'd like to remove the dependency onPackageManagerfrom our startup path, and we don't use anyGlideModules. While #1742 in 4.0 will address this long term, we'd like a short term way to run Glide without runtime manifest parsing.In order to enable unit testing, I decided to make this a set method that takes a
boolean.Not sure if I found the best way to unit test this. Feel free to propose a cleaner approach.