fix(covers): catch covers API timeouts and hardcode cache file extensions #123
Conversation
Fixes #107 ("Cover loading times out very often")
Other than grabbing the Content-Type header (which is cumbersome in the current scope), it is better for now to hardcode the file extension. Most file viewers today can recognize wrong extension and still read, for example, a png file disguised with a .jpg extension; At least now the cover cache files aren't a bunch of extension-less files anymore.
|
@xerus2000 same for this PR; needs a review |
| @@ -16,13 +16,16 @@ object Covers { | |||
| private fun coverCacheFile(coverUrl: String, size: Int): File { | |||
| coverCacheDir.mkdirs() | |||
| val newFile = coverCacheDir.resolve(coverUrl.substringBeforeLast('/').substringAfterLast('/').replaceIllegalFileChars()) | |||
There was a problem hiding this comment.
If the name of the coverUrl has changed, can't we simplify this? Otherwise, using split seems more sensible to me than two substring operations.
There was a problem hiding this comment.
Also, please write a test for this method. Should be easy.
There was a problem hiding this comment.
I have not a single idea on how to write tests unfortunately.
There was a problem hiding this comment.
Then look into the existing ones :)
| fun getThumbnailImage(coverUrl: String, size: Number = 64, invalidate: Boolean = false) = try { | ||
| getCover(coverUrl, 64, invalidate).use { createImage(it, size) } | ||
| } catch (e: Exception) { | ||
| null | ||
| } |
There was a problem hiding this comment.
Why swallow the exception here?
And if you have to, please document that.
There was a problem hiding this comment.
Indeed, there should be an error logging.
There was a problem hiding this comment.
Why does this function have to catch the error at all, why not propagate it?
And you should almost never catch a blanket-Exception - this might swallow critical unexpected errors too.
There was a problem hiding this comment.
Better handling of exceptions with 81f8555
| @@ -16,13 +16,16 @@ object Covers { | |||
| private fun coverCacheFile(coverUrl: String, size: Int): File { | |||
| coverCacheDir.mkdirs() | |||
| val newFile = coverCacheDir.resolve(coverUrl.substringBeforeLast('/').substringAfterLast('/').replaceIllegalFileChars()) | |||
| return coverCacheDir.resolve("${newFile.nameWithoutExtension}x$size.${newFile.extension}") | |||
| return coverCacheDir.resolve("${newFile.nameWithoutExtension}x$size.jpg") // FIXME : Do not hardcode extension | |||
There was a problem hiding this comment.
Why don't you use the extension of newFile anymore? If we don't know the extension, I would prefer to give the file no extension at all. If a reader is so versatile that it can recognize a png disguised as jpg, it can also handle files without extension. Besides, the cache is not for browsing anyways.
There was a problem hiding this comment.
The extensions returned empty each time, because coverUrl (the HTTP url to the cover) doesn't have an extension anymore. Only some gibberish by AWS...
Let's remove the hardcode then and keep files without extensions.
Co-authored-by: Janek <[email protected]>
Fixes #107
Regarding the file extension hardcode :
Other than grabbing the Content-Type header (which is cumbersome in the current scope), it is better for now to hardcode the file extension. Most file viewers today can recognize wrong extension and still read, for example, a png file disguised with a .jpg extension;