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

Add support for offline AnkiDroid manual and help pages #10027

Closed
wants to merge 2 commits into from

Conversation

ShridharGoel
Copy link
Member

Purpose / Description

Add support for offline AnkiDroid manual.

Fixes

Fixes #6829

How Has This Been Tested?

Verified on an emulator.

Checklist

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@ShridharGoel ShridharGoel changed the title Add support for offline AnkiDroid manual Add support for offline AnkiDroid manual and help pages Dec 30, 2021
@ShridharGoel ShridharGoel marked this pull request as ready for review December 30, 2021 06:47
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. One question regarding the release script.

My main question/issue is that we don't seem to be handling translations here.

@mikehardy what was intended here for this use case?

tools/release.sh Outdated Show resolved Hide resolved
@ShridharGoel
Copy link
Member Author

My main question/issue is that we don't seem to be handling translations here.

Translated HTML files are not present in ankidroiddocs currently.

@abdnh
Copy link
Contributor

abdnh commented Jan 1, 2022

Translated HTML files are not present in ankidroiddocs currently.

Translation sources are present in https://github.com/ankidroid/ankidroiddocs so they can be simply rendered and copied here as it's done for the English files. Can't they?

@ShridharGoel
Copy link
Member Author

Translated HTML files are not present in ankidroiddocs currently.

Translation sources are present in https://github.com/ankidroid/ankidroiddocs so they can be simply rendered and copied here as it's done for the English files. Can't they?

Yes, didn't notice earlier that they are present. Thanks for the information.

@mikehardy
Copy link
Member

I appear to have damaged this PR via a merge of a Kotlin migration PR @ShridharGoel apologies - please forgive me/us as we work through Kotlin migration and it impacts other PRs

The use case here is to in general have the app be as "offline first" as possible, so I like this change. With regard to translations it appears that is handled much more thoroughly now with the last couple of commits.

Now I have a slightly different concern which is that we will eventually use the .abb format (vs the .apk format) to publish AnkiDroid to the play store, and with that format Google will be able to "render" a downloaded apk that has only the localizations that are actually needed. In that way it will keep the download/install size on device as small as possible.

