-
Notifications
You must be signed in to change notification settings - Fork 257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[wip] Image caching #343
[wip] Image caching #343
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work !
Just one remark : why are the image paths in html content replaced when the article is shown instead of when the content is fetched and the images saved ? I fear for latency when opening an article with multiple pics.
@@ -7,7 +7,7 @@ android { | |||
|
|||
defaultConfig { | |||
applicationId "fr.gaulupeau.apps.InThePoche" | |||
minSdkVersion 11 | |||
minSdkVersion 19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ? WRITE_EXTERNAL_STORAGE exists before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so that we can use getExternalFilesDirs() to determine the real SD card and not only internal emulated external storage: https://developer.android.com/reference/android/content/Context.html#getExternalFilesDirs(java.lang.String)
if you have an other way to determine the REAL sd card path, i would be happy to see a commit :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was to use getExternalFilesDirs() if we're on SDK >= 19 and default to internal emulated external storage in the other case.
According to Google Play, we have something like 5% of users below KitKat (and the F-Droid numbers are certainly higher), so I'll prefer to have an special case just for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i understand that. But Kitkat is fairly old. Such user can still use the old app version, can't they?
I would really want to cache the images on my SD card, because they are consuming about 400 MB at my app. If we find a solution without getExternalFilesDirs to store on the real SD, let's just use that. getExternalFilesDir() (singular, without the s) does provide the /storage/emulated/ folder, which is the storage in the mobile and not the SD. That's really stupid behaviour by Android.
To keep the original article from wallabag. What happens if the user switches the setting back to not use cached images anymore? What happens if the user deletes the pictures on the SD card? Any user or app can do that. Then we have no way to retrieve the original pictures anymore.
I think this is not a very costly operation. What do you mean by multiple pics? 100? 1000? So far i saw a performance improvement when i compare pure offline mode against online mode. Just my 2 cents. I am open to any more concrete suggestions. There are for sure more open points to think about a lot. |
Good points. I'll see for myself about performance issues. |
Done. Not sure about what BTW, we can also add an option to move article DB to external storage. |
Yes, we could. But i would not use that option. That is not important to me, because i like the DB to be on the fast storage. SD Card can be slow (if you do not have class 10 or faster) and so the app might be slow aswell. Additionally, the DB is not that big, that i would need that on the SD Card to save internal space. Another reason to not have the DB on SD is the case, where the user ejects SD card. In that case the app is not usable anymore. That case is cared for in this PR w.r.t. the images. If the image cannot be found on SD card, it is going to be loaded via web. |
#347 |
Also, the DB scheme change is there just to make the prototype work. I'm not yet sure how to store image cache info better. |
I had the idea to create a separate DB table named After RSS feed sync the image fetching process should be triggered and the ImageFetch process works on the table. After successful or (in the first shot) failed download the row can be deleted from that table. |
Please rebase. |
…s if cached version of image has been found
…c util class ImageCacheUtils
…() to determine the real SD card and not only internal emulated external storage
… every article item being parsed
…t article id usage
…ge being displayed. especially important if image URL contains special chars like e.g. umlauts (äöüß)
Rebased and force-pushed (to keep the PR). Also kept the old branch just in case https://github.com/wallabag/android-app/tree/image-caching-bak. |
I just got the following exception on current HEAD of this branch:
|
This IllegalStateException is reproducible even on a fresh new installation, where i manually deleted the cache image folder on the sd card manually to have a clean state. It crashes with exact the same exception and after the same finished image being successfully download:
BTW, this article 2545 is the last in the LazyList. |
Maybe we experience a bug in org.greenrobot.greendao.query.LazyList here. I just tried using normal list() with a List<Article> and this works as expected without any exception. see attached patch 0001-use-List-instead-of-LazyList-when-querying-for-artic.patch.txt |
just also saw some timeouts being catched, where i am going to add some more information:
|
81f9745
to
2e02cd8
Compare
I don't get the exception. Did you change anything else (dependencies' versions, etc.)? |
No, only gradle, the PR which just got merged. |
I completely wiped my app and the content on the SD card and build a clean debug version of this branch here. Unfortunately i can still reproduce the crash with the LazyList. So i installed the app, configured it with my server, and the initial sync was done automatically. Then i changed the setting to enable image caching and pushed the button to sync. After a while (i have approx. 2000 articles in wallabag), i get the crash of the app. Even after restarting the app, it starts automatically to sync again and of crashes again. Restarting it for the 2nd time it still starts to sync and crashes. And after the third time, it just starts the app without automatically syncing. If it push the sync button again, i can reproduce the behaviour and the loop of crashes repeats again. The only change i have made to this branch is:
Can we please apply my patch and get this PR merged? |
Open points before merging from @di72nn:
|
due to having #394 we can close this PR? |
The PR may be obsolete, but I restored the |
okay, i just implemented a stupid image downloader which has no queue and separate no thread yet.
This is a TODO. I just want to save work here and maybe get some feedback or (even better would be) support from somebody (looking at @di72nn or @tcitworld 😉 )
I am not working on this branch again before sunday evening or even monday, if somebody wants to contribute some code.