i18nResources and graphData for getting data from backend#11559
Conversation
david-allison
left a comment
david-allison
left a comment
There was a problem hiding this comment.
LGTM. Do note that i18n isn't yet handled in Anki-Android-Backend
david-allison
added
the
Needs Second Approval
| fun renderCardForTemplateManager(templateRenderContext: TemplateRenderContext): RenderCardOut | ||
|
|
||
| @Throws(BackendNotSupportedException::class) | ||
| fun i18nResources(): ByteString |
There was a problem hiding this comment.
I'd really appreciate if you can add a documentation. Especially with ByteString output type, the type can't even help figuring out what's going on. And the method name is not really clear.
Even if the the method were documented in Anki-Android-Backend (which I don't understand enough to check), it is still necessary to document the interface if we want documentation to appear in android-studio interface where the method is used.
Ideally, I'd ask same change for other methods, and would try to do the PR myself. But I can't find in the backend code any documentation either, so it's hard to get exactly an idea.
Relatedy, since most other method uses a more specific type, I would love to understand why it does not use a more specific type? Is it because the type is not yet available in BackendProto? If so can you please help me understand if it's something you'll need to add yourself? To wait for an update from anki to appear in backend and then in this repo's dependenencies?
There was a problem hiding this comment.
Bytes are used here because the method is intended for consumption by the frontend typescript code. The Kotlin code just treats it as opaque data and passes it on. If you need to know what those bytes represent, you can look the method up in the .proto file.
IMHO, now is not the best time to be pushing for documentation. The code will need to undergo quite a few changes to get up to speed with the latest Anki, and presumably the DroidBackend interface and separate Java/11/18 implementations will be merged into a single concrete class once everything is up to date, as they add a bunch of extra boilerplate.
| fun i18nResources(): ByteString | ||
|
|
||
| @Throws(BackendNotSupportedException::class) | ||
| fun graphData(search: String, days: Int): ByteString |
There was a problem hiding this comment.
Is it a name copy-pasted from upstream?
Otherwise, I 'd love if you would mind adding "stats" somewhere in the method name. It tooks me a few seconds to understand what graphs are used in AnkiDroid
There was a problem hiding this comment.
It matches the endpoint name the ts code expects. It could be renamed now, but this method gets renamed in later Anki versions anyway. As mentioned above, this is not something you're going to call from AnkiDroid code outside of the ts request handler.
|
|
||
| override fun i18nResources(): ByteString = backend.backend.i18nResources().json | ||
|
|
||
| override fun graphData(search: String, days: Int): ByteString = backend.backend.graphs(search, days).toByteString() |
There was a problem hiding this comment.
Is it something that works today? if we call it, would it return a result? If so, is there a way to test it?
While I'd love test, I don't want to block you, so even just a todo, and potentially an issue explaining how to write those test would be nice
There was a problem hiding this comment.
Yes, it works: krmanik#23 (comment)
Like above, this can be largely treated like a black box - it returns opaque data from the backend, and passes it on to the TypeScript frontend, which is responsible for decoding the protobuf (1) and displaying the graphs.
Because AnkiDroid's only (2) involvement here is transferring the bytes from one desktop component to another, I'm not sure how much value you'd get out of trying to test this inside AnkiDroid.
(1) Technically there's an unnecessary step in the current code. The Rust backend returns encoded protobuf as bytes. @david-allison's backend code is decoding it and returning a protobuf message, which this method receives. It then encodes it to bytes again. A future optimization step would be to return the bytes directly from the backend so that the data is completely opaque to AnkiDroid, and doesn't need to be decoded only to be encoded again.
(2) Technically the follow-up code will also be decoding the JSON request from the frontend into the search and days args. Later desktop versions just pass opaque bytes into the backend instead.
There was a problem hiding this comment.
(the PR mentioned below provides a Raw method)
|
Let's wait for the updated AnkiDroid backend code. |
|
These should be exposed once ankidroid/Anki-Android-Backend#202 is merged I tried to automate the bundling of js/html/css, but ran into troubles - if you have ideas, follow-up PRs welcome :) ankidroid/Anki-Android-Backend@60a4233 |
|
I will implement automation for bundling js/html/css in follow-up PR. |
mikehardy
added
the
Blocked by dependency
|
backend PR is merged and #11644 is about to merge, nothing about the code will change now if you want to base on top of it, the commit order might change by the code may be considered stable |
|
#11644 will close this PR, as it exposes the required methods already. |
|
Oh even better - you can tell I've got blinders on with that one - so focused on getting it in I'm leaving notes in places but it's wide vs deep. Thanks |
mikehardy
removed
Needs Second Approval
5 participants
Pull Request template
Purpose / Description
For porting graph view from Anki to AnkiDroid, it needs have implementation for getting
i18nResourcesandgraphDatadata.Fixes
Fixes Link to the issues.
Approach
Took the idea from krmanik#23 and implemented it.
JavaDroidBackend.javaDroidBackend.ktRustDroidBackend.ktHow Has This Been Tested?
Tested on another branch with the same implementation
Learning (optional, can help others)
krmanik#23
Links to blog posts, patterns, libraries or addons used to solve this problem
Checklist
Please, go through these checks before submitting the PR.
ifstatements)