On the other hand we let the user switch to whatever localization they want (and that's actually a feature...) so it I think we actually need to keep all the localizations, so this works for me

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except for the Kotlin-merge on HelpDialog this LGTM

@mikehardy mikehardy added Needs Second Approval Has one approval, one more approval to merge Needs Author Reply Waiting for a reply from the original author labels Jan 2, 2022
@ShridharGoel

This comment was marked as resolved.

@mikehardy mikehardy removed the Needs Author Reply Waiting for a reply from the original author label Jan 2, 2022
@david-allison
Copy link
Member

Took a conflict.

What impact does this have on the apk size? I'm concerned about the translated manuals - they're big.

@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Jan 4, 2022
@ShridharGoel
Copy link
Member Author

After:
image

Before:
image

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1MB hurts, but seems acceptable as long as we have a follow up issue to split them out.

@mikehardy Thoughts?

@david-allison
Copy link
Member

@ShridharGoel Could you rebase this onto master - we can't rebase/merge this currently

@mikehardy
Copy link
Member

Dang. I'm really conflicted here. 1MB is a lot, it's a lot a lot - especially since this cost will scale linearly with any added translations in the future. We should want that, the more translations of Help and Manual we get the better. But if it's increasing APK size at this rate I'll have a nagging feeling like I don't want the translations ;-). That's not good.

With sincere regret I think this kills the idea for me temporarily, I don't think we can support even 1MB for current ones right now, that would feel like a regression to many users I think, for functionality they didn't even need.

I still like the goal of having an offline manual + help etc though. And ideally it should be in the users preferred language.

Here is my thinking on moving forward.

  1. Actionable now: require that the resources for the manual / help for all languages are provided in the standard "alternative resources" / locale-specific style that google understands and move the current English ones to it as proof of concept, as the "default" resource for locales missing a translation: https://developer.android.com/guide/topics/resources/providing-resources#AlternativeResources
  2. make the extension of adding new languages dependent / blocked-for-merge on the move from APK to ABB (Switch AnkiDroid publishing from APK-first to AAB-first #9259). Why? Once we ship .abb as our default, then our verify that Google is shipping optimized apks to end users - https://developer.android.com/guide/app-bundle - at that point users will get at most 2 translations, the default/English one, and whatever their device locale is set to
  3. Stretch goal: for users that have set a non-default locale on their device, somehow download their preferred translation ? I'm not even sure how that will work. To be honest, that's a problem with the ABB format's optimized delivery system and I suppose we'll have to handle that somehow, perhaps with a "feature pack" where all translations are downloaded for users that specify a non-device locale in our app preferences. Otherwise they'll never get the locale resources needed to translate....I'll add that to the ABB issue.

@ShridharGoel
Copy link
Member Author

@mikehardy For now, should we go ahead with only the English version?

@mikehardy
Copy link
Member

mikehardy commented Jan 6, 2022

Having the English version included, in a locale-specific resource directory would be perfect to start - it will get us on the path towards having it included, having it included in the user's language, but not bloating the app too much (concretely: having at most 2 manuals for non-English users, and at most 3 manuals for non-English users who use custom locales - vs O(n) manuals for all users 😅 )

@ShridharGoel
Copy link
Member Author

@mikehardy Is there a way to use HTML file in a resource specific directory?

@mikehardy
Copy link
Member

I wish I knew, and even better: I wish I knew and I wish I knew it was easy. But I don't apologies

@ShridharGoel
Copy link
Member Author

@mikehardy I think asset folder is not based on locales, so if we add the English version then it will be included for all languages.

@mikehardy
Copy link
Member

I believe you are correct, above I was thinking it would be in values somewhere, that's locale specific

@ShridharGoel ShridharGoel added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Jan 30, 2022
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, and I'm looking forward to getting this in.

  • Could you squash the commits (or split them out + remove the revert), and add a descriptive commit message explaining briefly how the files are created and updated.

@mikehardy I'd like consider a README.md inside the assets to explain the manual generation process

I think we want to include the manual and help on all releases, not just the public (non-alpha) release.

What do you think about the above?

AnkiDroid/src/main/res/values/03-dialogs.xml Outdated Show resolved Hide resolved
@david-allison david-allison removed the Needs Second Approval Has one approval, one more approval to merge label Jan 31, 2022
This has been done only for English version as of now
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer for the rendered versions of help.html and manual.html to not be committed, I normally prefer for "derivative" artifacts to remain out of source control as they are just a source of confusion and possible merge conflict. Just removing them entirely seems fine to me, or if necessary putting a placeholder in similar to the changelog that just explains these files are rendered during the release process and if you are seeing the placeholder, something went wrong with the release process

I had a separate question about locale-specific assets overlay - I'm not certain they actually do overlay? And that's an important problem to solve if not

Removing the public conditional in the release script seems like an easy change

tools/release.sh Outdated Show resolved Hide resolved
@@ -184,7 +184,9 @@
<string name="link_wikipedia_open_source">http://en.wikipedia.org/wiki/Open_source_software</string>
<string name="link_contribution">https://github.com/ankidroid/Anki-Android/wiki/Contributing</string>
<string name="link_help">https://docs.ankidroid.org/help.html</string>
<string name="asset_link_help">file:///android_asset/help.html</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these directories actually locale-specific? That is, currently the change you made renders in to src/main/assets/manual.html, If I had a device set to locale es-EC, and there was a manual in src/main/assets-es-EC/manual.html would that load in it's place?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assets directory is not locale-specific.

Co-authored-by: Mike Hardy <github@mikehardy.net>
@mikehardy
Copy link
Member

If the assets folder is not locale-specific this is a no-go for me. We cannot include locale-specific manuals in non-locale-specific places I think or we'll be headed down the wrong path. This may actually represent a regression for most as opening the web manual for most people where the manual is not in their language should at least trigger their web browser to offer to translate it for them automatically, whereas including it locally like this will force them to English with no ability to override

We must figure out locale-specific asset loading

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Mar 3, 2022
@ShridharGoel
Copy link
Member Author

@mikehardy The earlier solution had included separate files based on language but it was creating an increase in the APK size.

@krmanik
Copy link
Member

krmanik commented Mar 11, 2022

I am converting the manual pages to mdbook. It is not complete but it is coming soon.
ankidroid/ankidroiddocs#97

Instead of adding manual to app, it will be to better download the file after installation of AnkiDroid.
Or an option for user to download the manual for offline view.

image

@mikehardy
Copy link
Member

@ShridharGoel I don't feel like I have communicated my desire clearly

In the future, we will generate ABB files for distribution, and Google will remove versions of the manual for locales that are not your system locale.

We can use Google APIs to download locale-specific manuals for users that have used our in-app selector to choose a non-default locale.

For this to work, the manuals must be in a location that is part of the general locale-specific android file organization / build process.

We are not ready for ABB distribution yet, and we do not have the non-default-locale API in use yet to download non-default-locale translations. That means that we cannot put multiple translations in without increasing app size. However the English translation can can go in now, so long as it is in a locale-specific location

In order for this to not be a regression for locales where there is no translation, or it is not packaged at all, there should be a button or toast that says "Read the manual online", which opens it in a browser and then the user has access to google translate etc

If that's not clear in any way, please let me know.

@krmanik I actually would like to have the manual available offline in a way that works (offline) for the majority of users, not for it to be downloaded dynamically except for the rare case where the user has chosen a non-English non-default version of the manual. This is not an issue about the format of the manual, this is about packaging and distribution

@github-actions
Copy link
Contributor

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ship manual and help instructions offline in the app, similar to changelog
5 participants