Skip to content

i18nResources and graphData for getting data from backend #11559

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.

Sign up for GitHub

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

Closed
krmanik wants to merge 1 commit into from
+27 −0
Closed

i18nResources and graphData for getting data from backend #11559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions AnkiDroid/src/main/java/com/ichi2/libanki/backend/DroidBackend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import BackendProto.Backend.ExtractAVTagsOut
import BackendProto.Backend.RenderCardOut
import android.content.Context
import androidx.annotation.VisibleForTesting
import com.google.protobuf.ByteString
import com.ichi2.libanki.Collection
import com.ichi2.libanki.DB
import com.ichi2.libanki.DeckConfig
Expand Down Expand Up @@ -85,4 +86,10 @@ interface DroidBackend {

@Throws(BackendNotSupportedException::class)
fun renderCardForTemplateManager(templateRenderContext: TemplateRenderContext): RenderCardOut

@Throws(BackendNotSupportedException::class)
fun i18nResources(): ByteString

@Arthur-Milchior Arthur-Milchior Jun 6, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@dae dae Jun 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.


@Throws(BackendNotSupportedException::class)
fun graphData(search: String, days: Int): ByteString

@Arthur-Milchior Arthur-Milchior Jun 6, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@dae dae Jun 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

}
15 changes: 15 additions & 0 deletions AnkiDroid/src/main/java/com/ichi2/libanki/backend/JavaDroidBackend.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import android.content.Context;

import com.google.protobuf.ByteString;
import com.ichi2.libanki.Collection;
import com.ichi2.libanki.DB;
import com.ichi2.libanki.TemplateManager;
Expand Down Expand Up @@ -108,4 +109,18 @@ public void useNewTimezoneCode(Collection col) {
public @NonNull Backend.RenderCardOut renderCardForTemplateManager(@NonNull TemplateManager.TemplateRenderContext templateRenderContext) throws BackendNotSupportedException {
throw new BackendNotSupportedException();
}


@NonNull
@Override
public ByteString i18nResources() throws BackendNotSupportedException {
throw new BackendNotSupportedException();
}


@NonNull
@Override
public ByteString graphData(@NonNull String search, int days) throws BackendNotSupportedException {
throw new BackendNotSupportedException();
}
}
5 changes: 5 additions & 0 deletions AnkiDroid/src/main/java/com/ichi2/libanki/backend/RustDroidBackend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package com.ichi2.libanki.backend
import BackendProto.Backend.ExtractAVTagsOut
import BackendProto.Backend.RenderCardOut
import android.content.Context
import com.google.protobuf.ByteString
import com.ichi2.libanki.Collection
import com.ichi2.libanki.DB
import com.ichi2.libanki.TemplateManager.TemplateRenderContext
Expand Down Expand Up @@ -95,6 +96,10 @@ open class RustDroidBackend(
throw BackendNotSupportedException()
}

override fun i18nResources(): ByteString = backend.backend.i18nResources().json

override fun graphData(search: String, days: Int): ByteString = backend.backend.graphs(search, days).toByteString()

@Arthur-Milchior Arthur-Milchior Jun 6, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@dae dae Jun 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@dae dae Jun 24, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(the PR mentioned below provides a Raw method)


companion object {
const val UNUSED_VALUE = 0
}
Expand Down