Write highlights into PDF: revisited by hius07 · Pull Request #12509 · koreader/koreader

hius07 · 2024-09-12T15:12:30Z

Closes #12500.

This change is

Frenzie · 2024-09-12T15:17:45Z

Lowecase on/off is normally only for the bottom menu btw. The screenshots look nice
👍

hius07 · 2024-09-12T15:54:54Z

hius07 · 2024-09-12T16:52:03Z

CircleCI warnings:

WARN could not load liblipclua: module 'liblipclua' not found:

ERROR Failed to initialize terminal plugin: plugins/terminal.koplugin/main.lua:108: wrong number of arguments to 'insert'

WARN Error when loading plugins/coverbrowser.koplugin/main.lua common/lua-ljsqlite3/init.lua:109: ';' expected near '='

ObjectCache(0x7f1a4fd36200)::~ObjectCache(): WARNING! LEAK! object 0x55f02bbea7f0 still has count 1 (id ./data/tessdata/eng.traineddatapunc-dawg)
ObjectCache(0x7f1a4fd36200)::~ObjectCache(): WARNING! LEAK! object 0x55f02bbea860 still has count 1 (id ./data/tessdata/eng.traineddataword-dawg)
ObjectCache(0x7f1a4fd36200)::~ObjectCache(): WARNING! LEAK! object 0x55f02bbea910 still has count 1 (id ./data/tessdata/eng.traineddatanumber-dawg)
ObjectCache(0x7f1a4fd36200)::~ObjectCache(): WARNING! LEAK! object 0x55f02c1a4650 still has count 1 (id ./data/tessdata/eng.traineddatabigram-dawg)
ObjectCache(0x7f1a4fd36200)::~ObjectCache(): WARNING! LEAK! object 0x55f02c1a4c90 still has count 1 (id ./data/tessdata/eng.traineddatafreq-dawg)
make: unlink: base/thirdparty/kpvcrlib/crengine/cr3gui/data/tessdata: Is a directory

hius07 · 2024-09-12T17:09:43Z

Interesting feature would be scanning a pdf file for existing embedded highlights and creating our corresponding annotations. Dunno if MuPDF can do it.
@benoit-pierre ?

benoit-pierre · 2024-09-12T17:15:00Z

CircleCI warnings:

WARN could not load liblipclua: module 'liblipclua' not found:

Expected.

ERROR Failed to initialize terminal plugin: plugins/terminal.koplugin/main.lua:108: wrong number of arguments to 'insert'

Not sure about that one (I've just nuked the plugin locally after getting tired of it preventing KOReader' startup on Android).

WARN Error when loading plugins/coverbrowser.koplugin/main.lua common/lua-ljsqlite3/init.lua:109: ';' expected near '='

Expected (lack of proper isolation when running tests).

ObjectCache(0x7f1a4fd36200)::~ObjectCache(): WARNING! LEAK! object 0x55f02bbea7f0 still has count 1 (id ./data/tessdata/eng.traineddatapunc-dawg)
ObjectCache(0x7f1a4fd36200)::~ObjectCache(): WARNING! LEAK! object 0x55f02bbea860 still has count 1 (id ./data/tessdata/eng.traineddataword-dawg)
ObjectCache(0x7f1a4fd36200)::~ObjectCache(): WARNING! LEAK! object 0x55f02bbea910 still has count 1 (id ./data/tessdata/eng.traineddatanumber-dawg)
ObjectCache(0x7f1a4fd36200)::~ObjectCache(): WARNING! LEAK! object 0x55f02c1a4650 still has count 1 (id ./data/tessdata/eng.traineddatabigram-dawg)
ObjectCache(0x7f1a4fd36200)::~ObjectCache(): WARNING! LEAK! object 0x55f02c1a4c90 still has count 1 (id ./data/tessdata/eng.traineddatafreq-dawg)
make: unlink: base/thirdparty/kpvcrlib/crengine/cr3gui/data/tessdata: Is a directory

I investigated that one, and there did not seem to be a real leak, might look into it again after #12497.

NiLuJe · 2024-09-12T17:15:33Z

WARN could not load liblipclua: module 'liblipclua' not found:

Normal, kindle codepath.

ERROR Failed to initialize terminal plugin: plugins/terminal.koplugin/main.lua:108: wrong number of arguments to 'insert'

Might actually be a bug, but I don't recall if there are still terminal PRs in flight about that ;).

WARN Error when loading plugins/coverbrowser.koplugin/main.lua common/lua-ljsqlite3/init.lua:109: ';' expected near '='

Also expected, ye olde' busted.

NiLuJe · 2024-09-12T17:15:43Z

Welp, I was too slow ;p.

benoit-pierre · 2024-09-12T17:20:37Z

Interesting feature would be scanning a pdf file for existing embedded highlights and creating our corresponding annotations. Dunno if MuPDF can do it. @benoit-pierre ?

Using those to iterate over a page's annotations (like we already do in getMarkupAnnotation):

/*
	Returns a borrowed reference to the first annotation on
	a page, or NULL if none.

	The caller should fz_keep this if it intends to hold the
	pointer. Unless it fz_keeps it, it must not fz_drop it.
*/
pdf_annot *pdf_first_annot(fz_context *ctx, pdf_page *page);

