Implement full canAddNotes capabilities, add notesInfo and canAddNotesWithErrorDetail API #60
Conversation
This implementation is kinda hacky. canAddNotes in Ankiconnect-Android just checks for duplicate cards, so I pass the result of that check through and add the string yomitan is checking for so that it will mark the card as a duplicate.
drakargx
changed the title
app/src/main/java/com/kamwithk/ankiconnectandroid/ankidroid_api/IntegratedAPI.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/ankidroid_api/IntegratedAPI.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/ankidroid_api/IntegratedAPI.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/ankidroid_api/IntegratedAPI.java
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/ankidroid_api/IntegratedAPI.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/ankidroid_api/IntegratedAPI.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/ankidroid_api/IntegratedAPI.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/ankidroid_api/NoteAPI.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/ankidroid_api/Utility.java
Outdated
Show resolved
Hide resolved
app/src/main/java/com/kamwithk/ankiconnectandroid/request_parsers/NoteRequest.java
Outdated
Show resolved
Hide resolved
Logic for checking if a card is duplicate if scope is set to "deck" or "deck root" wasn't correct after refactor
There was a problem hiding this comment.
Hi @drakargx, thanks for the work on this and for letting me comment here. This is just for an issue I noticed when I tested the changes with asbplayer. notesInfo isn't returning anything so asbplayer fails to update the last card.
Also this is unrelated to your PR but I think it might also just be a minor fix. When an unsupported API is called, default_version() returns a String ("AnkiConnect v.6") and a MalformedJsonException is thrown: killergerbah/asbplayer#408. Since asbplayer also calls addTags, the issue will continue to happen even after notesInfo is added. Is it okay if the response is wrapped like this:
private String default_version() {
return Parser.gson.toJson("AnkiConnect v.6");
}
The tags won't be added but at the very least, it stops the exception from being thrown.
| } | ||
|
|
||
| try (cursor) { | ||
| while (!cursor.moveToNext()) { |
There was a problem hiding this comment.
Was this supposed to be while(cursor.moveToNext())? It seems like it's ignoring the results of the query right now
There was a problem hiding this comment.
Was this supposed to be while(cursor.moveToNext())? It seems like it's ignoring the results of the query right now
Thanks for pointing that out, you are right.
Also this is unrelated to your PR but I think it might also just be a minor fix. When an unsupported API is called, default_version() returns a String ("AnkiConnect v.6") and a MalformedJsonException is thrown: killergerbah/asbplayer#408. Since asbplayer also calls addTags, the issue will continue to happen even after notesInfo is added. Is it okay if the response is wrapped like this:
I think for this it would be better if instead of calling default_version() here we just threw an Exception, that way we will respond like the desktop AnkiConnect with a json object of {"result": null, "error":"unsupported action"}.
Will test the change in asbplayer to make sure it works before updating PR
There was a problem hiding this comment.
@naiza-asaad after @drakargx finishes up if you're able to please test everything and double check it works
After that I'm happy to merge :)
There was a problem hiding this comment.
Sure. Myself and others have been using this branch for some time now since it fixed the duplicate checking bug. Probably only the comment about fixing the result of notesInfo() should be changed before merging.
The other one about changing the result of default_version() should probably be in a different PR and not block this since it's technically unrelated.
There was a problem hiding this comment.
Sorry, got distracted with work & other things...
I agree with @naiza-asaad on leaving to fix default_version() on a different PR. notesInfo logic is fixed in most recent commit.
A few people in TMW have been running the build with these changes for some time now and we haven't heard of any issues, so I think it should be good.
Based on #46 this implements just the functionality of notesInfo and canAddNotesWithErrorDetail, at least as much for yomitan to work.
themoeway/yomitan#693 was recently pushed into yomitan's main release branch which prevents yomitan from displaying the add button when duplicate checking is enabled, as it relies on the canAddNotesWithErrorDetail API.
canAddNotesWithErrorDetail returns an array of JSON objects with a 'canAdd' field and an optional 'error' field. If canAdd is true then 'error' is not present. Yomitan is checking for the presence of the 'error' field so I also had to create another GSON instance that doesn't serialize nulls; if there's a better way let me know (this is the first time I've used Java in years).
canAddNotes is currently implemented by just checking if the card to add is a duplicate, so right now I pass the result of that over and if the result is false, I add the string Yomitan is checking for. If a future feature of Yomitan relies on the contents of the error message then this needs to be revisited.
I am also not sure how to credit @Aquafina-water-bottle for the work in implementing the notesInfo API, I simply added some performance improvements by caching model information.