make flag changes in the reviewer undoable

This splits update_card() into separate undoable/non-undoable ops
like the change to notes in b4396b94abdeba3347d30025c5c0240d991006c9

It means that actions get a blanket 'Update Card' description - in the
future we'll probably want to either add specific actions to the backend,
or allow an enum or string to be passed in to describe the op.

Other changes:
- card.flush() can no longer be used to add new cards. Card creation
is only supposed to be done in response to changes in a note's fields,
and this functionality was only exposed because the card generation
hadn't been migrated to the backend at that point. As far as I'm aware,
only Arthur's "copy notes" add-on used this functionality, and that should
be an easy fix - when the new note is added, the associated cards will
be generated, and they can then be retrieved with note.cards()
- tidy ups/PEP8
This commit is contained in:
Damien Elmes 2021-03-07 00:17:17 +10:00
parent d4765a301f
commit 3d0ddc8539
20 changed files with 166 additions and 129 deletions

View File

@ -14,3 +14,4 @@ undo-unbury-unsuspend = Unbury/Unsuspend
undo-add-note = Add Note
undo-update-tag = Update Tag
undo-update-note = Update Note
undo-update-card = Update Card

View File

@ -74,19 +74,9 @@ class Card:
self.flags = c.flags
self.data = c.data
def _bugcheck(self) -> None:
if (
self.queue == QUEUE_TYPE_REV
and self.odue
and not self.col.decks.isDyn(self.did)
):
hooks.card_odue_was_invalid()
def flush(self) -> None:
self._bugcheck()
hooks.card_will_flush(self)
def _to_backend_card(self) -> _pb.Card:
# mtime & usn are set by backend
card = _pb.Card(
return _pb.Card(
id=self.id,
note_id=self.nid,
deck_id=self.did,
@ -104,10 +94,15 @@ class Card:
flags=self.flags,
data=self.data,
)
def flush(self) -> None:
hooks.card_will_flush(self)
if self.id != 0:
self.col._backend.update_card(card)
self.col._backend.update_card(
card=self._to_backend_card(), skip_undo_entry=True
)
else:
self.id = self.col._backend.add_card(card)
raise Exception("card.flush() expects an existing card")
def question(self, reload: bool = False, browser: bool = False) -> str:
return self.render_output(reload, browser).question_and_style()
@ -197,9 +192,14 @@ class Card:
del d["timerStarted"]
return f"{super().__repr__()} {pprint.pformat(d, width=300)}"
def userFlag(self) -> int:
def user_flag(self) -> int:
return self.flags & 0b111
def setUserFlag(self, flag: int) -> None:
def set_user_flag(self, flag: int) -> None:
assert 0 <= flag <= 7
self.flags = (self.flags & ~0b111) | flag
# legacy
userFlag = user_flag
setUserFlag = set_user_flag

View File

@ -319,17 +319,23 @@ class Collection:
# Object creation helpers
##########################################################################
def getCard(self, id: int) -> Card:
def get_card(self, id: int) -> Card:
return Card(self, id)
def update_card(self, card: Card) -> None:
"""Save card changes to database, and add an undo entry.
Unlike card.flush(), this will invalidate any current checkpoint."""
self._backend.update_card(card=card._to_backend_card(), skip_undo_entry=False)
def get_note(self, id: int) -> Note:
return Note(self, id=id)
def update_note(self, note: Note) -> None:
"""Save note changes to database, and add an undo entry.
Unlike note.flush(), this will invalidate any current checkpoint."""
self._backend.update_note(note=note.to_backend_note(), skip_undo_entry=False)
self._backend.update_note(note=note._to_backend_note(), skip_undo_entry=False)
getCard = get_card
getNote = get_note
# Utils
@ -367,7 +373,7 @@ class Collection:
return Note(self, self.models.current(forDeck))
def add_note(self, note: Note, deck_id: int) -> None:
note.id = self._backend.add_note(note=note.to_backend_note(), deck_id=deck_id)
note.id = self._backend.add_note(note=note._to_backend_note(), deck_id=deck_id)
def remove_notes(self, note_ids: Sequence[int]) -> None:
hooks.notes_will_be_deleted(self, note_ids)
@ -953,7 +959,7 @@ table.review-log {{ {revlog_style} }}
# Card Flags
##########################################################################
def setUserFlag(self, flag: int, cids: List[int]) -> None:
def set_user_flag_for_cards(self, flag: int, cids: List[int]) -> None:
assert 0 <= flag <= 7
self.db.execute(
"update cards set flags = (flags & ~?) | ?, usn=?, mod=? where id in %s"

View File

@ -53,7 +53,7 @@ class Note:
self.fields = list(n.fields)
self._fmap = self.col.models.fieldMap(self.model())
def to_backend_note(self) -> _pb.Note:
def _to_backend_note(self) -> _pb.Note:
hooks.note_will_flush(self)
return _pb.Note(
id=self.id,
@ -69,7 +69,9 @@ class Note:
"""This preserves any current checkpoint.
For an undo entry, use col.update_note() instead."""
assert self.id != 0
self.col._backend.update_note(note=self.to_backend_note(), skip_undo_entry=True)
self.col._backend.update_note(
note=self._to_backend_note(), skip_undo_entry=True
)
def __repr__(self) -> str:
d = dict(self.__dict__)
@ -124,7 +126,7 @@ class Note:
_model = property(model)
def cloze_numbers_in_fields(self) -> Sequence[int]:
return self.col._backend.cloze_numbers_in_note(self.to_backend_note())
return self.col._backend.cloze_numbers_in_note(self._to_backend_note())
# Dict interface
##################################################
@ -187,5 +189,5 @@ class Note:
def dupeOrEmpty(self) -> int:
"1 if first is empty; 2 if first is a duplicate, 0 otherwise."
return self.col._backend.note_is_duplicate_or_empty(
self.to_backend_note()
self._to_backend_note()
).state

View File

@ -178,7 +178,7 @@ class TemplateRenderContext:
fields["Card"] = self._template["name"]
else:
fields["Card"] = ""
flag = self._card.userFlag()
flag = self._card.user_flag()
fields["CardFlag"] = flag and f"flag{flag}" or ""
self._fields = fields
@ -239,7 +239,7 @@ class TemplateRenderContext:
if self._template:
# card layout screen
out = self._col._backend.render_uncommitted_card(
note=self._note.to_backend_note(),
note=self._note._to_backend_note(),
card_ord=self._card.ord,
template=to_json_bytes(self._template),
fill_empty=self._fill_empty,

View File

@ -13,30 +13,30 @@ def test_flags():
c.flags = origBits
c.flush()
# no flags to start with
assert c.userFlag() == 0
assert c.user_flag() == 0
assert len(col.findCards("flag:0")) == 1
assert len(col.findCards("flag:1")) == 0
# set flag 2
col.setUserFlag(2, [c.id])
col.set_user_flag_for_cards(2, [c.id])
c.load()
assert c.userFlag() == 2
assert c.user_flag() == 2
assert c.flags & origBits == origBits
assert len(col.findCards("flag:0")) == 0
assert len(col.findCards("flag:2")) == 1
assert len(col.findCards("flag:3")) == 0
# change to 3
col.setUserFlag(3, [c.id])
col.set_user_flag_for_cards(3, [c.id])
c.load()
assert c.userFlag() == 3
assert c.user_flag() == 3
# unset
col.setUserFlag(0, [c.id])
col.set_user_flag_for_cards(0, [c.id])
c.load()
assert c.userFlag() == 0
assert c.user_flag() == 0
# should work with Cards method as well
c.setUserFlag(2)
assert c.userFlag() == 2
c.setUserFlag(3)
assert c.userFlag() == 3
c.setUserFlag(0)
assert c.userFlag() == 0
c.set_user_flag(2)
assert c.user_flag() == 2
c.set_user_flag(3)
assert c.user_flag() == 3
c.set_user_flag(0)
assert c.user_flag() == 0

View File

@ -559,6 +559,8 @@ def test_suspend():
def test_cram():
col = getEmptyCol()
opt = col.models.byName("Basic (and reversed card)")
col.models.setCurrent(opt)
note = col.newNote()
note["Front"] = "one"
col.addNote(note)
@ -654,10 +656,9 @@ def test_cram():
c.load()
assert col.sched.answerButtons(c) == 4
# add a sibling so we can test minSpace, etc
c.col = None
c2 = copy.deepcopy(c)
c2.col = c.col = col
c2.id = 0
note["Back"] = "foo"
note.flush()
c2 = note.cards()[1]
c2.ord = 1
c2.due = 325
c2.flush()

View File

@ -411,8 +411,8 @@ class StatusDelegate(QItemDelegate):
option.direction = Qt.RightToLeft
col = None
if c.userFlag() > 0:
col = getattr(colors, f"FLAG{c.userFlag()}_BG")
if c.user_flag() > 0:
col = getattr(colors, f"FLAG{c.user_flag()}_BG")
elif c.note().has_tag("Marked"):
col = colors.MARKED_BG
elif c.queue == QUEUE_TYPE_SUSPENDED:
@ -1286,13 +1286,13 @@ where id in %s"""
def _on_set_flag(self, n: int) -> None:
# flag needs toggling off?
if n == self.card.userFlag():
if n == self.card.user_flag():
n = 0
self.col.setUserFlag(n, self.selectedCards())
self.col.set_user_flag_for_cards(n, self.selectedCards())
self.model.reset()
def _updateFlagsMenu(self) -> None:
flag = self.card and self.card.userFlag()
flag = self.card and self.card.user_flag()
flag = flag or 0
f = self.form

View File

@ -216,7 +216,7 @@ class Reviewer:
return card.autoplay()
def _update_flag_icon(self) -> None:
self.web.eval(f"_drawFlag({self.card.userFlag()});")
self.web.eval(f"_drawFlag({self.card.user_flag()});")
def _update_mark_icon(self) -> None:
self.web.eval(f"_drawMark({json.dumps(self.card.note().has_tag('marked'))});")
@ -290,10 +290,10 @@ class Reviewer:
("m", self.showContextMenu),
("r", self.replayAudio),
(Qt.Key_F5, self.replayAudio),
("Ctrl+1", lambda: self.setFlag(1)),
("Ctrl+2", lambda: self.setFlag(2)),
("Ctrl+3", lambda: self.setFlag(3)),
("Ctrl+4", lambda: self.setFlag(4)),
("Ctrl+1", lambda: self.set_flag_on_current_card(1)),
("Ctrl+2", lambda: self.set_flag_on_current_card(2)),
("Ctrl+3", lambda: self.set_flag_on_current_card(3)),
("Ctrl+4", lambda: self.set_flag_on_current_card(4)),
("*", self.toggle_mark_on_current_note),
("=", self.bury_current_note),
("-", self.bury_current_card),
@ -698,7 +698,7 @@ time = %(time)d;
# note the shortcuts listed here also need to be defined above
def _contextMenu(self) -> List[Any]:
currentFlag = self.card and self.card.userFlag()
currentFlag = self.card and self.card.user_flag()
opts = [
[
tr(TR.STUDYING_FLAG_CARD),
@ -706,25 +706,25 @@ time = %(time)d;
[
tr(TR.ACTIONS_RED_FLAG),
"Ctrl+1",
lambda: self.setFlag(1),
lambda: self.set_flag_on_current_card(1),
dict(checked=currentFlag == 1),
],
[
tr(TR.ACTIONS_ORANGE_FLAG),
"Ctrl+2",
lambda: self.setFlag(2),
lambda: self.set_flag_on_current_card(2),
dict(checked=currentFlag == 2),
],
[
tr(TR.ACTIONS_GREEN_FLAG),
"Ctrl+3",
lambda: self.setFlag(3),
lambda: self.set_flag_on_current_card(3),
dict(checked=currentFlag == 3),
],
[
tr(TR.ACTIONS_BLUE_FLAG),
"Ctrl+4",
lambda: self.setFlag(4),
lambda: self.set_flag_on_current_card(4),
dict(checked=currentFlag == 4),
],
],
@ -782,12 +782,13 @@ time = %(time)d;
def onOptions(self) -> None:
self.mw.onDeckConf(self.mw.col.decks.get(self.card.odid or self.card.did))
def setFlag(self, flag: int) -> None:
def set_flag_on_current_card(self, flag: int) -> None:
# need to toggle off?
if self.card.userFlag() == flag:
if self.card.user_flag() == flag:
flag = 0
self.card.setUserFlag(flag)
self.card.flush()
self.card.set_user_flag(flag)
self.mw.col.update_card(self.card)
self.mw.update_undo_actions()
self._update_flag_icon()
def toggle_mark_on_current_note(self) -> None:
@ -797,8 +798,8 @@ time = %(time)d;
else:
note.add_tag("marked")
self.mw.col.update_note(note)
self._update_mark_icon()
self.mw.update_undo_actions()
self._update_mark_icon()
def on_set_due(self) -> None:
if self.mw.state != "review" or not self.card:
@ -863,3 +864,4 @@ time = %(time)d;
onSuspendCard = suspend_current_card
onDelete = delete_current_note
onMark = toggle_mark_on_current_note
setFlag = set_flag_on_current_card

View File

@ -162,8 +162,7 @@ service BackendService {
// cards
rpc GetCard(CardID) returns (Card);
rpc UpdateCard(Card) returns (Empty);
rpc AddCard(Card) returns (CardID);
rpc UpdateCard(UpdateCardIn) returns (Empty);
rpc RemoveCards(RemoveCardsIn) returns (Empty);
rpc SetDeck(SetDeckIn) returns (Empty);
@ -953,6 +952,11 @@ message UpdateNoteIn {
bool skip_undo_entry = 2;
}
message UpdateCardIn {
Card card = 1;
bool skip_undo_entry = 2;
}
message EmptyCardsReport {
message NoteWithEmptyCards {
int64 note_id = 1;

41
rslib/src/backend/card.rs Normal file
View File

@ -0,0 +1,41 @@
// Copyright: Ankitects Pty Ltd and contributors
// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
use std::convert::TryFrom;
use crate::prelude::*;
use crate::{
backend_proto as pb,
card::{CardQueue, CardType},
};
impl TryFrom<pb::Card> for Card {
type Error = AnkiError;
fn try_from(c: pb::Card) -> Result<Self, Self::Error> {
let ctype = CardType::try_from(c.ctype as u8)
.map_err(|_| AnkiError::invalid_input("invalid card type"))?;
let queue = CardQueue::try_from(c.queue as i8)
.map_err(|_| AnkiError::invalid_input("invalid card queue"))?;
Ok(Card {
id: CardID(c.id),
note_id: NoteID(c.note_id),
deck_id: DeckID(c.deck_id),
template_idx: c.template_idx as u16,
mtime: TimestampSecs(c.mtime_secs),
usn: Usn(c.usn),
ctype,
queue,
due: c.due,
interval: c.interval,
ease_factor: c.ease_factor as u16,
reps: c.reps,
lapses: c.lapses,
remaining_steps: c.remaining_steps,
original_due: c.original_due,
original_deck_id: DeckID(c.original_deck_id),
flags: c.flags as u8,
data: c.data,
})
}
}

View File

@ -1,6 +1,12 @@
// Copyright: Ankitects Pty Ltd and contributors
// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
mod card;
mod dbproxy;
mod generic;
mod http_sync_server;
mod scheduler;
pub use crate::backend_proto::BackendMethod;
use crate::{
backend::dbproxy::db_command_bytes,
@ -10,7 +16,6 @@ use crate::{
AddOrUpdateDeckConfigLegacyIn, BackendResult, Empty, RenderedTemplateReplacement,
},
card::{Card, CardID},
card::{CardQueue, CardType},
cloze::add_cloze_numbers_in_string,
collection::{open_collection, Collection},
config::SortKind,
@ -49,7 +54,6 @@ use crate::{
template::RenderedNode,
text::{escape_anki_wildcards, extract_av_tags, sanitize_html_no_images, strip_av_tags, AVTag},
timestamp::TimestampSecs,
types::Usn,
undo::UndoableOpKind,
};
use fluent::FluentValue;
@ -69,11 +73,6 @@ use std::{
};
use tokio::runtime::{self, Runtime};
mod dbproxy;
mod generic;
mod http_sync_server;
mod scheduler;
struct ThrottlingProgressHandler {
state: Arc<Mutex<ProgressState>>,
last_update: coarsetime::Instant,
@ -985,26 +984,19 @@ impl BackendService for Backend {
})
}
fn update_card(&self, input: pb::Card) -> BackendResult<Empty> {
let mut card = pbcard_to_native(input)?;
fn update_card(&self, input: pb::UpdateCardIn) -> BackendResult<Empty> {
self.with_col(|col| {
col.transact(None, |ctx| {
let orig = ctx
.storage
.get_card(card.id)?
.ok_or_else(|| AnkiError::invalid_input("missing card"))?;
ctx.update_card(&mut card, &orig, ctx.usn()?)
})
let op = if input.skip_undo_entry {
None
} else {
Some(UndoableOpKind::UpdateCard)
};
let mut card: Card = input.card.ok_or(AnkiError::NotFound)?.try_into()?;
col.update_card_with_op(&mut card, op)
})
.map(Into::into)
}
fn add_card(&self, input: pb::Card) -> BackendResult<pb::CardId> {
let mut card = pbcard_to_native(input)?;
self.with_col(|col| col.transact(None, |ctx| ctx.add_card(&mut card)))?;
Ok(pb::CardId { cid: card.id.0 })
}
fn remove_cards(&self, input: pb::RemoveCardsIn) -> BackendResult<Empty> {
self.with_col(|col| {
col.transact(None, |col| {
@ -2063,33 +2055,6 @@ impl From<Card> for pb::Card {
}
}
fn pbcard_to_native(c: pb::Card) -> Result<Card> {
let ctype = CardType::try_from(c.ctype as u8)
.map_err(|_| AnkiError::invalid_input("invalid card type"))?;
let queue = CardQueue::try_from(c.queue as i8)
.map_err(|_| AnkiError::invalid_input("invalid card queue"))?;
Ok(Card {
id: CardID(c.id),
note_id: NoteID(c.note_id),
deck_id: DeckID(c.deck_id),
template_idx: c.template_idx as u16,
mtime: TimestampSecs(c.mtime_secs),
usn: Usn(c.usn),
ctype,
queue,
due: c.due,
interval: c.interval,
ease_factor: c.ease_factor as u16,
reps: c.reps,
lapses: c.lapses,
remaining_steps: c.remaining_steps,
original_due: c.original_due,
original_deck_id: DeckID(c.original_deck_id),
flags: c.flags as u8,
data: c.data,
})
}
impl From<crate::scheduler::timing::SchedTimingToday> for pb::SchedTimingTodayOut {
fn from(t: crate::scheduler::timing::SchedTimingToday) -> pb::SchedTimingTodayOut {
pb::SchedTimingTodayOut {

View File

@ -3,12 +3,12 @@
pub(crate) mod undo;
use crate::define_newtype;
use crate::err::{AnkiError, Result};
use crate::notes::NoteID;
use crate::{
collection::Collection, config::SchedulerVersion, timestamp::TimestampSecs, types::Usn,
};
use crate::{define_newtype, undo::UndoableOpKind};
use crate::{deckconf::DeckConf, decks::DeckID};
use num_enum::TryFromPrimitive;
@ -139,6 +139,15 @@ impl Card {
}
impl Collection {
pub(crate) fn update_card_with_op(
&mut self,
card: &mut Card,
op: Option<UndoableOpKind>,
) -> Result<()> {
let existing = self.storage.get_card(card.id)?.ok_or(AnkiError::NotFound)?;
self.transact(op, |col| col.update_card_inner(card, &existing, col.usn()?))
}
#[cfg(test)]
pub(crate) fn get_and_update_card<F, T>(&mut self, cid: CardID, func: F) -> Result<Card>
where
@ -150,12 +159,17 @@ impl Collection {
.ok_or_else(|| AnkiError::invalid_input("no such card"))?;
let mut card = orig.clone();
func(&mut card)?;
self.update_card(&mut card, &orig, self.usn()?)?;
self.update_card_inner(&mut card, &orig, self.usn()?)?;
Ok(card)
}
/// Marks the card as modified, then saves it.
pub(crate) fn update_card(&mut self, card: &mut Card, original: &Card, usn: Usn) -> Result<()> {
pub(crate) fn update_card_inner(
&mut self,
card: &mut Card,
original: &Card,
usn: Usn,
) -> Result<()> {
card.set_modified(usn);
self.update_card_undoable(card, original)
}
@ -204,7 +218,7 @@ impl Collection {
}
let original = card.clone();
card.set_deck(deck_id, sched);
col.update_card(&mut card, &original, usn)?;
col.update_card_inner(&mut card, &original, usn)?;
}
Ok(())
})

View File

@ -184,7 +184,7 @@ impl Collection {
if let Some(mut card) = self.storage.get_card(*cid)? {
let original = card.clone();
card.remove_from_filtered_deck_restoring_queue(sched);
self.update_card(&mut card, &original, usn)?;
self.update_card_inner(&mut card, &original, usn)?;
}
}
Ok(())
@ -249,7 +249,7 @@ impl Collection {
for mut card in self.storage.all_searched_cards_in_search_order()? {
let original = card.clone();
card.move_into_filtered_deck(ctx, position);
self.update_card(&mut card, &original, ctx.usn)?;
self.update_card_inner(&mut card, &original, ctx.usn)?;
position += 1;
}

View File

@ -324,7 +324,8 @@ impl Collection {
self.generate_cards_for_new_note(ctx, note, did)
}
pub fn update_note(&mut self, note: &mut Note) -> Result<()> {
#[cfg(test)]
pub(crate) fn update_note(&mut self, note: &mut Note) -> Result<()> {
self.update_note_with_op(note, Some(UndoableOpKind::UpdateNote))
}

View File

@ -269,7 +269,7 @@ impl Collection {
self.maybe_bury_siblings(&original, &updater.config)?;
let timing = updater.timing;
let mut card = updater.into_card();
self.update_card(&mut card, &original, usn)?;
self.update_card_inner(&mut card, &original, usn)?;
if answer.new_state.leeched() {
self.add_leech_tag(card.note_id)?;
}

View File

@ -62,7 +62,7 @@ impl Collection {
for original in self.storage.all_searched_cards()? {
let mut card = original.clone();
if card.restore_queue_after_bury_or_suspend() {
self.update_card(&mut card, &original, usn)?;
self.update_card_inner(&mut card, &original, usn)?;
}
}
self.storage.clear_searched_cards_table()
@ -113,7 +113,7 @@ impl Collection {
card.remove_from_learning();
}
card.queue = desired_queue;
self.update_card(&mut card, &original, usn)?;
self.update_card_inner(&mut card, &original, usn)?;
}
}

View File

@ -114,7 +114,7 @@ impl Collection {
if log {
col.log_manually_scheduled_review(&card, &original, usn)?;
}
col.update_card(&mut card, &original, usn)?;
col.update_card_inner(&mut card, &original, usn)?;
position += 1;
}
col.set_next_card_position(position)?;
@ -155,7 +155,7 @@ impl Collection {
for mut card in cards {
let original = card.clone();
card.set_new_position(sorter.position(&card));
self.update_card(&mut card, &original, usn)?;
self.update_card_inner(&mut card, &original, usn)?;
}
self.storage.clear_searched_cards_table()
}
@ -177,7 +177,7 @@ impl Collection {
for mut card in self.storage.all_searched_cards()? {
let original = card.clone();
card.set_new_position(card.due as u32 + by);
self.update_card(&mut card, &original, usn)?;
self.update_card_inner(&mut card, &original, usn)?;
}
self.storage.clear_searched_cards_table()?;
Ok(())

View File

@ -96,7 +96,7 @@ impl Collection {
let days_from_today = distribution.sample(&mut rng);
card.set_due_date(today, days_from_today, spec.force_reset);
col.log_manually_scheduled_review(&card, &original, usn)?;
col.update_card(&mut card, &original, usn)?;
col.update_card_inner(&mut card, &original, usn)?;
}
col.storage.clear_searched_cards_table()?;
Ok(())

View File

@ -25,7 +25,7 @@ impl UndoableOpKind {
impl Collection {
pub fn describe_op_kind(&self, op: UndoableOpKind) -> String {
let key = match op {
UndoableOpKind::UpdateCard => todo!(),
UndoableOpKind::UpdateCard => TR::UndoUpdateCard,
UndoableOpKind::AnswerCard => TR::UndoAnswerCard,
UndoableOpKind::Bury => TR::StudyingBury,
UndoableOpKind::Suspend => TR::StudyingSuspend,