Skip to content

processImg: skip data URI#13034

Merged
Frenzie merged 2 commits into
koreader:masterfrom
Frenzie:data-uri
Jan 9, 2025
Merged

processImg: skip data URI#13034
Frenzie merged 2 commits into
koreader:masterfrom
Frenzie:data-uri

Conversation

@Frenzie

@Frenzie Frenzie commented Jan 9, 2025

Copy link
Copy Markdown
Member

This is the underlying reason that The Verge + download full article fails, see #12953 (comment).


This change is Reviewable

This is the underlying reason that The Verge + download full article fails, see <koreader#12953 (comment)>.
@Frenzie Frenzie added this to the 2025.01 milestone Jan 9, 2025
@Frenzie Frenzie requested a review from poire-z January 9, 2025 20:30
Comment thread frontend/ui/wikipedia.lua
Comment on lines +701 to +703
if src:sub(1,5) == "data:" then
logger.dbg("NewsDownloader: skipping data URI")
return nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please donc infect wikipedia.lua with references to NewsDownloaded :)
What happens when we meet a data: ? It just let it as is and it works ? How did it behave previously ?
html = html:gsub("(<%s*img [^>]*>)", processImg) just let it as-is when it returns nil ? Or it removes it completely ?
Just asking, this feels right if it lets it untouched.

@Frenzie Frenzie Jan 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is to leave it untouched (as opposed to converting it, let alone trying to download it — which will fail just as hard on Wikipedia). But you're right, it should probably be img_tag for that purpose and not nil.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, nil is correct. That's unchanged. I thought I had tested it and I had but I wasn't sure anymore. :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good.
So just update logger.dbg("NewsDownloader:... to how any other logger.dbg in wikipedia.lua looks :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh now I see what you were getting at. I'm not sure how that got in there, haha

@Frenzie

Frenzie commented Jan 9, 2025

Copy link
Copy Markdown
Member Author

Incidentally, something like this might not be a bad idea either. But perhaps it's better to stick a pcall around the actual download instead.

diff --git a/plugins/newsdownloader.koplugin/epubdownloadbackend.lua b/plugins/newsdownloader.koplugin/epubdownloadbackend.lua
index 68e6dda44..cfeadcd97 100644
--- a/plugins/newsdownloader.koplugin/epubdownloadbackend.lua
+++ b/plugins/newsdownloader.koplugin/epubdownloadbackend.lua
@@ -381,6 +381,11 @@ function EpubDownloadBackend:createEpub(epub_path, html, url, include_images, me
                     end
                 end
             end
+            local parsed_url = socket_url.parse(src)
+            if not (parsed_url and parsed_url.scheme and parsed_url.host) then
+                logger.dbg("skipping unparsable URL", src)
+                return nil
+            end
             cur_image = {
                 imgid = imgid,
                 imgpath = imgpath,

@Frenzie Frenzie merged commit 9b444cb into koreader:master Jan 9, 2025
@Frenzie Frenzie deleted the data-uri branch January 9, 2025 21:41
0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026
This is the underlying reason that The Verge + download full article fails, see <koreader#12953 (comment)>.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants