New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat] Open with: choose which engine to use for file #3653

Merged
merged 1 commit into from Feb 2, 2018

Conversation

Projects
None yet
3 participants
@Frenzie
Member

Frenzie commented Feb 1, 2018

Fixes #3345

screenshot_2018-02-01_11-08-22
screenshot_2018-02-01_11-32-36

@Frenzie Frenzie requested review from robert00s and poire-z Feb 1, 2018

@Frenzie Frenzie added the enhancement label Feb 1, 2018

@@ -250,7 +251,7 @@ function PdfDocument:register(registry)
registry:addProvider("xhtml", "application/xhtml+xml", self, 100)
registry:addProvider("xml", "application/xml", self, 10)
registry:addProvider("xps", "application/oxps", self, 100)
registry:addProvider("zip", "application/zip", self, 100)
registry:addProvider("zip", "application/zip", self, 10)

This comment has been minimized.

@Frenzie

Frenzie Feb 1, 2018

Member

Either this or crengine should be upped to 20. Previously only MuPDF did zip; not sure why that one specifically.

@poire-z

Looking fine code wise.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 1, 2018

Some thoughts:

  • be sure to check how coverbrowser (which hacks the filemanager onHold buttons list to add its own stuff in between original buttons) does with your new Open with button.
  • any alternative to the stars in ** engine ** ? May be by implementing a no_bold attribute to Button, for the others, or a background for it?
  • minor stuff that I will get used to reading :) but the word provider sounds a bit strange for what it is, why not just engine ?

May be a more important issue:
if I remember correctly, when I tried mupdf for epub, there is some incompatibilites in the docsettings content between engines. I think the highlights are different (xpointer for cre, pages for mupdf, some other attributes there or not depending on engine) - and koreader may crash when you open a book previously read with mupdf, with crengine, as crengine will not find what it expects to have in a highlight.
So, if the above is really happening, 2 options: make things more solid so it does not fail (ignore ? migrate?) - or a different docsetting per engine... Well, not sure, to check and think about.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Feb 1, 2018

but the word provider sounds a bit strange for what it is, why not just engine

I'll have to check if there's a reason; I didn't write it.

Regarding highlights and bookmarks good point. I guess that for this PR I'll add a reset for those when switching?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 1, 2018

I didn't write it.

Oh, right, it was already named that, didnt shocked me before - I just haven't read it as many times as when reading this PR code. Better let it be named like that then.

I'll add a reset for those when switching?

OK, with a confirm box similar to the Purge sdr then :)

@KenMaltby

This comment has been minimized.

KenMaltby commented Feb 1, 2018

What is the difference between "Just once" and "This document" ? Doesn't the difference between "This document" and "All documents" imply that "This document" would remain using the selected reader, until the user changes it, after one reading session or many? You might need this for the listing in History, as well, so an existing setting can be changed.

"All Documents" means all documents with the same extension? Or really all documents, opened after this?

You might want to have a way to indicate the current setting for the document long-pressed, perhaps the default, also.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Feb 1, 2018

What is the difference between "Just once" and "This document" ?

"Just once" means open this document with this engine now but don't touch any settings.

"This document" means to always open this document with this engine from now on (but no other documents of the same type).

"All Documents" means all documents with the same extension? Or really all documents, opened after this?

Yes, all documents with the same extension. I'll add the extension in there. All pdf documents or something like that.

You might want to have a way to indicate the current setting for the document long-pressed, perhaps the default, also.

Something for Book information I reckon.

@KenMaltby

This comment has been minimized.

KenMaltby commented Feb 1, 2018

I was thinking something like greyed out, regular, Bold, or italic, to have an indication while viewing the selections available. In your example the highlighted CR3 selection could be what is currently set. Perhaps a gray background for another option if that were the default. (If the default were not the current selection)

@Frenzie Frenzie force-pushed the Frenzie:open-with branch from bf6e332 to 29598f8 Feb 1, 2018

@Frenzie

This comment has been minimized.

Member

Frenzie commented Feb 1, 2018

@poire-z How about something like this? It needs to go in ReaderUI because you can open files from all over the place (History, Search, etc.)

screenshot_2018-02-01_18-10-32

@Frenzie

This comment has been minimized.

Member

Frenzie commented Feb 1, 2018

@KenMaltby

I was thinking something like greyed out, regular, Bold, or italic, to have an indication while viewing the selections available.

I would've done that if it were readily available. ;-)

I don't really want to write new UI things atm, just to easily enable some cool new stuff that's currently hard to effect without constantly changing the source. How about something like "(Current)" between parentheses behind the name of the engine?

@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 1, 2018

(The prompt code and ui looks fine.)
Are you sure bookmarks are the only incompatible thing, and there's no other issue with other settings? If yes, that's less brutal than what I was expected (thought you would just delete the whole docsettings :).

@Frenzie

This comment has been minimized.

Member

Frenzie commented Feb 1, 2018

I haven't noticed anything, but besides making sure the switching itself works I haven't really tested it too much. I just wrote this today. ;-) Besides bookmarks I think all of the settings are generally sufficiently mutually meaningless between crengine and mupdf.

