Skip to content
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

[very wip]: add support for ACTION_VIEW on content:// schemes #138

Closed
wants to merge 1 commit into from

Conversation

pazos
Copy link
Member

@pazos pazos commented Apr 3, 2019

Edit: updated PR to add support for Android Storage Access Framework on KitKat+ devices. This means gaining access to dropbox, gdrive or any other content providers and being able to import documents from there. This seems important to some users.

This PR implements a interface to obtain a string containing the full path of the file you select on the SAF file picker. We accept all kind of mimetypes here to import.

The frontend needs to be updated to implement the rest of the code:

in openable/readable files: show a menu file imported, now what? with some options. move home, move to, open from cache...

in non readable files just move the file to users home is fine.

original message

Not sure if this is a good idea or we just need to label koreader/koreader#4441 as can't fix and avoid to open uris with content scheme.

This PRs works as is:

  • if the intent provides an uri with scheme "file://" works without problems.
  • if the intent provides an uri with scheme "content://":
  1. get a content resolver
  2. open a input stream
  3. store the stream into a cache file
  4. get the full path to that cache file
  5. finally open the file, now that we have a valid path.

I've followed recommendations from https://commonsware.com/blog/2016/03/15/how-consume-content-uri.html, so no strange workarounds that can work on some folders in some devices. This method should work in every android device from every app.

But has some problems:

  1. cache files are stored in /data/data/... and is impossible to return to /sdcard unless "home" is actually set to that folder.
  2. sdr folder is stored on the same path of the file. If we open the same file from two locations (ie: opening a file via ko file browser or consuming a intent via inputstream) then there are two sdr folders.
  3. ofc, inputstream makes a duplicate of the file. That's a bad thing too.

Most of the problems are common to other android document viewers but some are very related to how KOReader works. I found just one app that does the things differently: ReadEra.

ReadEra scans the entire sdcard, looking for recognized mimetypes and hashes each file. It stores the info in a database. When an application opens a file declared as ACTION_VIEW by ReadEra the program just looks if the name is indeed in the database and gets the file path from there. If the file is not in the database it uses inputStream to create a cache file, does a sha256sum and compare the hash agaisnt the database. These are just guesses because the program isn't opensource.

Other programs choose one of the following three routes:

  • they're broken
  • they're importing documents using a cache file (like this PR) and have a file browser to import new content from the application
  • they're skipping the ACTION_VIEW on content schemes and just let you explore the sdcard filesystem (like KOReader is doing now).

I'm going to need some feedback here @Frenzie @poire-z @NiLuJe

@Frenzie
Copy link
Member

Frenzie commented Apr 3, 2019

Looks okay at a quick skim (no time to read in detail today).

It seems more or less the least bad option.

@poire-z
Copy link
Contributor

poire-z commented Apr 3, 2019

(Personally, I just put my books into my koreader HOME, and open them from koreader file browser. So I guess can't fix is fine with me :) But if I were only half me, the tidy part of me, I would hate the books to be copied in some place, on which I don't have any real clean up option.)
And if I were not me, like most users aren't :) I guess I would not care much about the crap that ends up on my phone, and just want the opening to work.

So, some questions and thoughts:

if the intent provides an uri with scheme "file://" works without problems.
if the intent provides an uri with scheme "content://":

What decides with scheme is used? The Android version? The app developper in the app itself?
Most file explorers would use file:// then, no? Only other apps that download stuff or access network resources would download and/or provide a stream to the content itself with content:// ?

If it's as clear as that (I guess it's not :), we could just have a popup telling the user "Do you want to import this book in Koreader?" and save the book into HOME/imported/HASHHASHHASH.epub (or .pdf, I guess the mimetype you get is correct) - and have the .sdr alongside there. No real filename (do you get it somehow?), but the Cover views would show author and title from metadata. Having the hash in the filename would allow you to notice it has already been imported (if the user keeps opening it from that other app) and you can open it directly.

But yes, if we get via content:// a file that is already there near KOreader HOME, it's crappy. So, just an additional warning about that in the "Do you want to import this book in Koreader? If you can open this book from KOReader, please do, otherwise it will be duplicated. Or use another File explorer."

(Indexing the whole sdcard is an ugly idea if we were to do that just for carefree users - have them learn to cherish and organize their collections :)

@NiLuJe
Copy link
Member

NiLuJe commented Apr 3, 2019

We could add the hashing as part of whatever we currently do when browsing a file in the FM for coverview, and then comparing that hash live and redirecting to the local copy if it's already been seen (sql query?).

But this seems fairly convoluted for a crappy corner-case, so, eh :D. Also, a massive perf hit everywhere (the hashing) for something used on a single platform, and even there, maybe not ;).

