Fix wallabag's "Prefer original non-HTML document" setting#13333
Conversation
Frenzie
left a comment
There was a problem hiding this comment.
lgtm, let me know when you're done testing.
| if mimetype == "text/html" then | ||
| logger.dbg("Wallabag:downloadArticle: not ignoring EPUB, because", article.url, "is HTML") | ||
| elseif mimetype == nil then -- base ourselves on the file extension | ||
| if util.getFileNameSuffix(article.url):lower() == "html" then |
There was a problem hiding this comment.
.htm is or used to be common as well, if this is sufficiently much of a concern.
There was a problem hiding this comment.
Oh, right, forgot about that one
|
@Frenzie I'm done testing. Could you take a look? |
| -- A function represents `null` in our JSON.decode, because `nil` would just disappear. | ||
| -- We can simplify that to not a string. | ||
| local mimetype = type(article.mimetype) == string and util.trim(article.mimetype:match("^[^;]*")) or nil | ||
| -- The mimetype is actually an HTTP Content-Type, so we remove everything from the `;` onward |
There was a problem hiding this comment.
The original comment is better. It explains the why, while this just repeats the code below.
| -- We can simplify that to not a string. | ||
| local mimetype = type(article.mimetype) == string and util.trim(article.mimetype:match("^[^;]*")) or nil | ||
| -- The mimetype is actually an HTTP Content-Type, so we remove everything from the `;` onward | ||
| -- Wallabag returns `null` if no mimetype is known, which is JSON.decoded to a function, |
There was a problem hiding this comment.
The old comment here is also mich clearer. "which is JSON decoded to a function"? Why, what does that even mean? The original comment is clear.
| -- All webpages are HTML. Ignore them since we want the Wallabag EPUB instead! | ||
| if self.download_original_document then | ||
| if mimetype ~= "text/html" and DocumentRegistry:hasProvider(nil, mimetype) then | ||
| -- Download any filetype that we have a provider for, other than HTML, from the original URL |
There was a problem hiding this comment.
This too seems to be rewritten to paraphrase rather than explain the code.
| logger.dbg("Wallabag:downloadArticle: ignoring EPUB in favor of original", article.url) | ||
| file_ext = "." .. util.getFileNameSuffix(article.url) | ||
| item_url = article.url | ||
| -- Fix duplicate extensions (e.g. `.txt.txt`) |
There was a problem hiding this comment.
What does this mean? When or why might this happen?
|
I'm not sure if it matters much here, but having a provider doesn't necessarily mean a format is very well supported. EPUB conversion by Wallabag could theoretically be preferable in some cases. |
harmtemolder
left a comment
There was a problem hiding this comment.
Interesting thought. It shouldn't be too hard to add more exceptions in the future. Maybe leave it for now, but add it if someone encounters such a case and files an issue?
Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @Frenzie)
| -- A function represents `null` in our JSON.decode, because `nil` would just disappear. | ||
| -- We can simplify that to not a string. | ||
| local mimetype = type(article.mimetype) == string and util.trim(article.mimetype:match("^[^;]*")) or nil | ||
| -- The mimetype is actually an HTTP Content-Type, so we remove everything from the `;` onward |
| -- We can simplify that to not a string. | ||
| local mimetype = type(article.mimetype) == string and util.trim(article.mimetype:match("^[^;]*")) or nil | ||
| -- The mimetype is actually an HTTP Content-Type, so we remove everything from the `;` onward | ||
| -- Wallabag returns `null` if no mimetype is known, which is JSON.decoded to a function, |
| -- All webpages are HTML. Ignore them since we want the Wallabag EPUB instead! | ||
| if self.download_original_document then | ||
| if mimetype ~= "text/html" and DocumentRegistry:hasProvider(nil, mimetype) then | ||
| -- Download any filetype that we have a provider for, other than HTML, from the original URL |
| logger.dbg("Wallabag:downloadArticle: ignoring EPUB in favor of original", article.url) | ||
| file_ext = "." .. util.getFileNameSuffix(article.url) | ||
| item_url = article.url | ||
| -- Fix duplicate extensions (e.g. `.txt.txt`) |
I assume Wallabag doesn't convert Mobi or some such anyway, but if it did there's a chance it'd do a better job. |
|
Anyway, thanks for testing! |
Fixes #13279: the wallabag plugin now correctly ignores URLs ending in
.htmlwhen no mimetype is known and downloads the.epubinstead. Also fixes duplicate file extensions for files where no mimetype is known and the title is based on the URL (e.g..txtfiles). Also felt the comments explaining the behavior could be improved upon.Setting it to draft, because I made a minor tweak today, and I'm still testing that.
This change is