There are some associated issues, but not about DocSettings. Trying to create a highlight in an EPUB will currently crash MuPDF and there might be other issues caused by our MuPDF reader view being PDF-centric. There's doubtless more that'll pop up. I could hide the Open with dialog itself away behind an advanced settings toggle if a few issues of that nature are considered too large of a potential issue.

@KenMaltby

This comment has been minimized.

KenMaltby commented Feb 1, 2018

I think this would be something different, but:
If we were just talking setting "file associations" perhaps two columns of "checkbox .extension", one for CR3 and one for MuPDF. If on the same page, it could be setup so the same extension could not be selected in both.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Feb 1, 2018

There are definitely better ways to present it, though most of them would likely involve writing new widgets. I can probably copy Windows in a slightly worse way without too much effort.

Except it'd all be the CheckButtons I made last year #3088

(maybe I should also write a RadioButton, but again, not really my focus here :-P)

It can't be copied one on one though. I definitely want per-document selection from the POV that crengine is generally but not necessarily better. For example, @poire-z had some sample enormous EPUB that loaded significantly faster.

So what I'm thinking is something like:

  • mupdf (not checked; checking it will uncheck all other options)
  • crengine (already checked because it's the current setting)

  • Always use this engine for this file
  • Always use this engine for file epub files

That way "just once" disappears out of the picture because that's simply what happens if you don't tick either of the always options.

@Frenzie Frenzie force-pushed the Frenzie:open-with branch from 3614a49 to d93cda8 Feb 1, 2018

[feat] Open with: choose which engine to use for file
Fixes #3345

* Add SVG to MuPDF filetypes

@Frenzie Frenzie force-pushed the Frenzie:open-with branch from d93cda8 to bca9a1e Feb 1, 2018

@KenMaltby

This comment has been minimized.

KenMaltby commented Feb 1, 2018

Yeah, that seems most consistent with what is done now. I like that approach a lot.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Feb 2, 2018

I'll merge this as is so people can start playing around with it (and find bugs :-P) and in the meantime I'll start working on a RadioButton and a RadioButtonDialog widget.

@Frenzie Frenzie merged commit f6ca1c7 into koreader:master Feb 2, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@Frenzie Frenzie deleted the Frenzie:open-with branch Feb 2, 2018

@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 10, 2018

function DocumentRegistry:getProvider(file)
local providers = self:getProviders(file)
if providers then
-- provider for document
local doc_settings_provider = require("docsettings"):open(file):readSetting("provider")
if doc_settings_provider then

will be called for each file in a directory because of:
file_filter = function(filename)
if DocumentRegistry:getProvider(filename) then
return true
end
end,

which:

  1. will create an empty sidecar directory (and I'm allergic to them :)
  2. will read all the existing metadata.lua, which may cause some slow down (see #3614 (comment)).

I would solve 1) with:

        -- provider for document
        local DocSettings = require("docsettings")
        if DocSettings:hasSidecarFile(file) then
            local doc_settings_provider = DocSettings:open(file):readSetting("provider")
            if doc_settings_provider then

Not sure about 2) (an option to DocumentRegistry:getProvider() so it does not look for specific provider, as its obviously one of those returned by getProviders(), or just use getProviders() in filemanager file_filter.

Dunno if there are other code that call getProvider() many times, so may be it should have a non-default option to go look for the docsetting value (that would be used by readerui and the onHold only) ?

@Frenzie

This comment has been minimized.

Member

Frenzie commented Feb 10, 2018

I'd be more inclined to add a hasProvider() (even if it's technically just a boolean based on getProviders).

But yes, I'd definitely prefer using getProviders() or adding hasProvider() over adding weird flags.

@Frenzie

This comment has been minimized.

Member

Frenzie commented Feb 10, 2018

PS Adding it right now. I reckon this is more efficient:

diff --git a/frontend/document/documentregistry.lua b/frontend/document/documentregistry.lua
index da80c98c..0c2a2994 100644
--- a/frontend/document/documentregistry.lua
+++ b/frontend/document/documentregistry.lua
@@ -13,6 +13,7 @@ local T = require("ffi/util").template
 local DocumentRegistry = {
     registry = {},
     providers = {},
+    filetype_provider = {},
 }
 
 function DocumentRegistry:addProvider(extension, mimetype, provider, weight)
@@ -22,6 +23,19 @@ function DocumentRegistry:addProvider(extension, mimetype, provider, weight)
         provider = provider,
         weight = weight or 100,
     })
+    self.filetype_provider[extension] = true
+end
+
+--- Returns true if file has provider.
+-- @string file
+-- @treturn boolean
+function DocumentRegistry:hasProvider(file)
+    local filename_suffix = util.getFileNameSuffix(file)
+
+    if self.filetype_provider.filename_suffix then
+        return true
+    end
+    return false
 end
@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 10, 2018

Thanks :)

Frenzie added a commit to Frenzie/koreader that referenced this pull request Feb 10, 2018

Frenzie added a commit that referenced this pull request Feb 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment