Open with... improvement #11056

hius07 · 2023-10-31T06:13:16Z

The purpose is to open non-document files by a tap in file browser and file search results.
Currently we have 4 auxiliary (non-document) providers: Image viewer, Text viewer, Text editor plugin, Archive viewer plugin.

Changes:
(1) DocumentRegistry
-a plugin can register itself as an auxiliary provider
-deduplicated code and run

(2) Open with... dialog
-associate a file type with an auxiliary provider
-view the list of providers associated with a file type
-reset the provider associated with a single file

Unchanged:
A single file cannot be associated with an auxiliary provider.
Document providers' standard behaviour.

(3) Archive viewer
-view images in an archive as a multi-page image (useful to view cbz content)

This change is

Frenzie · 2023-10-31T13:46:02Z

frontend/apps/filemanager/filemanagercollection.lua

@@ -161,7 +161,7 @@ function FileManagerCollection:showCollDialog()
                    path = G_reader_settings:readSetting("home_dir"),
                    select_directory = false,
                    file_filter = function(file)
-                        return DocumentRegistry:getProviders(file) ~= nil
+                        return DocumentRegistry:hasProvider(file)


Is the behavior the same for normally hidden files with a custom assigned engine and similar edge cases?

Please give an example what do you mean.
We add to favorites only documents (ie they have a provider).

I mainly meant I assumed it was a line I wrote a few years back, but I see now that it's in something you wrote. ;-)

Frenzie

lgtm in principle, not able to test atm though

hius07 · 2023-10-31T16:56:32Z

Aligned infomessage with type associates.

NiLuJe · 2023-10-31T18:14:09Z

You can't cache the providers registered from plugins; they reinstantiate on view changes, so you'd be pinning stale objects ;).

(IIRC, that's less of a problem for real Doc* providers, as we either feed it the class, or they're singletons, can't recall and a bit short on time right now).

hius07 · 2023-10-31T19:10:44Z

Thank you, I understand. I'll change the architecture.

poire-z

Not a thorough review (this provider business is a bit too much for me :)), but nothing jumps out. Trusting you.
Just a nitpick.

poire-z · 2023-11-01T11:27:13Z

