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

[wip]: android: (re)enable downloads using full apks #4639

Merged
merged 2 commits into from Feb 23, 2019

Conversation

Projects
None yet
3 participants
@pazos
Copy link
Contributor

pazos commented Feb 22, 2019

Requires koreader/android-luajit-launcher#122

to-do:

  • make hasOTAUpdate false for Fdroid.
    - better name for "koreader-latest.apk".

won't fix:

  • opening the apk when downloaded, it is a hell, requires special permissions, a file provider on Nougat+, probably breaks our target api, requiring api24+, forcing us to request permissions at runtime instead of install time...

  • display a notification when the file is downloaded, because it can't be translated.

preview:

untitled

@pazos pazos requested a review from Frenzie Feb 22, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Feb 22, 2019

Completely unrelated question, because I've just noticed your version tag, and I just did a build from my fork too: how do we get the light tags in our forks so that we end up with matching versions compared to "upstream"?

(Because I'm at v2013.03-4877 on my fork, which is, err, a bit unwieldy :D).

@Frenzie
Copy link
Member

Frenzie left a comment

I think opening it automatically when the download has finished would basically be an anti-feature, anyway.

@@ -222,6 +222,8 @@ function OTAManager:fetchAndProcessUpdate()
ok_callback = function()
if Device:isSDL() then
os.execute("xdg-open '"..link.."'")
elseif Device:isAndroid() then
Device:update(link, "koreader-lastest.apk")

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2019

Member

I don't understand the rationale behind wanting to discard the perfectly nice filename for one lacking in clarity.

(As an aside, it'd be latest.)

This comment has been minimized.

@pazos

pazos Feb 22, 2019

Author Contributor

I was thinking in "koreader-2019-xx-xxx.apk"

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2019

Member

Definitely not the place of this PR. That falls under #4297.

This comment has been minimized.

@pazos

pazos Feb 22, 2019

Author Contributor

See below

@@ -109,6 +109,18 @@ function Device:retrieveNetworkInfo()
end
end

function Device:update(url, filename)
-- download the update file if not present
local is_already_downloaded = android.download(url, filename)

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2019

Member

Besides being less enthused with this concept tout court (see below), wouldn't this always return true after the first time?

This comment has been minimized.

@pazos

pazos Feb 22, 2019

Author Contributor

it returns true if the file exists, thats why I think we should add version to downloaded apks.

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2019

Member

What do you mean by "add version to downloaded apks"? Why are you removing it in the first place?

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2019

Member

The update file at http://ota.koreader.rocks/koreader-android-latest-nightly contains this:

koreader-android-arm-linux-androideabi-v2019.02-72-gb48b0d2_2019-02-22.apk

This will point the link variable to:

http://ota.koreader.rocks/koreader-android-arm-linux-androideabi-v2019.02-72-gb48b0d2_2019-02-22.apk

But you're replacing it with a mysteriously named file instead.

This comment has been minimized.

@pazos

pazos Feb 22, 2019

Author Contributor

What do you mean by "add version to downloaded apks"? Why are you removing it in the first place?

I didn't remove a thing. It is just that I need to specify a name for saving a url. The file gets downloaded to /sdcard/Downloads with x name. x name is not obtained from url but specified manually. That's why I want to replace the "koreader-latest.apk" whatever name for the apk is inside the link variable.

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2019

Member

I see. There does seem to be something built into Android that might help: https://developer.android.com/reference/android/webkit/URLUtil.html#guessFileName(java.lang.String,%20java.lang.String,%20java.lang.String)

But as far as I'm concerned your proposed solution is essentially fine. Just change this line to include ota_package.

return ota_version, local_version, link

With return ota_version, local_version, link, ota_package you'll have the filename as the fourth return value.

This comment has been minimized.

@pazos

pazos Feb 22, 2019

Author Contributor

Thanks, done!

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 22, 2019

@NiLuJe They're not light tags, but annotated tags. ;-) You'll need to git fetch --tags (from upstream).

@Frenzie Frenzie added the Android label Feb 22, 2019

@@ -109,6 +109,18 @@ function Device:retrieveNetworkInfo()
end
end

function Device:update(url, filename)

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2019

Member

If we add a Device:update() there should also be a no-op in generic.

This comment has been minimized.

@pazos

pazos Feb 22, 2019

Author Contributor

moved that to ota manager 👍

@Frenzie Frenzie added this to the 2019.03 milestone Feb 22, 2019

@pazos pazos force-pushed the pazos:android_download_updates branch from e5cb975 to ea0512f Feb 22, 2019

Show resolved Hide resolved frontend/ui/otamanager.lua Outdated
@Frenzie
Copy link
Member

Frenzie left a comment

Much better. 👍

Update frontend/ui/otamanager.lua
Co-Authored-By: pazos <paziusss@gmail.com>

Frenzie added a commit to Frenzie/koreader that referenced this pull request Feb 23, 2019

Frenzie added a commit that referenced this pull request Feb 23, 2019

@Frenzie Frenzie merged commit 0532d7a into koreader:master Feb 23, 2019

1 check failed

ci/circleci Your tests failed on CircleCI
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.