Skip to content

Dictionary: fix missing images#12877

Merged
Frenzie merged 1 commit into
koreader:masterfrom
TnS-hun:dictionary_images_fix
Dec 12, 2024
Merged

Dictionary: fix missing images#12877
Frenzie merged 1 commit into
koreader:masterfrom
TnS-hun:dictionary_images_fix

Conversation

@TnS-hun

@TnS-hun TnS-hun commented Dec 8, 2024

Copy link
Copy Markdown
Contributor

Pass the directory that contains the dictionary's ifo file to Mupdf.openDocumentFromText to allow MuPDF to display images referenced by HTML dictionaries.

Depends on: koreader/koreader-base#2002
Fixes: #12628


This change is Reviewable

@TnS-hun

TnS-hun commented Dec 8, 2024

Copy link
Copy Markdown
Contributor Author

I was not sure to which directory to use: the current working directory or the directory that contains the dictionary's ifo file. I went with the latter.

With the former all dictionaries need a custom lua file to rewrite the resource paths. Based on the sample in the bug report this was the original behavior.

With the latter dictionaries can simply use res/image.jpg to resolve the images, so it is easier for the users. The only downside of this method that it does not allow combining entries from different dictionaries, but because the dictionaries are always displayed separately, I don't think this is a problem.

What do you think?

@Frenzie

Frenzie commented Dec 8, 2024

Copy link
Copy Markdown
Member

I'm not sure I entirely follow what you're saying. Relative directories and absolute directories are no longer functional at all?

Comment thread frontend/ui/widget/dictquicklookup.lua Outdated
Comment thread frontend/ui/widget/dictquicklookup.lua Outdated
@TnS-hun

TnS-hun commented Dec 8, 2024

Copy link
Copy Markdown
Contributor Author

I'm not sure I entirely follow what you're saying. Relative directories and absolute directories are no longer functional at all?

Images with absolute paths won't resolve in dictionaries. After this change paths must be relative to the dictionary's directory (that contains the ifo file).

But why would be absolute paths in the dictionaries?

I can change the code to use the current directory as the base directory, to not break existing custom lua dictionary scripts. But I think paths that are relative to the dictionary's directory are more logical.

@Frenzie

Frenzie commented Dec 8, 2024

Copy link
Copy Markdown
Member

After this change paths must be relative to the dictionary's directory (that contains the ifo file).

Is that the correct behavior? Shouldn't it be relative to the dictionary's res folder?

But why would be absolute paths in the dictionaries?

This is HtmlBoxWidget we're talking about, dictionary is merely something using it. :-) But as per koreader/koreader-base#2002 (comment) the old behavior is available simply by not passing anything in html_resource_directory.

Nonetheless there's still a very simple answer to the question as written: to keep the gigantic res folder with gigabytes of images on the SD card rather than internal storage.

If you feel motivated you could add an optional html_resource_directory argument to the customization function, along the lines of local ok, fixed_definition, fixed_html_resource_directory = pcall(ifo.fix_html_func, result.definition, dict_path). But then again the user simply updating to ../../../../etc will also do the trick.

@TnS-hun

TnS-hun commented Dec 8, 2024

Copy link
Copy Markdown
Contributor Author

Is that the correct behavior? Shouldn't it be relative to the dictionary's res folder?

You are right, the res is the correct base directory.

This is HtmlBoxWidget we're talking about, dictionary is merely something using it. :-) But as per koreader/koreader-base#2002 (comment) the old behavior is available simply by not passing anything in html_resource_directory.

Unfortunately, no. The fz_read_archive_entry function will not resolve any images if there is no archive handler specified.

I think MuPDF supports custom archive handlers , so we could make one that handles absolute paths if that is needed.

@Frenzie

Frenzie commented Dec 8, 2024

Copy link
Copy Markdown
Member

You can turn any absolute path into a relative path so it's probably not the biggest deal, but I would expect to be able to use either one at will from a standalone HTML file or string/stream, just like how it worked previously. In any case it's not important here unless you want it to be. ^_^

@TnS-hun TnS-hun force-pushed the dictionary_images_fix branch from de60a42 to 575a860 Compare December 9, 2024 19:44
@TnS-hun

TnS-hun commented Dec 9, 2024

Copy link
Copy Markdown
Contributor Author

I made the requested changes. Paths are now relative to the res directory (Nice catch! I did not know that the res directory was a standard in StarDicts.), and ReaderDictionary stores the res directory's path instead of the ifo file's path (It is indeed clearer this way. Thanks benoit-pierre!).

Pass the dictionary's res directory to Mupdf.openDocumentFromText to allow MuPDF to display images referenced by HTML dictionaries.

Depends on: koreader/koreader-base#2002
Fixes: koreader#12628
@TnS-hun TnS-hun force-pushed the dictionary_images_fix branch from 575a860 to 1e4198d Compare December 9, 2024 19:58
@Frenzie Frenzie added this to the 2025.01 milestone Dec 9, 2024
@Frenzie Frenzie mentioned this pull request Dec 11, 2024
Frenzie added a commit that referenced this pull request Dec 11, 2024
Includes:

* sqlite: update to 3.47.2 (koreader/koreader-base#1999)
* mupdf: fix finalizers (koreader/koreader-base#2001)
* mupdf: allow memory streams to load resources (koreader/koreader-base#2002) for #12877
* thirdparty/lua-Spore 0.3.4 (koreader/koreader-base#2003)
@Frenzie Frenzie merged commit f232b0e into koreader:master Dec 12, 2024
0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026
Includes:

* sqlite: update to 3.47.2 (koreader/koreader-base#1999)
* mupdf: fix finalizers (koreader/koreader-base#2001)
* mupdf: allow memory streams to load resources (koreader/koreader-base#2002) for koreader#12877
* thirdparty/lua-Spore 0.3.4 (koreader/koreader-base#2003)
0xstillb pushed a commit to 0xstillb/koreader-thai that referenced this pull request May 9, 2026
Pass the dictionary's res directory to Mupdf.openDocumentFromText to allow MuPDF to display images referenced by HTML dictionaries.

Depends on: koreader/koreader-base#2002
Fixes: koreader#12628
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.

Dictionary Lookup - images are not rendered

4 participants