Switch to Glide #42
Switch to Glide #42
Conversation
| /** | ||
| * Initialize MySoup so that we can start making API requests | ||
| */ | ||
| public static void initSoup(){ |
There was a problem hiding this comment.
Why was this removed? The way the WhatAPI works is that we need to set some static information like the site name, if we're using SSL and our user agent. Without this we won't be able to make API requests to the site. Related to this the site string shouldn't have been removed either.
I haven't had time to fully go through the PR and try testing it but this stuck out.
There was a problem hiding this comment.
Ah I see this moved up to WhatApplication
|
Still working through the Pull Request, but why did you split parts of LoggedInActivity into WhatApplication? It seems like it's just a container for some static methods so why would it inherit from Application if we won't actually instantiate it (do we? I'm still reading). We override |
|
A normal class with some static methods/variables is not guaranteed to live through application lifecycle. It can be killed, variables may get removed etc. Application class is guaranteed to live. By adding it into the manifest as the name of the application, it is guaranteed that WhatApplication class initializes first, before any acticity or class, and it is killed last. For these properties, i always thought doing app-wide things and initializations in Application class is beneficial, or convenient at least. And had no problems in the past with it. |
|
Hmm, I see. I also noticed you took out checking for null or empty image URLs which we do sometimes get on the site. Will Glide just throw an exception which is picked up by the exception handler given to it? |
| sharedConstructing(context); | ||
| } | ||
|
|
||
| private void sharedConstructing(Context context) { |
There was a problem hiding this comment.
For readability I'd prefer it if there wasn't a newline after every statement here but that things were grouped logically instead.
There was a problem hiding this comment.
Sure, i will refactor it.
|
Yes, our exception handler catches it and shows the appropriate error image/placeholder. |
|
Should this PR be targeting develop rather than master? |
|
Oh, my bad. I didn't even use development branch to begin with. I will reapply these changes to development branch and open a new pull request if we are okay with the changes. |
|
@Hfatih do you think you may have time to finish up this PR? We're hoping to get a release out soon with the material design update and some bug fixes and this would be awesome PR to have in as well. If you don't think you'd have time I can finish making the final tweaks and testing to merge this in if it's in an ok state. |
|
Sure, i already applied little changes mentioned above and tested everywhere image loading used. Gifs work everywhere too. I will make another pull request in dev branch tonight. |
Migrated to glide for better performance and cache management, and built in gif support to be able to use gifs on any ImageView. Also made ImageDialog full screen, with gif support and panning/zooming.
I used application class for centralized image loading, been using this approach with no issues, if there is any caveat please let me know.