frontend/document/documentregistry.lua

 local util = require("util")

 local DocumentRegistry = {
    registry = {},
    providers = {},
+    providers_hash = {}, -- hash table of registered providers { provider_key = provider }


providers_hash sounds like it would be a hash value (a string or a number).
It is a Lua hash (not an array) table, ok, but we don't advertize that fact in most of our names :)
So, may be known_providers or provider_names ?

hius07 · 2023-11-01T12:31:49Z

@pazos, regarding your #9615 and #11001, I do not follow your way:
-because such handlers as Text viewer or Text editor can be used for any file types,
-and to avoid far route via "showReader, then detect non-document, close Reader, reopen FM and finally handle the file".

I think your non-document handlers (plugins?) can be registered as auxiliary provider (see Text editor and Archive viewer here). Then any packet/non-document extension can be associated (manually in Open with... dialog or automatically by the plugin itself) with your aux provider. It will be opened just by a tap in the file browser and in the file search results.
No changes to the core would be required (except your icons in the coverbrowser, but that's another story).

pazos · 2023-11-01T17:37:31Z

@pazos, regarding your #9615 and #11001, I do not follow your way: -because such handlers as Text viewer or Text editor can be used for any file types, -and to avoid far route via "showReader, then detect non-document, close Reader, reopen FM and finally handle the file".

I think your non-document handlers (plugins?) can be registered as auxiliary provider (see Text editor and Archive viewer here). Then any packet/non-document extension can be associated (manually in Open with... dialog or automatically by the plugin itself) with your aux provider. It will be opened just by a tap in the file browser and in the file search results. No changes to the core would be required (except your icons in the coverbrowser, but that's another story).

Thanks for the hints. Feel free to do it your way. Mine was a quick attempt. Anyhow, I won't probably have time for that stuff until I'm retired :/

Will close those, while I'm at it :)

NiLuJe

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

frontend/apps/filemanager/filemanagerfilesearcher.lua line 322 at r4 (raw file):

                close_dialog_callback()
                local FileManager = require("apps/filemanager/filemanager")
                FileManager.openFile(self.ui, file, nil, self.close_callback)

Unfamiliar with all this, but I don't follow why close_callback is now limited to running when opening via auxiliary providers?

Code quote:

                close_dialog_callback()
                local FileManager = require("apps/filemanager/filemanager")
                FileManager.openFile(self.ui, file, nil, self.close_callback)

frontend/document/documentregistry.lua line 285 at r4 (raw file):

require("document/djvudocument"):register(DocumentRegistry)
require("document/picdocument"):register(DocumentRegistry)
-- auxuliary built-in

nit: auxiliary ;)

frontend/ui/widget/imageviewer.lua line 863 at r4 (raw file):

            return registry:isImageFile(file)
        end,
        callback = function(file)

You should be able to use a direct assignment here, e.g., callback = ImageViewer.openFile,, the signatures are identical, and the closure doesn't do anything fancy at all ;).

frontend/ui/widget/textviewer.lua line 544 at r4 (raw file):

        callback = function(file)
            TextViewer.openFile(file)
        end,

Ditto ;).

Code quote:

        callback = function(file)
            TextViewer.openFile(file)
        end,

NiLuJe · 2023-11-01T18:12:28Z

I also wouldn't mind clearer comments about the fact that none of these aux stuff actually implement the Document* API, and we're just shoe-horning those in DocumentRegistry, which requires more careful consideration in how you actually deal with them to do something useful (e.g., in FileManager.openFile)

hius07 · 2023-11-01T18:18:49Z

why close_callback is now limited to running when opening via auxiliary providers?

We are in the search results list.
I want to view a file (say, in the Text viewer), keep the list opened and then view the next found file, so on.
When I open a file in Reader, the list should be closed.

koreader/frontend/apps/filemanager/filemanager.lua

Line 1384 in 0b6cd11

    
           function FileManager:openFile(file, provider, doc_caller_callback, aux_caller_callback)

Actually, aux_caller_callback is not yet used anywhere, for the future.

hius07 · 2023-11-01T18:41:37Z

koreader/frontend/document/documentregistry.lua

Lines 49 to 55 in 4027288

    
           -- Register an auxiliary (non-document) provider. 
        
           -- Aux providers are modules (eg TextViewer) or plugins (eg TextEditor). 
        
           -- It does not implement the Document API. 
        
           -- For plugins the hash table value does not contain file handler, 
        
           -- but only a provider_key (provider.provider) to call the corresponding 
        
           -- plugin in FileManager:openFile(). 
        
           function DocumentRegistry:addAuxProvider(provider)

NiLuJe

Reviewed 1 of 1 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hius07)

poire-z · 2023-11-03T07:34:10Z

frontend/ui/widget/infomessage.lua

 function InfoMessage:init()
+    if not self.font_adjusted then
+        self.face = Font:getFace(self.monospace_font and "infont" or "infofont")


Or just if not self.face then self.face = ... ?
(Will allow passing a face= when we want something other of infont/infofont?)

Fix koreader#11390 Regression since koreader#11056

The previous check was inlined in the dir walk, so it always saw a relative path. Here, it gets an absolute one instead, so act accordingly ;). Fix koreader#11390 Regression since koreader#11056

The previous check was inlined in the dir walk, so it always saw a relative path. Here, it gets an absolute one instead, so act accordingly ;). Fix #11390 Regression since #11056

hius07 added 16 commits October 31, 2023 07:52

documentregistry: register aux providers

234a8ff

openwithdialog: support aux providers

4a94db3

filemanager: improve Open with... dialog

a5376be

imagewidget: local DocumentRegistry

f330928

imageviewer: register to DocumentRegistry

4515f2f

textviewer: register to DocumentRegistry

c243588

Archive viewer: register to DocumentRegistry, multi-page images

6ee625a

Text editor: register to DocumentRegistry

b48fb4f

filemanagerfilesearcher: open file with doc or aux providers

cdedf47

filemanagerutil: getRandomFile

73a6252

filechooser: remove Open with...

70ff270

screensaver: use getRandomFile

46214ea

readerlink: getProvider -> hasProvider

0c85d9c

filemanagercollection: getProviders -> hasProvider

2a2054e

