search

Gwindow / WhatAndroid

The What.CD Android App
http://gwindow.github.com/WhatAndroid/
BSD 2-Clause "Simplified" License
100 stars 20 forks source link

Switch to Glide #42

Closed fatih-atasever closed 9 years ago

fatih-atasever commented 9 years ago

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.

Twinklebear commented 9 years ago

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 onCreate so maybe it is created somewhere? Where does it fit in?

fatih-atasever commented 9 years ago

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.

Twinklebear commented 9 years ago

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?

fatih-atasever commented 9 years ago

Yes, our exception handler catches it and shows the appropriate error image/placeholder.

stuxo commented 9 years ago

Should this PR be targeting develop rather than master?

Twinklebear commented 9 years ago

@stuxo You're right, good catch. @hfatih Could you change this pull to be targeting the develop branch?

fatih-atasever commented 9 years ago

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.

Twinklebear commented 9 years ago

@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.

fatih-atasever commented 9 years ago

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.