@pazos
Copy link
Member Author

pazos commented Apr 3, 2019

What decides with scheme is used? The Android version? The app developper in the app itself?

App developers can choose the scheme in Android until Nougat (7.0). In Oreo (8.0) file providers are forced to use a content scheme.

No real filename (do you get it somehow?)

We get a display name, which doesn't need to match the filename. But in all scenarios I tested it will match the filename.

I guess the mimetype you get is correct

We cannot guess the mimetype without saving the document first. Once the document is saved we can find its type without problem.

@pazos
Copy link
Member Author

pazos commented Jul 1, 2019

Now, with koreader/koreader#4441 closed, I've repurposed this PR to implement Storage Access Framework.

Sadly, I'm not able to import directly to (any) place in sdcard. the file needs to be copied on cache dir and then KOReader should be able to receive the full path of the imported file and ask users what they want to do:

Here is an example showing how the storage access framework is started as a new activity on top of KOReader:

untitled

Obviously, It doesn't try to open/copy the file. But this can be handled entirely in the frontend.

@Frenzie
Copy link
Member

Frenzie commented Jul 1, 2019

Rather awkward usability-wise, but it seems to be about as good as it gets. 👍

@pazos
Copy link
Member Author

pazos commented Jul 1, 2019

Rather awkward usability-wise, but it seems to be about as good as it gets. +1

The good thing is that it will allow to get books from different content providers (dropbox, google drive...)

And the other good thing is that we can merge these kind of pull request without affecting KOReader, because its implementation needs to be done in frontend/device/android using the app_glue code.

The example attached above just needs these changes:

diff --git a/frontend/device/android/device.lua b/frontend/device/android/device.lua
index 93920eef..09a9fc3f 100644
--- a/frontend/device/android/device.lua
+++ b/frontend/device/android/device.lua
@@ -86,6 +86,8 @@ function Device:init()
                 or ev.code == C.APP_CMD_WINDOW_REDRAW_NEEDED then
                 this.device.screen:_updateWindow()
             elseif ev.code == C.APP_CMD_RESUME then
                 local new_file = android.getIntent()
                 if new_file ~= nil and lfs.attributes(new_file, "mode") == "file" then
                     -- we cannot blit to a window here since we have no focus yet.
@@ -101,6 +103,19 @@ function Device:init()
                         require("apps/reader/readerui"):doShowReader(new_file)
                     end)
                 end
+                -- check if we return from importing a document
+                local last_document_imported = android.getLastImportedFile()
+                if last_document_imported ~= "none" then
+                    -- we cannot blit to a window here since we have no focus yet.
+                    local UIManager = require("ui/uimanager")
+                    local InfoMessage = require("ui/widget/infomessage")
+                    UIManager:scheduleIn(0.1, function()
+                        UIManager:show(InfoMessage:new{
+                            text = T(_("File was stored in the cache: %1\nWhat do you want to do?."), last_document_imported),
+                        })
+                    end)
+                end
             end
         end,
         hasClipboardText = function()
diff --git a/frontend/ui/elements/common_settings_menu_table.lua b/frontend/ui/elements/common_settings_menu_table.lua
index 4dee307e..baca1da1 100644
--- a/frontend/ui/elements/common_settings_menu_table.lua
+++ b/frontend/ui/elements/common_settings_menu_table.lua
@@ -189,6 +189,20 @@ if Device:isAndroid() then
             callback = function() require("ui/elements/screen_android"):toggleFullscreen() end,
         }
     end
+    -- storage access framework is available from Kitkat
+    if Device.firmware_rev >= 19 then
+        common_settings.import_file = {
+            text = _("import a file"),
+            callback = function()
+                ok = android.importFile()
+                if not ok then
+                    UIManager:show(InfoMessage:new{
+                        text = _("Unable to import files from Android Storage Framework,please check the logs with logcat")
+                    })
+                end
+            end,
+        }
+    end
 end
 
 if Device:isTouchDevice() then
diff --git a/frontend/ui/elements/filemanager_menu_order.lua b/frontend/ui/elements/filemanager_menu_order.lua
index ed6e9e05..5fa2baaf 100644
--- a/frontend/ui/elements/filemanager_menu_order.lua
+++ b/frontend/ui/elements/filemanager_menu_order.lua
@@ -139,7 +139,9 @@ local order = {
         "----------------------------",
         "about",
     },
-    plus_menu = {},
+    plus_menu = {
+        "import_file",
+    },
     exit_menu = {
         "restart_koreader",
         "----------------------------",

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.

None yet

4 participants