documentregistry: remove unused lfs

6d3d3c4

Archive viewer: remove unused

a4a6b5b

Frenzie reviewed Oct 31, 2023

View reviewed changes

Frenzie approved these changes Oct 31, 2023

View reviewed changes

hius07 added 2 commits October 31, 2023 18:53

infomessage: add monospace font

f37b946

filemanager: accurate info message with associates

f054e5c

hius07 added 4 commits November 1, 2023 09:39

filemanager: use actual plugin instance to open file

d112c69

filemanagerfilesearcher: use actual plugin instance to open file

443cdaf

Archive viewer: do not cache plugin instance

083955e

Text editor: do not cache plugin instance

9ab28ba

poire-z reviewed Nov 1, 2023

View reviewed changes

documentregistry: providers_hash -> known_providers

0b6cd11

pazos mentioned this pull request Nov 1, 2023

[RFC]: File Manager documents #9615

Closed

NiLuJe requested changes Nov 1, 2023

View reviewed changes

hius07 added 2 commits November 1, 2023 20:34

documentregistry: comments

08e0af6

documentregistry: comment

4027288

hius07 added 2 commits November 1, 2023 21:03

imageviewer: simple callback

9c6b3ce

textviewer: simple callback

5820fbf

NiLuJe reviewed Nov 1, 2023

View reviewed changes

infomessage: fix init font face when font size adjusted

e75859d

poire-z reviewed Nov 3, 2023

View reviewed changes

infomessage: simple font face init

b3f9502

hius07 merged commit 68aa209 into koreader:master Nov 5, 2023
2 of 4 checks passed

hius07 deleted the open-with branch November 5, 2023 05:24

hius07 added this to the 2023.11 milestone Nov 5, 2023

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Jan 19, 2024

ScreenSaver: Properly exlude macOS resource forks again

3d072cc

Fix koreader#11390 Regression since koreader#11056

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open with... improvement #11056

Open with... improvement #11056

hius07 commented Oct 31, 2023 •

edited by Frenzie

Loading

Frenzie Oct 31, 2023

hius07 Oct 31, 2023

Frenzie Oct 31, 2023

Frenzie left a comment

hius07 commented Oct 31, 2023

NiLuJe commented Oct 31, 2023 •

edited

Loading

hius07 commented Oct 31, 2023

poire-z left a comment

poire-z Nov 1, 2023

hius07 commented Nov 1, 2023 •

edited

Loading

pazos commented Nov 1, 2023

NiLuJe left a comment

NiLuJe commented Nov 1, 2023

hius07 commented Nov 1, 2023 •

edited

Loading

hius07 commented Nov 1, 2023 •

edited

Loading

NiLuJe left a comment

poire-z Nov 3, 2023

Open with... improvement #11056

Open with... improvement #11056

Conversation

hius07 commented Oct 31, 2023 • edited by Frenzie Loading

Frenzie Oct 31, 2023

Choose a reason for hiding this comment

hius07 Oct 31, 2023

Choose a reason for hiding this comment

Frenzie Oct 31, 2023

Choose a reason for hiding this comment

Frenzie left a comment

Choose a reason for hiding this comment

hius07 commented Oct 31, 2023

NiLuJe commented Oct 31, 2023 • edited Loading

hius07 commented Oct 31, 2023

poire-z left a comment

Choose a reason for hiding this comment

poire-z Nov 1, 2023

Choose a reason for hiding this comment

hius07 commented Nov 1, 2023 • edited Loading

pazos commented Nov 1, 2023

NiLuJe left a comment

Choose a reason for hiding this comment

NiLuJe commented Nov 1, 2023

hius07 commented Nov 1, 2023 • edited Loading

hius07 commented Nov 1, 2023 • edited Loading

NiLuJe left a comment

Choose a reason for hiding this comment

poire-z Nov 3, 2023

Choose a reason for hiding this comment

hius07 commented Oct 31, 2023 •

edited by Frenzie

Loading

NiLuJe commented Oct 31, 2023 •

edited

Loading

hius07 commented Nov 1, 2023 •

edited

Loading

hius07 commented Nov 1, 2023 •

edited

Loading

hius07 commented Nov 1, 2023 •

edited

Loading