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

apiVersioning and developerContact implementation for AndkiDroid functions call from WebView #6377

Closed
wants to merge 17 commits into from

Conversation

krmanik
Copy link
Member

@krmanik krmanik commented Jun 6, 2020

Pull Request template

Purpose / Description

This is implementation for api versioning for usage of AnkiDroid native functions in WebView.

Fixes

Fixes #6328

Approach

Getting api version & developer contact from card developer, then allow them to call the above functions implementation.

How Has This Been Tested?

Tested on Emulator
Calling this function before calling #6307 implemented functions

<script>
   AnkiDroidJS.init(ankiJSapiVersion, ankiDeveloperContact)
</script>

as

AnkiDroidJS.init("1.0.0", "dev1")

Learning (optional, can help others)

https://stackoverflow.com/questions/51513715/node-js-rest-api-versioning-the-right-way
https://stackoverflow.com/questions/32228300/how-to-prevent-my-snackbar-text-from-being-truncated-on-android

Checklist

Please, go through these checks before submitting the PR.

  • [ x ] You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • [ x ] You have a descriptive commit message with a short title (first line, max 50 chars).
  • [ x ] Your code follows the style of the project (e.g. never omit braces in if statements)
  • [ x ] You have commented your code, particularly in hard-to-understand areas
  • [ x ] You have performed a self-review of your own code

	Updated AbstractFlashcardViewer.java

	Updated AbstractFlashcardViewer.java

	Updated AbstractFlashcardViewer.java & Card.js
@krmanik
Copy link
Member Author

krmanik commented Jun 6, 2020

I have just implement it to know that am I on right track for this implementation?

@krmanik
Copy link
Member Author

krmanik commented Jun 6, 2020

As I understand the api versioning but for implementation part I need starting point.

@krmanik krmanik marked this pull request as draft June 8, 2020 16:14
@mikehardy
Copy link
Member

tagging this up for review of the design - thanks for putting this up @infinyte7 ! @david-allison-1 this is a PR (that I haven't looked at yet, sorry) for the API management stuff we were batting around

@mikehardy
Copy link
Member

This looks like what I was thinking for javascript developers to call to register, and then we'll have the processed values in Java to do what "seems right"

Then the following bits from the related issue where

  • inject a warning in review below the reviewer minibar / top of card if any parts of the interface were used with either no init or a deprecated API, including a link to a wiki page for more info - bonus points if it is dismissable for the entirety of a review session
  • a wiki page explaining that we're adding to the interface and need to be able to version it to manage change over time, plus the exact changes (ideally copy-pastable for users) to get rid of the warning

So I think next step is

  • abstracting the tentative / design review && "1.0.0".equals(apiVersion)) to something like a requireApiVersion("1.0.0") and
  • that requireApiVersion method should decompose major/minor/patch to make sure that current API version (sJsApiVersion = "1.0.0" ?) is at or above it

Here's where I'd just go for a side effect where we maybe also have a toggle (mJsApiCorrect ?) and if the card either has not registered or registered with an API we deem insufficient (maybe they are behind?) we inject a chunk of javascript that displays the API warning on top with a link to the wiki and developer contact (if the developer contact exists)

Please note that I am almost just thinking out loud here - this is like the brainstorming phase of design for the feature for me. I might be missing something obvious that you or anyone else sees, or there may be a much easier way to accomplish my real goal (which is for users to know when the Javascript on their cards might be broken, and if it was written by someone else, who and where to go for help, so they don't log issues here)

So any thoughts or completely different designs are welcome, as long as they meet that goal

@mikehardy mikehardy added the Needs Author Reply Waiting for a reply from the original author label Jun 11, 2020
@krmanik
Copy link
Member Author

krmanik commented Jun 11, 2020

I understand the things but according to current knowledge it seems challenging but want to implement it so that I can learn more about this.

I will update this PR with above idea.
Thank you @mikehardy

@mikehardy
Copy link
Member

Sure thing! And the learning is the fun part :-), I don't consider this blocking for your other stuff either, I appreciate that you're giving it a go in parallel. Cheers

@krmanik
Copy link
Member Author

krmanik commented Jun 12, 2020

It work as expected.

Using deprecate api without develper contact.

Using deprecate api with develper contact.

@krmanik krmanik marked this pull request as ready for review June 12, 2020 08:19
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.

Likely on the side of overengineering here (@mikehardy - could do with guidance), but I hope this API gets long-term usage.

This approach doesn't allow a developer a capability-based approach. A developer can't currently know that their desired features are not supported and take corrective action.

I think the above approach will come in handy later on. I specifically will not want to allow my cards to toggle the navigation drawer, or perform "delete note" (for example), and it'd be nice for the developer to know that I've disabled the functionality, even if they choose to ignore the fact and call the API anyway.