/*
	Returns a borrowed reference to the next annotation
	on a page, or NULL if none.

	The caller should fz_keep this if it intends to hold the
	pointer. Unless it fz_keeps it, it must not fz_drop it.
*/
pdf_annot *pdf_next_annot(fz_context *ctx, pdf_annot *annot);

Frenzie · 2024-09-12T17:34:18Z

I've just nuked the plugin locally after getting tired of it preventing KOReader' startup on Android

You can simply disable it btw. (Unless that's what you meant by "nuke")

hius07 · 2024-09-12T17:34:40Z

Using those to iterate over a page's annotations (like we already do in getMarkupAnnotation):

Thanks, looks like I cannot do it in frontend, new methods in mupdf.lua are required. Postponing so far.

benoit-pierre · 2024-09-12T17:37:32Z

I've just nuked the plugin locally after getting tired of it preventing KOReader' startup on Android

You can simply disable it btw.

That does not help on that first startup, which is when the shell detection fails.

(Unless that's what you meant by "nuke")

Nope. :P

Frenzie · 2024-09-12T17:38:18Z

Might actually be a bug, but I don't recall if there are still terminal PRs in flight about that ;).

#12384 remains in a bit of limbo atm.

Frenzie · 2024-09-12T17:40:47Z

That does not help on that first startup, which is when the shell detection fails.

Fair enough, but while it doesn't seem to be working quite right for me atm (and I think it used to?), it doesn't have any such effects.

NiLuJe

Looks good, just a few questions basically ;).

Reviewed 8 of 10 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @hius07)

frontend/apps/filemanager/filemanagermenu.lua line 948 at r4 (raw file):

    end

    UIManager:close(self.menu_container)

What's the rationale for moving this after the ConfirmBox?

frontend/apps/reader/modules/readerhighlight.lua line 560 at r4 (raw file):

            text_func = function()
                local text = self.highlight_write_into_pdf and _("on") or _("off")
                if (not self.highlight_write_into_pdf) == (not G_reader_settings:isTrue("highlight_write_into_pdf")) then

Is there a quirk I'm missing that wouldn't allow writing this as

if self.highlight_write_into_pdf == G_reader_settings:isTrue("highlight_write_into_pdf" then

instead?

frontend/apps/reader/modules/readerhighlight.lua line 661 at r4 (raw file):

                    callback = function()
                        UIManager:show(ConfirmBox:new{
                            text = _("Are you sure you want to delete all highlights from pdf file?"),

We might want to explain that this currently only applies to KOReader highlights.

frontend/apps/reader/modules/readerhighlight.lua line 2426 at r4 (raw file):

                self.highlight_write_into_pdf = config:isTrue("highlight_write_into_pdf") -- true or false
            else
                self.highlight_write_into_pdf = G_reader_settings:readSetting("highlight_write_into_pdf") -- true or nil

Ah. That could be the quirk I was looking for above ;).

Maybe use G_reader_settings:readSetting("highlight_write_into_pdf") or false to avoid the inconsistency?

frontend/apps/reader/modules/readerhighlight.lua line 2452 at r4 (raw file):

    self.ui.doc_settings:saveSetting("highlight_drawer", self.view.highlight.saved_drawer)
    self.ui.doc_settings:saveSetting("highlight_color", self.view.highlight.saved_color)
    self.ui.doc_settings:saveSetting("highlight_write_into_pdf", self.highlight_write_into_pdf)

Which would imply we always write this in doc_settings (which I'm fine with, mind you, just mentioning that in passing ;)).

frontend/apps/reader/modules/readermenu.lua line 378 at r4 (raw file):

    end

    self:onTapCloseMenu()

Same question as in FMMenu ;).

frontend/apps/reader/modules/readermenu.lua line 409 at r4 (raw file):

    if FileManager.instance then
        FileManager.instance:onClose()
    end

Closing FM before the nextTick was really really important, IIRC ;).

Code quote:

    local FileManager = require("apps/filemanager/filemanager")
    if FileManager.instance then
        FileManager.instance:onClose()
    end

frontend/ui/data/onetime_migration.lua line 711 at r4 (raw file):

end

-- 20240912, Write highlights to PDF: revisited

Missing the PR link in the comment ;).

frontend/ui/elements/common_settings_menu_table.lua line 569 at r4 (raw file):

local metadata_folder_help_text = table.concat(metadata_folder_help_table, "\n")

local hash_filemod_warn = T(_("%1 requires calculating partial file hashes of documents which may slow down file browser navigation. Any file modifications (such as embedding annotations into PDF files or downloading from calibre) may change the partial hash, thereby losing track of any highlights, bookmarks, and progress data. Embedding PDF annotations can be set at menu Typeset - Highlights - Write highlights into PDF."), metadata_folder_str.hash)

nit: The fancy arrows → were nicer ;).

Code quote:

local hash_filemod_warn = T(_("%1 requires calculating partial file hashes of documents which may slow down file browser navigation. Any file modifications (such as embedding annotations into PDF files or downloading from calibre) may change the partial hash, thereby losing track of any highlights, bookmarks, and progress data. Embedding PDF annotations can be set at menu Typeset - Highl

hius07

Reviewable status: 7 of 10 files reviewed, 8 unresolved discussions (waiting on @NiLuJe)

frontend/apps/filemanager/filemanagermenu.lua line 948 at r4 (raw file):