Feat/Svelte port for reviewer#4289
Conversation
|
Hi Luc, My vision for the reviewer migration is to have the answer buttons handled as part of the reviewer page, rather than a separate webview. While we could potentially migrate the bottom bar separately first and potentially reuse some of it when we later merge the two pages, I imagine it would be more efficient to tackle both at once. WDYT? Due to it being a rather radical change, we're probably going to maintain two separate paths, with users being able to switch between the old and new modes. |
|
I'll try and make the reviewer svelte as well then. Hopefully inheritance can make the toggle-able option easy to implement. No promises on the timescale for this 😅. I thought I was biting off more than I could chew with just the bottom bar. |
|
I am testing 8d3179c
Now, I get a different behavior. Choosing Undo from In this recording, I selected "Bury note" and then pressed Recording1.mp4Long-pressing the Ctrl key also seems to trigger undo. In this recording, I selected "Mark note" and then held the Ctrl key for a second or two and the mark got removed. After this, the undo in the Edit menu was empty but pressing Ctrl + Z caused the mark to reappear and pressing Ctrl + Z again removed the mark. I really can't understand what's going on here. Recording2.mp4
This was fixed. |
ts/routes/reviewer/reviewer.ts
Outdated
| public setUndo(status: string) { | ||
| // For a list of statuses, see | ||
| // https://github.com/ankitects/anki/blob/acdf486b290bd47d13e2e880fbb1c14773899091/rslib/src/ops.rs#L57 | ||
| if (this.undoStatus) { // Skip if "undoStatus" is disabled / not set | ||
| this.undoStatus.undo = status; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm going to wait for input before I fix any more undo bugs because I feel I may have overcomplicated it's implementation.
Should undoState be stored here?
There was a problem hiding this comment.
Looks like we can use #4508. Here's a diff:
Diff
diff --git a/proto/anki/collection.proto b/proto/anki/collection.proto
index 330413613..0ed1a155c 100644
--- a/proto/anki/collection.proto
+++ b/proto/anki/collection.proto
@@ -78,6 +78,10 @@ message OpChangesOnly {
collection.OpChanges changes = 1;
}
+message NestedOpChanges {
+ OpChangesOnly changes = 1;
+}
+
message OpChangesWithCount {
OpChanges changes = 1;
uint32 count = 2;
diff --git a/pylib/anki/collection.py b/pylib/anki/collection.py
index 60360470c..fb50f0691 100644
--- a/pylib/anki/collection.py
+++ b/pylib/anki/collection.py
@@ -35,6 +35,7 @@ Preferences = config_pb2.Preferences
UndoStatus = collection_pb2.UndoStatus
OpChanges = collection_pb2.OpChanges
OpChangesOnly = collection_pb2.OpChangesOnly
+NestedOpChanges = collection_pb2.NestedOpChanges
OpChangesWithCount = collection_pb2.OpChangesWithCount
OpChangesWithId = collection_pb2.OpChangesWithId
OpChangesAfterUndo = collection_pb2.OpChangesAfterUndo
diff --git a/qt/aqt/mediasrv.py b/qt/aqt/mediasrv.py
index c7ebef28b..a1f3f0024 100644
--- a/qt/aqt/mediasrv.py
+++ b/qt/aqt/mediasrv.py
@@ -16,6 +16,7 @@ from collections.abc import Callable
from dataclasses import dataclass
from errno import EPROTOTYPE
from http import HTTPStatus
+from typing import Any
import flask
import flask_cors
@@ -30,6 +31,7 @@ import aqt.operations
from anki import hooks
from anki.cards import Card, CardId
from anki.collection import (
+ NestedOpChanges,
OpChanges,
OpChangesOnly,
Progress,
@@ -880,7 +882,28 @@ def raw_backend_request(endpoint: str) -> Callable[[], bytes]:
assert hasattr(RustBackend, f"{endpoint}_raw")
- return lambda: getattr(aqt.mw.col._backend, f"{endpoint}_raw")(request.data)
+ def wrapped() -> bytes:
+ output = getattr(aqt.mw.col._backend, f"{endpoint}_raw")(request.data)
+ if op_changes_type := int(request.headers.get("Anki-Op-Changes", "0")):
+ op_message_types = (OpChanges, OpChangesOnly, NestedOpChanges)
+ try:
+ response = op_message_types[op_changes_type - 1]()
+ response.ParseFromString(output)
+ changes: Any = response
+ for _ in range(op_changes_type - 1):
+ changes = changes.changes
+ except IndexError:
+ raise ValueError(f"unhandled op changes level: {op_changes_type}")
+
+ def handle_on_main() -> None:
+ handler = aqt.mw.app.activeWindow()
+ on_op_finished(aqt.mw, changes, handler)
+
+ aqt.mw.taskman.run_on_main(handle_on_main)
+
+ return output
+
+ return wrapped
# all methods in here require a collection
diff --git a/qt/aqt/webview.py b/qt/aqt/webview.py
index 464aa130b..c025a0d14 100644
--- a/qt/aqt/webview.py
+++ b/qt/aqt/webview.py
@@ -12,14 +12,16 @@ from collections.abc import Callable, Sequence
from enum import Enum
from typing import TYPE_CHECKING, Any, Type, cast
+from google.protobuf.json_format import MessageToDict
from typing_extensions import TypedDict, Unpack
import anki
import anki.lang
from anki._legacy import deprecated
from anki.lang import is_rtl
-from anki.utils import hmr_mode, is_lin, is_mac, is_win
+from anki.utils import hmr_mode, is_lin, is_mac, is_win, to_json_bytes
from aqt import colors, gui_hooks
+from aqt.operations import OpChanges
from aqt.qt import *
from aqt.qt import sip
from aqt.theme import theme_manager
@@ -383,6 +385,7 @@ class AnkiWebView(QWebEngineView):
self._filterSet = False
gui_hooks.theme_did_change.append(self.on_theme_did_change)
gui_hooks.body_classes_need_update.append(self.on_body_classes_need_update)
+ gui_hooks.operation_did_execute.append(self.on_operation_did_execute)
qconnect(self.loadFinished, self._on_load_finished)
@@ -912,6 +915,7 @@ html {{ {font} }}
gui_hooks.theme_did_change.remove(self.on_theme_did_change)
gui_hooks.body_classes_need_update.remove(self.on_body_classes_need_update)
+ gui_hooks.operation_did_execute.remove(self.on_operation_did_execute)
# defer page cleanup so that in-flight requests have a chance to complete first
# https://forums.ankiweb.net/t/error-when-exiting-browsing-when-the-software-is-installed-in-the-path-c-program-files-anki/38363
mw.progress.single_shot(5000, lambda: mw.mediaServer.clear_page_html(id(self)))
@@ -953,6 +957,17 @@ html {{ {font} }}
f"""document.body.classList.toggle("reduce-motion", {json.dumps(mw.pm.reduce_motion())}); """
)
+ def on_operation_did_execute(
+ self, changes: OpChanges, handler: object | None
+ ) -> None:
+ if handler is self.parentWidget():
+ return
+
+ changes_json = to_json_bytes(MessageToDict(changes)).decode()
+ self.eval(
+ f"if(globalThis.anki && globalThis.anki.onOperationDidExecute) globalThis.anki.onOperationDidExecute({changes_json})"
+ )
+
@deprecated(info="use theme_manager.qcolor() instead")
def get_window_bg_color(self, night_mode: bool | None = None) -> QColor:
return theme_manager.qcolor(colors.CANVAS)
diff --git a/rslib/proto/typescript.rs b/rslib/proto/typescript.rs
index 4e941a0ca..3b70efdfc 100644
--- a/rslib/proto/typescript.rs
+++ b/rslib/proto/typescript.rs
@@ -12,6 +12,7 @@ use anki_proto_gen::Method;
use anyhow::Result;
use inflections::Inflect;
use itertools::Itertools;
+use prost_reflect::MessageDescriptor;
pub(crate) fn write_ts_interface(services: &[BackendService]) -> Result<()> {
let root = Path::new("../../out/ts/lib/generated");
@@ -73,14 +74,16 @@ fn write_ts_method(
input_type,
output_type,
comments,
+ op_changes_type,
}: &MethodDetails,
out: &mut String,
) {
+ let op_changes_type = *op_changes_type as u8;
let comments = format_comments(comments);
writeln!(
out,
r#"{comments}export async function {method_name}(input: PlainMessage<{input_type}>, options?: PostProtoOptions): Promise<{output_type}> {{
- return await postProto("{method_name}", new {input_type}(input), {output_type}, options);
+ return await postProto("{method_name}", new {input_type}(input), {output_type}, options, {op_changes_type});
}}"#
).unwrap()
}
@@ -92,11 +95,21 @@ fn format_comments(comments: &Option<String>) -> String {
.unwrap_or_default()
}
+#[derive(Clone, Copy)]
+#[repr(u8)]
+enum OpChangesType {
+ None = 0,
+ OpChanges = 1,
+ OpChangesOnly = 2,
+ NestedOpChanges = 3,
+}
+
struct MethodDetails {
method_name: String,
input_type: String,
output_type: String,
comments: Option<String>,
+ op_changes_type: OpChangesType,
}
impl MethodDetails {
@@ -105,12 +118,43 @@ impl MethodDetails {
let input_type = full_name_to_imported_reference(method.proto.input().full_name());
let output_type = full_name_to_imported_reference(method.proto.output().full_name());
let comments = method.comments.clone();
+ let op_changes_type =
+ get_op_changes_type(&method.proto.output(), &method.proto.output(), 1);
Self {
method_name: name,
input_type,
output_type,
comments,
+ op_changes_type,
+ }
+ }
+}
+
+fn get_op_changes_type(
+ root_message: &MessageDescriptor,
+ message: &MessageDescriptor,
+ level: u8,
+) -> OpChangesType {
+ if message.full_name() == "anki.collection.OpChanges" {
+ match level {
+ 0 => OpChangesType::None,
+ 1 => OpChangesType::OpChanges,
+ 2 => OpChangesType::OpChangesOnly,
+ 3 => OpChangesType::NestedOpChanges,
+ _ => panic!(
+ "unhandled op changes level for message {}: {}",
+ root_message.full_name(),
+ level
+ ),
+ }
+ } else if let Some(field) = message.get_field(1) {
+ if let Some(field_message) = field.kind().as_message() {
+ get_op_changes_type(root_message, field_message, level + 1)
+ } else {
+ OpChangesType::None
}
+ } else {
+ OpChangesType::None
}
}
diff --git a/ts/lib/generated/post.ts b/ts/lib/generated/post.ts
index 90e372520..c70b789e6 100644
--- a/ts/lib/generated/post.ts
+++ b/ts/lib/generated/post.ts
@@ -11,11 +11,12 @@ export async function postProto<T>(
input: { toBinary(): Uint8Array; getType(): { typeName: string } },
outputType: { fromBinary(arr: Uint8Array): T },
options: PostProtoOptions = {},
+ opChangesType = 0,
): Promise<T> {
try {
const inputBytes = input.toBinary();
const path = `/_anki/${method}`;
- const outputBytes = await postProtoInner(path, inputBytes);
+ const outputBytes = await postProtoInner(path, inputBytes, opChangesType);
return outputType.fromBinary(outputBytes);
} catch (err) {
const { alertOnError = true } = options;
@@ -26,11 +27,12 @@ export async function postProto<T>(
}
}
-async function postProtoInner(url: string, body: Uint8Array): Promise<Uint8Array> {
+async function postProtoInner(url: string, body: Uint8Array, opChangesType: number): Promise<Uint8Array> {
const result = await fetch(url, {
method: "POST",
headers: {
"Content-Type": "application/binary",
+ "Anki-Op-Changes": opChangesType.toString(),
},
body,
});
diff --git a/ts/lib/tslib/operations.ts b/ts/lib/tslib/operations.ts
new file mode 100644
index 000000000..f7244e8a9
--- /dev/null
+++ b/ts/lib/tslib/operations.ts
@@ -0,0 +1,20 @@
+// Copyright: Ankitects Pty Ltd and contributors
+// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
+
+import type { OpChanges } from "@generated/anki/collection_pb";
+
+type OperationHandler = (changes: Partial<OpChanges>) => void;
+const handlers: OperationHandler[] = [];
+
+export function registerOperationHandler(handler: (changes: Partial<OpChanges>) => void): void {
+ handlers.push(handler);
+}
+
+function onOperationDidExecute(changes: Partial<OpChanges>): void {
+ for (const handler of handlers) {
+ handler(changes);
+ }
+}
+
+globalThis.anki = globalThis.anki || {};
+globalThis.anki.onOperationDidExecute = onOperationDidExecute;
diff --git a/ts/routes/reviewer/+page.svelte b/ts/routes/reviewer/+page.svelte
index 31ab196dc..214c346b3 100644
--- a/ts/routes/reviewer/+page.svelte
+++ b/ts/routes/reviewer/+page.svelte
@@ -10,17 +10,15 @@ License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
import ReviewerBottom from "./reviewer-bottom/ReviewerBottom.svelte";
import Reviewer from "./Reviewer.svelte";
import { _blockDefaultDragDropBehavior } from "../../reviewer";
- import type { PageData } from "./$types";
+ import { registerOperationHandler } from "../../lib/tslib/operations";
- export let data: PageData;
const state = new ReviewerState();
onMount(() => {
updateNightMode();
globalThis.anki ??= {};
- globalThis.anki.changeReceived = () => state.showQuestion(null);
+ registerOperationHandler(() => state.showQuestion(null));
_blockDefaultDragDropBehavior();
- state.undoStatus = data.initialUndoStatus;
});
</script>
diff --git a/ts/routes/reviewer/+page.ts b/ts/routes/reviewer/+page.ts
deleted file mode 100644
index fc5d7d672..000000000
--- a/ts/routes/reviewer/+page.ts
+++ /dev/null
@@ -1,9 +0,0 @@
-// Copyright: Ankitects Pty Ltd and contributors
-// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
-import { getUndoStatus } from "@generated/backend";
-import type { PageLoad } from "./$types";
-
-export const load = (async () => {
- const initialUndoStatus = await getUndoStatus({});
- return { initialUndoStatus };
-}) satisfies PageLoad;
diff --git a/ts/routes/reviewer/reviewer.ts b/ts/routes/reviewer/reviewer.ts
index ba9d13f5f..58b94d434 100644
--- a/ts/routes/reviewer/reviewer.ts
+++ b/ts/routes/reviewer/reviewer.ts
@@ -1,7 +1,6 @@
// Copyright: Ankitects Pty Ltd and contributors
// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
import type { AVTag } from "@generated/anki/card_rendering_pb";
-import type { UndoStatus } from "@generated/anki/collection_pb";
import { DeckConfig_Config_AnswerAction, DeckConfig_Config_QuestionAction } from "@generated/anki/deck_config_pb";
import { ReviewerActionRequest_ReviewerAction } from "@generated/anki/frontend_pb";
import {
@@ -16,13 +15,11 @@ import {
getConfigJson,
nextCardData,
playAvtags,
- redo,
removeNotes,
removeNoteTags,
reviewerAction,
setConfigJson,
setFlag,
- undo,
} from "@generated/backend";
import * as tr from "@generated/ftl";
import { derived, get, writable } from "svelte/store";
@@ -67,7 +64,6 @@ export class ReviewerState {
readonly flag = writable(0);
readonly marked = writable(false);
readonly autoAdvance = writable(false);
- undoStatus: UndoStatus | undefined = undefined;
autoAdvanceQuestionTimeout: ReturnType<typeof setTimeout> | undefined;
autoAdvanceAnswerTimeout: ReturnType<typeof setTimeout> | undefined;
_answerShown = false;
@@ -224,10 +220,8 @@ export class ReviewerState {
const noteIds = [this.currentCard.card.noteId];
if (this._cardData.marked) {
await removeNoteTags({ noteIds, tags: "marked" });
- this.setUndo(tr.actionsRemoveTag());
} else {
await addNoteTags({ noteIds, tags: "marked" });
- this.setUndo(tr.actionsUpdateTag());
}
this.marked.update($marked => !$marked);
this._cardData.marked = !this._cardData.marked;
@@ -253,18 +247,6 @@ export class ReviewerState {
}, TOOLTIP_TIMEOUT_MS);
}
- public setUndo(status: string) {
- // For a list of statuses, see
- // https://github.com/ankitects/anki/blob/acdf486b290bd47d13e2e880fbb1c14773899091/rslib/src/ops.rs#L57
- if (this.undoStatus) { // Skip if "undoStatus" is disabled / not set
- this.undoStatus.undo = status;
- }
- }
-
- public setBuryOrSuspendUndo(suspend: boolean) {
- this.setUndo(suspend ? tr.studyingSuspend() : tr.studyingBury());
- }
-
public async buryOrSuspendCurrentCard(suspend: boolean) {
const mode = suspend ? BuryOrSuspendCardsRequest_Mode.SUSPEND : BuryOrSuspendCardsRequest_Mode.BURY_USER;
if (this.currentCard?.card?.id) {
@@ -274,7 +256,6 @@ export class ReviewerState {
mode,
});
this.showTooltip(suspend ? tr.studyingCardSuspended() : tr.studyingCardsBuried({ count: 1 }));
- this.setBuryOrSuspendUndo(suspend);
this.refresh();
}
}
@@ -288,7 +269,6 @@ export class ReviewerState {
mode,
});
this.showTooltip(suspend ? tr.studyingNoteSuspended() : tr.studyingCardsBuried({ count: op.count }));
- this.setBuryOrSuspendUndo(suspend);
this.refresh();
}
}
@@ -297,12 +277,11 @@ export class ReviewerState {
if (this.currentCard?.card?.noteId) {
const op = await removeNotes({ noteIds: [this.currentCard.card.noteId], cardIds: [] });
this.showTooltip(tr.browsingCardsDeleted({ count: op.count }));
- this.setUndo(tr.studyingDeleteNote());
this.refresh();
}
}
- async handleKeyPress(key: string, ctrl: boolean, shift: boolean) {
+ async handleKeyPress(key: string, ctrl: boolean, _shift: boolean) {
key = key.toLowerCase();
switch (key) {
case "1": {
@@ -330,23 +309,6 @@ export class ReviewerState {
}
break;
}
- case "z": {
- if (ctrl) {
- if (shift && this.undoStatus?.redo) {
- const op = await redo({});
- this.showTooltip(tr.undoActionRedone({ action: op.operation }));
- this.undoStatus = op.newStatus;
- } else if (this.undoStatus?.undo) {
- const op = await undo({});
- this.showTooltip(tr.undoActionUndone({ action: op.operation }));
- this.undoStatus = op.newStatus;
- } else {
- this.showTooltip(shift ? tr.actionsNothingToRedo() : tr.actionsNothingToUndo());
- }
- this.refresh();
- }
- break;
- }
case "e": {
if (!ctrl) {
this.displayEditMenu();
@@ -420,10 +382,6 @@ export class ReviewerState {
}
async showQuestion(answer: CardAnswer | null) {
- if (answer !== null) {
- this.setUndo(tr.actionsAnswerCard());
- }
-
this._answerShown = false;
const resp = await nextCardData({
answer: answer || undefined,
There was a problem hiding this comment.
After applying the diff to 4363fb4 (not the current head), it seems like on cards where localstorage is set the reviewer is constantly refreshed and becomes unresponsive.
Example Card
Front Template
{{Front}}
<script>
window.localStorage.setItem("test", "{{Front}}")
</script>Back Template
{{Front}}
<hr id=answer>
{{Back}}
<script>
console.log(window.localStorage.getItem("test", "{{Front}}"))
</script>Styling
.card {
font-family: arial;
font-size: 20px;
line-height: 1.5;
text-align: center;
color: black;
background-color: white;
}If you would prefer me to dig into this myself I will.
There was a problem hiding this comment.
This is caused by the state.showQuestion(null) call inside the operation handler. The logic in Reviewer.op_executed needs to be ported. I guess we have to ignore the focused param for now.
There was a problem hiding this comment.
I have fixed it by continuing to use
globalThis.anki.changeReceived = () => state.showQuestion(null);in ts/routes/reviewer/+page.svelte.
I think porting Reviewer.op_executed is the plan. This is what Dae said about the refresh code before: #4289 (comment)
The Python/Kotlin/Swift layers currently take care of tracking changes, so the simplest/most pragmatic solution for now is probably to have the Python code inject messages into the outer frame when changes are received (e.g. web.eval(“anki.changeReceived(…)”), like you were thinking of. We’ll presumably want to build a similar change manager into the TypeScript layer, which could potentially use addEventListener. In the future, it should be possible to switch from the JS injection into polling or web socket push, without having to change how TS consumers process the change messages.
GithubAnon0000
left a comment
There was a problem hiding this comment.
With 11e111e the "flag card" menu doesn't open on hover but needs a click instead (old reviewer works with hover).
I think hover is more conventional than click for such a submenu. Would be nice if it could be adjusted. Maybe allow both, in case mobile clients rely on a "click" / touch.
ts/routes/reviewer/reviewer.ts
Outdated
| import type { InnerReviewerRequest } from "../reviewer-inner/innerReviewerRequest"; | ||
| import type { ReviewerRequest } from "./reviewerRequest"; | ||
|
|
||
| export function isNightMode() { |
There was a problem hiding this comment.
Is this needed? Other Svelte pages use nightmode.ts and theme.ts
There was a problem hiding this comment.
I think this is separate because of the code that deals with detecting night mode in browsers (outside of Anki).
We should be able to load the Svelte page in a browser, and have it function properly without any Python code running (aside from mediasrv.py).
Maybe that check should be included with nightmode.ts and theme.ts instead of as a separate function?
There was a problem hiding this comment.
Yes, I think we should move the media query check to checkNightMode() for consistency: If the URL contains #night or prefers-color-scheme: dark is active, we add dark mode classes.
|
|
||
| function checkKey(event: KeyboardEvent, key: number): boolean { | ||
| // avoid deprecation warning | ||
| const which = event["which" + ""]; |
There was a problem hiding this comment.
Was this changed to fix some issue? See #1183 for the reason which is used.
There was a problem hiding this comment.
I think the issue was that the modifier keys had no effect on the key-code, for example * would have to have shortcuts for both Shift+8 and also the button above the number-pad. This might not have been why, it has been a while.
There was a problem hiding this comment.
One issue I noticed with the current approach is that shortcuts such as "R" no longer work in non-latin layouts.
Keyboard layouts are a mess 😩 This is probably better discussed and handled in a separate topic. I'll look into creating an issue.
I guess enabling async support for Flask would help us implement this cleanly. This is what I've done to implement things such as getting a path from the Qt file picker: Line 680 in 98a721c I'm thinking of extracting this into a separate PR (along with undo handling) for easier review. |
|
I noticed external links in cards open inside the iframe. Removing the check for |
|
Could an option be to intercept any clicks on |
|
This would be tricky to make reliable though? In addition to the different scenarios we need to handle (e.g., The acceptNavigationRequest() solution on the other hand is simple and reliable: We can add a variable similar to |
--------- Co-authored-by: Abdo <abdo@abdnh.net>
39e73f3 to
63c4bc1
Compare
Probably keep mutateNextCardStates for the old reviewer, and reimplement it for the new reviewer entirely in reviewer.ts? config can be loaded using getConfigString() end evaluated.
It would be great if we can figure out a way to keep important hooks working. Better discussed in a separate issue probably. 1 I'll create an issue. |
As I've implemented it (cd6184e) This change completely breaks iframes because the Iframe page load itself triggers the browser to load. If I've implemented it how you intended me to, then maybe its best to revert it. Edit: On second looking Iframes seem to be broken even after reverting that commit :/ (testing with a YouTube embed) although without that commit they do not open in the browser. Edit2: It seems that the Iframe works if we use |


Note
While testing this, make sure to enable "Use new reviewer" in the preferences:

I'm not sure if I have the right idea here. If I don't please tell me so I can give up and leave it to the people who probably should be left to do this.
I'm not sure how faithful to the original html I need to be. If I should use the old table methods, for addon support reasons perhaps, let me know.
Reviewer
./tools/runoptnext_card_datatakes a long time.NextCardDataprotobuf message (Maybe group some fields into separate messages?)Note
I'll repeatedly edit this comment with any queries I have about how things should be implemented. For the time being.