@@ -185,6 +186,11 @@
// ETA
private int eta;

// JavaScript Versioning
protected String apiVersion = "";
Copy link
Member

Choose a reason for hiding this comment

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

Could this follow the naming standards

Suggested change
protected String apiVersion = "";
protected String mCardSuppliedApiVersion = "";

@@ -185,6 +186,11 @@
// ETA
private int eta;

// JavaScript Versioning
protected String apiVersion = "";
protected String developerContact = "";
Copy link
Member

Choose a reason for hiding this comment

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

These variables aren't reset between cards, this could theoretically show a previous developer's contact

private void showDeveloperContact() {
View parentLayout = findViewById(android.R.id.content);
String snackbarMsg;
if (developerContact.equals("")) {
Copy link
Member

Choose a reason for hiding this comment

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

Needs to handle the user supplying null here

Suggested change
if (developerContact.equals("")) {
if (TextUtils.isEmpty(developerContact)) {


Snackbar snackbar = Snackbar.make(parentLayout, snackbarMsg, 5000);
View snackbarView = snackbar.getView();
TextView snackTextView = (TextView) snackbarView.findViewById(com.google.android.material.R.id.snackbar_text);
Copy link
Member

Choose a reason for hiding this comment

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

I believe there are auto-linting suggestions here that will let you remove (TextView)

Suggested change
TextView snackTextView = (TextView) snackbarView.findViewById(com.google.android.material.R.id.snackbar_text);
TextView snackTextView = (TextView) snackbarView.findViewById(com.google.android.material.R.id.snackbar_text);

snackTextView.setMaxLines(3);

snackbar.setActionTextColor(Color.MAGENTA)
.setAction("VIEW", view -> {
Copy link
Member

Choose a reason for hiding this comment

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

This will fail if the user does not have a browser. See AnkiActivity.openUrl

snackTextView.setMaxLines(3);

snackbar.setActionTextColor(Color.MAGENTA)
.setAction("VIEW", view -> {
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be extracted to a translatable string constant. Maybe rename to View Wiki.

Does snackbar auto-capitalise this text? showSimpleSnackBar will, and will make it easier for translators to get it right.

@@ -3092,6 +3098,9 @@ private boolean filterUrl(String url) {
}
// flag card (blue, green, orange, red) using javascript from AnkiDroid webview
if (url.startsWith("signal:flag_")) {
if (!requireApiVersion(apiVersion)) {
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to decide which APIs are currently unstable.

@@ -185,6 +186,11 @@
// ETA
private int eta;

// JavaScript Versioning
protected String apiVersion = "";
protected String developerContact = "";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
protected String developerContact = "";
protected String mCardSuppliedDeveloperContact = "";

snackbarMsg = getString(R.string.api_version_developer_contact, developerContact);
}

Snackbar snackbar = Snackbar.make(parentLayout, snackbarMsg, 5000);
Copy link
Member

Choose a reason for hiding this comment

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

Could you extract the constant (presumably the display time). Would it be possible to use LENGTH_LONG (2750ms), or is this too short?

@@ -261,4 +261,7 @@
<string name="restore_default">Restore Default</string>

<string name="reviewer_tts_cloze_spoken_replacement">Blank</string>

<string name="api_version_developer_contact">Using uninitialized or deprecated JS api. Contact developer at %s, or view wiki to resolve this.</string>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<string name="api_version_developer_contact">Using uninitialized or deprecated JS api. Contact developer at %s, or view wiki to resolve this.</string>
<string name="api_version_developer_contact">This card uses unsupported AnkiDroid features. Contact the deck creator at %s, or view the wiki to resolve this manually.</string>

@mikehardy
Copy link
Member

This approach doesn't allow a developer a capability-based approach. A developer can't currently know that their desired features are not supported and take corrective action.

I think the above approach will come in handy later on. I specifically will not want to allow my cards to toggle the navigation drawer, or perform "delete note" (for example), and it'd be nice for the developer to know that I've disabled the functionality, even if they choose to ignore the fact and call the API anyway.

Okay, so what exactly would that look like? spell out the actual methods and arguments and we can see what it looks like. I like the idea, as it might be nice to only allow (for instance) read-only APIs by default and for a developer to be able to handle that

@david-allison
Copy link
Member

david-allison commented Jun 12, 2020

This approach doesn't allow a developer a capability-based approach. A developer can't currently know that their desired features are not supported and take corrective action.
I think the above approach will come in handy later on. I specifically will not want to allow my cards to toggle the navigation drawer, or perform "delete note" (for example), and it'd be nice for the developer to know that I've disabled the functionality, even if they choose to ignore the fact and call the API anyway.

Okay, so what exactly would that look like? spell out the actual methods and arguments and we can see what it looks like. I like the idea, as it might be nice to only allow (for instance) read-only APIs by default and for a developer to be able to handle that

  • Add another parameter/add a field to the object passed to init with a list of strings ['markCard', 'deleteNote', 'microphone']
  • Return a map of the same size with results:
    • ['markCard'] = "allowed"
    • ['deleteNote'] = "unavailable"
    • //['microphone'] = "requiresPermission" - future use
  • The same, but single-parameter. Constant name

Multiple methods raise an API versioning issue.

Caller can ignore the result.

Current (slight) concerns:

  • is this overengineering?
  • Mix of permissions/versioning concerns.

@krmanik
Copy link
Member Author

krmanik commented Jun 12, 2020

Getting information about api version and developer contact and pass all available functions status in one go.

I have implemented that.
My way is that pass developer contact and api version.
If api version match the mCurrentCardJsApiVersion then pass all the enabled/disabled functions.

If card have button to call ankiToggleFlag("red"); and supplied the incorrect / empty api version then nothing will happen, then deck developer can find all status regarding current api implementaion using following.

<script>
var jsApi = { "version" : "1.0.0", "developer" : "[email protected]" }
var apiData = AnkiDroidJS.init(JSON.stringify(jsApi));

// Return string 
var apiStatus = JSON.parse(apiData);

console.log("available api : " + apiStatus);

console.log(apiStatus["toggleFlag"]);  
console.log(apiStatus["markCard"]);
</script>

It will easy if we pass data in one go for all available functions.
Becase iterating for markCard, toggleFlag, microphone, then assigning enabled/disabled or true/false, make reviewing cards a little bit slow.

Capture

When api 1.0.0 passed

en

When api 2.0.0 passed

dis

	Updated apiVersioning

	Updated AbstractFlashcardViewer.java

	api enabled/disabled status

	Updated AbstractFlashcardViewer.java

	Updated AbstractFlashcardViewer.java

	api enabled/disabled status

	Updated apiVersioning
@mikehardy
Copy link
Member

@david-allison-1 what do you think? I do see this mixing "versioning" and "capabilities" but since it's grouped within a well-named (IMHO) 'init' method I think having "the things we care about" (which is currently versioning and capabilities) in there isn't mixing, it's just where you would put them. So I don't think of it as mixing and don't mind.

I like how the version number the JS expects is simply declarative, so on the AnkiDroid side we can do API version progression (planned deprecation, removal) but we can also warn if the JS expects a version that is higher and tell users they need to upgrade AnkiDroid to use the full features of the card.

I'll review the actual implementation in a bit but wanted to flag David to check the current API shape since he proposed capabilities

Many thanks @infinyte7 for actually test-driving the ideas in an implementation we can see as we design it on the fly, I appreciate that as there is nothing like seeing it in action and having someone (you in this case) tell us how it feels

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.

Looks good to me! minor nitpicks and good to go!

@infinyte7 thanks so much for getting this in! Do you have any comments on the implementation, or is there anything here that could make your life easier?

@mikehardy Thanks for the guidance, as always

private void showDeveloperContact() {
View parentLayout = findViewById(android.R.id.content);
String snackbarMsg;
if (TextUtils.isEmpty(mCardSuppliedDeveloperContact )) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (TextUtils.isEmpty(mCardSuppliedDeveloperContact )) {
if (TextUtils.isEmpty(mCardSuppliedDeveloperContact)) {

JSONObject enabledJsApi = new JSONObject();

JSONObject data = new JSONObject(jsonData);
mCardSuppliedApiVersion = data.getString("version");
Copy link
Member

@david-allison david-allison Jun 12, 2020

Choose a reason for hiding this comment

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

Let the user pass invalid data without a crash

Suggested change
mCardSuppliedApiVersion = data.getString("version");
mCardSuppliedApiVersion = data.optString("version", "");

public String init(String jsonData) {
JSONObject enabledJsApi = new JSONObject();

JSONObject data = new JSONObject(jsonData);
Copy link
Member

Choose a reason for hiding this comment

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

Need to try.. catch here, just in-case the user provides bad input

mCardSuppliedApiVersion = data.getString("version");
mCardSuppliedDeveloperContact = data.getString("developer");

if (TextUtils.isEmpty(mCardSuppliedApiVersion) && TextUtils.isEmpty(mCardSuppliedDeveloperContact )) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (TextUtils.isEmpty(mCardSuppliedApiVersion) && TextUtils.isEmpty(mCardSuppliedDeveloperContact )) {
if (TextUtils.isEmpty(mCardSuppliedApiVersion) && TextUtils.isEmpty(mCardSuppliedDeveloperContact)) {

@@ -179,4 +179,6 @@
<string name="read_write_permission_description">access existing notes, cards, note types, and decks, as well as create new ones</string>
<string name="card_browser_label">Card Browser</string>
<string name="card_browser_context_menu">Card Browser</string>

<string name="view">View</string>
Copy link
Member

Choose a reason for hiding this comment

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

Could you rename view to something that will help translators pick the correct phrase: reviewer_invalid_api_version_visit_documentation.

@krmanik
Copy link
Member Author

krmanik commented Jun 13, 2020

I have updated suggested changes.

Comment on lines 3435 to 3444
if (TextUtils.isEmpty(mCardSuppliedApiVersion) && TextUtils.isEmpty(mCardSuppliedDeveloperContact)) {
enabledJsApi.put("toggleFlag", "disabled");
enabledJsApi.put("markCard", "disabled");
} else if (requireApiVersion(mCardSuppliedApiVersion)) {
enabledJsApi.put("toggleFlag", "enabled");
enabledJsApi.put("markCard", "enabled");
} else {
enabledJsApi.put("toggleFlag", "disabled");
enabledJsApi.put("markCard", "disabled");
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this chunk can be cleaner.
I think we will need an array of API strings, so that should be pulled out.
Then there should be a function that returns a map of all API functions, set to disabled.
You can call that function here to get a 'default off' map, then in here (where we are theoretically verifying API version compatibility and permission) you can check the API parameter then iterate and turn them on

So instead of a 3-part boolean test there is just a "default off" pre-init, a quick / short-circuit return if parameters are bad, then an iteration where you toggle on the APIs

Copy link
Member Author

Choose a reason for hiding this comment

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

I used hashmap, so that one time initialization done. At signal:url, checking the hashmap value, when true then available, false unavailable.
One time call of requireApiVersion function in init.

May be more better way for this.

Comment on lines 3363 to 3365
private boolean requireApiVersion(String apiVer) {
return apiVer.equals(sCurrentJsApiVersion);
}
Copy link
Member

Choose a reason for hiding this comment

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

This mechanism needs some work I think, because it needs to take apart the semantic version and compare the fields numerically. This looks like a reasonable way to get semantic version info parsed out and comparable without any fuss https://github.com/AntiLaby/jsemver

We need to handle a few things with regard to versions though:
1- if the template JS for some reason needs a newer version than we support, we need to toast that
1- if the template JS calls an API we do implement, but that we are deprecating, we need to toast that (so we need the notion of deprecation somewhere, per-API?)

Maybe other stuff, try to think through an API lifecycle (AnkiDroid creating a new API and bumping version, deprecating an API, removing an API)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have implemented it but need more improvement.

This is https://github.com/AntiLaby/jsemver nice. But may it will be helpful large project.
I use that idea and implemented in requireApiVersion.

@mikehardy mikehardy removed the Needs Author Reply Waiting for a reply from the original author label Jun 14, 2020
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.

Mostly stylistic suggestions, will need to give this a final look

@@ -53,6 +53,7 @@
import android.text.SpannedString;
import android.text.TextUtils;
import android.text.style.UnderlineSpan;
import android.util.Log;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is used?

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.

Will need the merge in the history removed (or squashed out when merged into master)

@krmanik
Copy link
Member Author

krmanik commented Jun 15, 2020

I have updated it.

@krmanik
Copy link
Member Author

krmanik commented Jun 15, 2020

Is there any improvements needed to be done in this PR?

@david-allison
Copy link
Member

@infinyte7 This still has merges in the commit history and can't be rebased onto the current master due to conflicts.

image

@krmanik
Copy link
Member Author

krmanik commented Jun 15, 2020

So, should I rebase my github repo?

@krmanik
Copy link
Member Author

krmanik commented Jun 20, 2020

Conflicts are in this PR.

@krmanik krmanik marked this pull request as draft June 20, 2020 15:53
@mikehardy
Copy link
Member

Something about the git commands you are using is not working at all
the sequence is typically git fetch upstream && git rebase upstream/master then resolve conflicts, assuming you have done git remote add upstream [email protected]:ankidroid/Anki-Android.git

There should never be a merge by this projects convention - and we appear to have lost the java changes from AbstractFlashcardViewer.java 😢

@mikehardy
Copy link
Member

Android Studio does a pretty good job of saving "local history" so the files might be retrievable

@krmanik
Copy link
Member Author

krmanik commented Jun 21, 2020

Thank you, I will create a clean separate PR after updating forked repo.

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.

implement AbstractFlashcardViewer / Javascript API versioning
3 participants