gingo/LazyList — comment on commit e9e2c8310f49768d78aad21bb6a29b831d4ef2f4
File:  line 0
By gingo:

Hello,

my comments are inline.

Dne 19.1.2012 17:56, Fedor Vlasov napsal(a):
> Hello Vladislav!
>
> Fantastic modifications, I like the way you do it! Things that I always wanted but never could implement myself.
>
> Unfortunately I can't merge it because you put them all as a single commit. I want to have a clear history so I prefer small commits. Small commits would be easy to understand and evaluate but one big commit is really hard :-) It affects almost every single piece of code.
>
> Here are some comments from me:
>
> 1. You say it may crash when used in fragments. I know that's true because some people reported me about that. But I was never able to reproduce it myself. Can you advice me a clear steps to reproduce the issue?
> The way you fix it - using handler - perfectly OK. But. One handler instance per ImageLoader. I usually reuse one ImageLoader instance in all activities in application to get better cache usage. Different activities use different UI threads so one handler for all may not work. What do you think? I expected to have one handler in each PhotoToLoad task object. Not sure about that. That's why I wanted to reproduce the issue before making any fixes.
The problem is somewhere in this lines:

BitmapDisplayer bd=new BitmapDisplayer(bmp, photoToLoad);
Activity a=(Activity)photoToLoad.imageView.getContext();
a.runOnUiThread(bd);

I don't know why, but the BitmapDisplayer never starts up when used with
fragments. Maybe it is related only to android-support-v4.jar
compatibility pack.

One handler per PhotoToLoad is much more better then my solution, but I
don't know how expensive is to create new Handler.

>
> 2. You use imageView.setTag. I used it before but then replaced with imageViews map. Because people may want to use setTag() in their own code and that will break ImageLoader logic.
Yes, you can discard this change, this was only for testing purposes ;)
>
> 3. You use lastModified to clean cache. Very good but unfortunately I'm not sure I understand your logic correctly. I expect lastModified to be updated when I read file from cache. This way I can identify files that were not accessed for long time. But in your implementation you update lastModified in getBitmapFromWeb. Are you sure about that?
In my opinion is too expensive to update lastModification date on
sdcard/flash every time you read the file. Method getBitmapFromWeb is
called when server is checked for newer version of the file and when we
are looking for newer version of the file, the file was accessed. This
suppose that timeInMilisToCheckChanges is much smaller than
timeInMilisToCleanUnusedFiles.
>
> 4. You update lastModified in getBitmapFromWeb. But getBitmapFromWeb is only called when we failed to get image from cache so there's no file. Makes no sense updating lastModified. I guess I just misunderstood your logic.
> Anyway I would be happy to implement something like that. Are you sure it's safe. Is "touch" always fast operation or it can be locked for some time if file system is locked by other operations?
I'm not sure about "touch" performance, this is why I place it only in
getBitmapFromWeb method.
>
> 5. In my implementation images are never modified that's why I never refresh images I already have in cache. It's a good thing to do if you need it but I don't want this code in my repository because I want my sample to be as clear as possible so that everyone can understand it and reuse it. Some specific things may be outside of public repo.
Ok, then just modify the code to update lastModified time every
timeInMilisToCleanUnusedFiles/10 ms ;-)
> Anyway very inspiring commit. Thank you! Don't you mind if I reuse some of your ideas?
In fact it is compilation from other opensource projects, so feel free
to reuse it!
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/gingo/LazyList/commit/e9e2c8310f49768d78aad21bb6a29b831d4ef2f4#commitcomment-882752

--
Vladislav Skoumal

+420606722715
skoumal@skoumal.net