From f1a1b0891e0f0935de0e2fb564c3854b59e491f1 Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Fri, 5 Mar 2021 22:45:55 +1000 Subject: [PATCH] make mark toggling undoable - note.flush() behaves like before, as otherwise actions or add-ons that perform bulk flushing would end up creating an undo entry for each note - added col.update_note() to opt in to the new behaviour - tidy up the names of some related routines --- ftl/core/undo.ftl | 1 + pylib/anki/cards.py | 2 +- pylib/anki/collection.py | 15 +++++++++++---- pylib/anki/notes.py | 28 +++++++++++++++++----------- pylib/anki/sched.py | 2 +- pylib/anki/scheduler.py | 2 +- pylib/anki/schedv2.py | 4 ++-- pylib/tests/test_importing.py | 16 ++++++++-------- pylib/tests/test_models.py | 20 ++++++++++---------- qt/aqt/addcards.py | 2 +- qt/aqt/browser.py | 4 ++-- qt/aqt/main.py | 18 +++++++++++------- qt/aqt/reviewer.py | 35 ++++++++++++++++++++--------------- rslib/backend.proto | 7 ++++++- rslib/src/backend/mod.rs | 12 +++++++++--- rslib/src/notes.rs | 10 +++++++++- rslib/src/undo.rs | 2 ++ 17 files changed, 112 insertions(+), 68 deletions(-) diff --git a/ftl/core/undo.ftl b/ftl/core/undo.ftl index e8a04d7e5..b1b9ced57 100644 --- a/ftl/core/undo.ftl +++ b/ftl/core/undo.ftl @@ -13,3 +13,4 @@ undo-answer-card = Answer Card undo-unbury-unsuspend = Unbury/Unsuspend undo-add-note = Add Note undo-update-tag = Update Tag +undo-update-note = Update Note diff --git a/pylib/anki/cards.py b/pylib/anki/cards.py index bfb4a937b..4e46aa779 100644 --- a/pylib/anki/cards.py +++ b/pylib/anki/cards.py @@ -141,7 +141,7 @@ class Card: def note(self, reload: bool = False) -> Note: if not self._note or reload: - self._note = self.col.getNote(self.nid) + self._note = self.col.get_note(self.nid) return self._note def note_type(self) -> NoteType: diff --git a/pylib/anki/collection.py b/pylib/anki/collection.py index 7db1968ce..17e673740 100644 --- a/pylib/anki/collection.py +++ b/pylib/anki/collection.py @@ -322,9 +322,16 @@ class Collection: def getCard(self, id: int) -> Card: return Card(self, id) - def getNote(self, id: int) -> Note: + 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) + + getNote = get_note + # Utils ########################################################################## @@ -789,7 +796,7 @@ table.review-log {{ {revlog_style} }} if not isinstance(self._undo, _ReviewsUndo): self._undo = _ReviewsUndo() - was_leech = card.note().hasTag("leech") + was_leech = card.note().has_tag("leech") entry = ReviewUndo(card=copy.copy(card), was_leech=was_leech) self._undo.entries.append(entry) @@ -824,8 +831,8 @@ table.review-log {{ {revlog_style} }} card = entry.card # remove leech tag if it didn't have it before - if not entry.was_leech and card.note().hasTag("leech"): - card.note().delTag("leech") + if not entry.was_leech and card.note().has_tag("leech"): + card.note().remove_tag("leech") card.note().flush() # write old data diff --git a/pylib/anki/notes.py b/pylib/anki/notes.py index 2b90e35f9..c6c646201 100644 --- a/pylib/anki/notes.py +++ b/pylib/anki/notes.py @@ -66,8 +66,10 @@ class Note: ) def flush(self) -> None: + """This preserves any current checkpoint. + For an undo entry, use col.update_note() instead.""" assert self.id != 0 - self.col._backend.update_note(self.to_backend_note()) + self.col._backend.update_note(note=self.to_backend_note(), skip_undo_entry=True) def __repr__(self) -> str: d = dict(self.__dict__) @@ -154,16 +156,10 @@ class Note: # Tags ################################################## - def hasTag(self, tag: str) -> bool: + def has_tag(self, tag: str) -> bool: return self.col.tags.inList(tag, self.tags) - def stringTags(self) -> Any: - return self.col.tags.join(self.col.tags.canonify(self.tags)) - - def setTagsFromStr(self, tags: str) -> None: - self.tags = self.col.tags.split(tags) - - def delTag(self, tag: str) -> None: + def remove_tag(self, tag: str) -> None: rem = [] for t in self.tags: if t.lower() == tag.lower(): @@ -171,10 +167,20 @@ class Note: for r in rem: self.tags.remove(r) - def addTag(self, tag: str) -> None: - # duplicates will be stripped on save + def add_tag(self, tag: str) -> None: + "Add tag. Duplicates will be stripped on save." self.tags.append(tag) + def stringTags(self) -> Any: + return self.col.tags.join(self.col.tags.canonify(self.tags)) + + def setTagsFromStr(self, tags: str) -> None: + self.tags = self.col.tags.split(tags) + + hasTag = has_tag + addTag = add_tag + delTag = remove_tag + # Unique/duplicate check ################################################## diff --git a/pylib/anki/sched.py b/pylib/anki/sched.py index 143397a02..8eb73a00f 100644 --- a/pylib/anki/sched.py +++ b/pylib/anki/sched.py @@ -606,7 +606,7 @@ did = ? and queue = {QUEUE_TYPE_REV} and due <= ? limit ?""", if card.lapses >= lf and (card.lapses - lf) % (max(lf // 2, 1)) == 0: # add a leech tag f = card.note() - f.addTag("leech") + f.add_tag("leech") f.flush() # handle a = conf["leechAction"] diff --git a/pylib/anki/scheduler.py b/pylib/anki/scheduler.py index c8317730e..17920aee7 100644 --- a/pylib/anki/scheduler.py +++ b/pylib/anki/scheduler.py @@ -411,7 +411,7 @@ select id from cards where did in %s and queue = {QUEUE_TYPE_REV} and due <= ? l self.set_due_date(card_ids, f"{min_interval}-{max_interval}!") def buryNote(self, nid: int) -> None: - note = self.col.getNote(nid) + note = self.col.get_note(nid) self.bury_cards(note.card_ids()) def unburyCards(self) -> None: diff --git a/pylib/anki/schedv2.py b/pylib/anki/schedv2.py index 3dd435785..6a73412e1 100644 --- a/pylib/anki/schedv2.py +++ b/pylib/anki/schedv2.py @@ -1078,7 +1078,7 @@ limit ?""" if card.lapses >= lf and (card.lapses - lf) % (max(lf // 2, 1)) == 0: # add a leech tag f = card.note() - f.addTag("leech") + f.add_tag("leech") f.flush() # handle a = conf["leechAction"] @@ -1323,7 +1323,7 @@ select id from cards where did in %s and queue = {QUEUE_TYPE_REV} and due <= ? l self.set_due_date(card_ids, f"{min_interval}-{max_interval}!") def buryNote(self, nid: int) -> None: - note = self.col.getNote(nid) + note = self.col.get_note(nid) self.bury_cards(note.card_ids()) def unburyCards(self) -> None: diff --git a/pylib/tests/test_importing.py b/pylib/tests/test_importing.py index 84470b2a1..6c8bcd022 100644 --- a/pylib/tests/test_importing.py +++ b/pylib/tests/test_importing.py @@ -51,7 +51,7 @@ def test_anki2_mediadupes(): imp = Anki2Importer(empty, col.path) imp.run() assert os.listdir(empty.media.dir()) == ["foo.mp3"] - n = empty.getNote(empty.db.scalar("select id from notes")) + n = empty.get_note(empty.db.scalar("select id from notes")) assert "foo.mp3" in n.fields[0] # if the local file content is different, and import should trigger a # rename @@ -61,7 +61,7 @@ def test_anki2_mediadupes(): imp = Anki2Importer(empty, col.path) imp.run() assert sorted(os.listdir(empty.media.dir())) == ["foo.mp3", "foo_%s.mp3" % mid] - n = empty.getNote(empty.db.scalar("select id from notes")) + n = empty.get_note(empty.db.scalar("select id from notes")) assert "_" in n.fields[0] # if the localized media file already exists, we rewrite the note and # media @@ -72,7 +72,7 @@ def test_anki2_mediadupes(): imp.run() assert sorted(os.listdir(empty.media.dir())) == ["foo.mp3", "foo_%s.mp3" % mid] assert sorted(os.listdir(empty.media.dir())) == ["foo.mp3", "foo_%s.mp3" % mid] - n = empty.getNote(empty.db.scalar("select id from notes")) + n = empty.get_note(empty.db.scalar("select id from notes")) assert "_" in n.fields[0] @@ -162,8 +162,8 @@ def test_csv(): assert len(i.log) == 10 assert i.total == 5 # but importing should not clobber tags if they're unmapped - n = col.getNote(col.db.scalar("select id from notes")) - n.addTag("test") + n = col.get_note(col.db.scalar("select id from notes")) + n.add_tag("test") n.flush() i.run() n.load() @@ -217,7 +217,7 @@ def test_tsv_tag_modified(): n["Front"] = "1" n["Back"] = "2" n["Top"] = "3" - n.addTag("four") + n.add_tag("four") col.addNote(n) # https://stackoverflow.com/questions/23212435/permission-denied-to-write-to-my-temporary-file @@ -253,8 +253,8 @@ def test_tsv_tag_multiple_tags(): n["Front"] = "1" n["Back"] = "2" n["Top"] = "3" - n.addTag("four") - n.addTag("five") + n.add_tag("four") + n.add_tag("five") col.addNote(n) # https://stackoverflow.com/questions/23212435/permission-denied-to-write-to-my-temporary-file diff --git a/pylib/tests/test_models.py b/pylib/tests/test_models.py index 47e6cf55f..b0d584f6f 100644 --- a/pylib/tests/test_models.py +++ b/pylib/tests/test_models.py @@ -46,37 +46,37 @@ def test_fields(): # add a field field = col.models.newField("foo") col.models.addField(m, field) - assert col.getNote(col.models.nids(m)[0]).fields == ["1", "2", ""] + assert col.get_note(col.models.nids(m)[0]).fields == ["1", "2", ""] assert col.models.scmhash(m) != h # rename it field = m["flds"][2] col.models.renameField(m, field, "bar") - assert col.getNote(col.models.nids(m)[0])["bar"] == "" + assert col.get_note(col.models.nids(m)[0])["bar"] == "" # delete back col.models.remField(m, m["flds"][1]) - assert col.getNote(col.models.nids(m)[0]).fields == ["1", ""] + assert col.get_note(col.models.nids(m)[0]).fields == ["1", ""] # move 0 -> 1 col.models.moveField(m, m["flds"][0], 1) - assert col.getNote(col.models.nids(m)[0]).fields == ["", "1"] + assert col.get_note(col.models.nids(m)[0]).fields == ["", "1"] # move 1 -> 0 col.models.moveField(m, m["flds"][1], 0) - assert col.getNote(col.models.nids(m)[0]).fields == ["1", ""] + assert col.get_note(col.models.nids(m)[0]).fields == ["1", ""] # add another and put in middle field = col.models.newField("baz") col.models.addField(m, field) - note = col.getNote(col.models.nids(m)[0]) + note = col.get_note(col.models.nids(m)[0]) note["baz"] = "2" note.flush() - assert col.getNote(col.models.nids(m)[0]).fields == ["1", "", "2"] + assert col.get_note(col.models.nids(m)[0]).fields == ["1", "", "2"] # move 2 -> 1 col.models.moveField(m, m["flds"][2], 1) - assert col.getNote(col.models.nids(m)[0]).fields == ["1", "2", ""] + assert col.get_note(col.models.nids(m)[0]).fields == ["1", "2", ""] # move 0 -> 2 col.models.moveField(m, m["flds"][0], 2) - assert col.getNote(col.models.nids(m)[0]).fields == ["2", "", "1"] + assert col.get_note(col.models.nids(m)[0]).fields == ["2", "", "1"] # move 0 -> 1 col.models.moveField(m, m["flds"][0], 1) - assert col.getNote(col.models.nids(m)[0]).fields == ["", "2", "1"] + assert col.get_note(col.models.nids(m)[0]).fields == ["", "2", "1"] def test_templates(): diff --git a/qt/aqt/addcards.py b/qt/aqt/addcards.py index d5c44330d..01a9821b3 100644 --- a/qt/aqt/addcards.py +++ b/qt/aqt/addcards.py @@ -145,7 +145,7 @@ class AddCards(QDialog): m = QMenu(self) for nid in self.history: if self.mw.col.findNotes(SearchNode(nid=nid)): - note = self.mw.col.getNote(nid) + note = self.mw.col.get_note(nid) fields = note.fields txt = htmlToTextLine(", ".join(fields)) if len(txt) > 30: diff --git a/qt/aqt/browser.py b/qt/aqt/browser.py index b71c96a2e..a3b142d40 100644 --- a/qt/aqt/browser.py +++ b/qt/aqt/browser.py @@ -413,7 +413,7 @@ class StatusDelegate(QItemDelegate): col = None if c.userFlag() > 0: col = getattr(colors, f"FLAG{c.userFlag()}_BG") - elif c.note().hasTag("Marked"): + elif c.note().has_tag("Marked"): col = colors.MARKED_BG elif c.queue == QUEUE_TYPE_SUSPENDED: col = colors.SUSPENDED_BG @@ -1317,7 +1317,7 @@ where id in %s""" self.remove_tags_from_selected_notes(tags="marked") def isMarked(self) -> bool: - return bool(self.card and self.card.note().hasTag("Marked")) + return bool(self.card and self.card.note().has_tag("Marked")) # Repositioning ###################################################################### diff --git a/qt/aqt/main.py b/qt/aqt/main.py index c717acc61..1fe2af6bb 100644 --- a/qt/aqt/main.py +++ b/qt/aqt/main.py @@ -506,7 +506,7 @@ class AnkiQt(QMainWindow): # make sure we don't get into an inconsistent state if an add-on # has broken the deck browser or the did_load hook try: - self.maybeEnableUndo() + self.update_undo_actions() gui_hooks.collection_did_load(self.col) self.moveToState("deckBrowser") except Exception as e: @@ -688,7 +688,7 @@ class AnkiQt(QMainWindow): if not guiOnly: self.col.reset() gui_hooks.state_did_reset() - self.maybeEnableUndo() + self.update_undo_actions() self.moveToState(self.state) def requireReset( @@ -1032,7 +1032,7 @@ title="%s" %s>%s""" % ( if result is None: # should not happen showInfo("nothing to undo") - self.maybeEnableUndo() + self.update_undo_actions() return elif isinstance(result, ReviewUndo): @@ -1070,9 +1070,11 @@ title="%s" %s>%s""" % ( tooltip(tr(TR.UNDO_ACTION_UNDONE, action=name)) gui_hooks.state_did_revert(name) - self.maybeEnableUndo() + self.update_undo_actions() - def maybeEnableUndo(self) -> None: + def update_undo_actions(self) -> None: + """Update menu text and enable/disable menu item as appropriate. + Plural as this may handle redo in the future too.""" if self.col: status = self.col.undo_status() undo_action = status.undo or None @@ -1091,11 +1093,13 @@ title="%s" %s>%s""" % ( def checkpoint(self, name: str) -> None: self.col.save(name) - self.maybeEnableUndo() + self.update_undo_actions() def autosave(self) -> None: self.col.autosave() - self.maybeEnableUndo() + self.update_undo_actions() + + maybeEnableUndo = update_undo_actions # Other menu operations ########################################################################## diff --git a/qt/aqt/reviewer.py b/qt/aqt/reviewer.py index fd6f420a8..07040c83d 100644 --- a/qt/aqt/reviewer.py +++ b/qt/aqt/reviewer.py @@ -204,8 +204,8 @@ class Reviewer: bodyclass = theme_manager.body_classes_for_card_ord(c.ord) self.web.eval(f"_showQuestion({json.dumps(q)},'{bodyclass}');") - self._drawFlag() - self._drawMark() + self._update_flag_icon() + self._update_mark_icon() self._showAnswerButton() self.mw.web.setFocus() # user hook @@ -215,11 +215,14 @@ class Reviewer: print("use card.autoplay() instead of reviewer.autoplay(card)") return card.autoplay() - def _drawFlag(self) -> None: + def _update_flag_icon(self) -> None: self.web.eval(f"_drawFlag({self.card.userFlag()});") - def _drawMark(self) -> None: - self.web.eval(f"_drawMark({json.dumps(self.card.note().hasTag('marked'))});") + def _update_mark_icon(self) -> None: + self.web.eval(f"_drawMark({json.dumps(self.card.note().has_tag('marked'))});") + + _drawMark = _update_mark_icon + _drawFlag = _update_flag_icon # Showing the answer ########################################################################## @@ -291,7 +294,7 @@ class Reviewer: ("Ctrl+2", lambda: self.setFlag(2)), ("Ctrl+3", lambda: self.setFlag(3)), ("Ctrl+4", lambda: self.setFlag(4)), - ("*", self.onMark), + ("*", self.toggle_mark_on_current_note), ("=", self.bury_current_note), ("-", self.bury_current_card), ("!", self.suspend_current_note), @@ -726,7 +729,7 @@ time = %(time)d; ], ], ], - [tr(TR.STUDYING_MARK_NOTE), "*", self.onMark], + [tr(TR.STUDYING_MARK_NOTE), "*", self.toggle_mark_on_current_note], [tr(TR.STUDYING_BURY_CARD), "-", self.bury_current_card], [tr(TR.STUDYING_BURY_NOTE), "=", self.bury_current_note], [tr(TR.ACTIONS_SET_DUE_DATE), "Ctrl+Shift+D", self.on_set_due], @@ -785,16 +788,17 @@ time = %(time)d; flag = 0 self.card.setUserFlag(flag) self.card.flush() - self._drawFlag() + self._update_flag_icon() - def onMark(self) -> None: - f = self.card.note() - if f.hasTag("marked"): - f.delTag("marked") + def toggle_mark_on_current_note(self) -> None: + note = self.card.note() + if note.has_tag("marked"): + note.remove_tag("marked") else: - f.addTag("marked") - f.flush() - self._drawMark() + note.add_tag("marked") + self.mw.col.update_note(note) + self._update_mark_icon() + self.mw.update_undo_actions() def on_set_due(self) -> None: if self.mw.state != "review" or not self.card: @@ -858,3 +862,4 @@ time = %(time)d; onSuspend = suspend_current_note onSuspendCard = suspend_current_card onDelete = delete_current_note + onMark = toggle_mark_on_current_note diff --git a/rslib/backend.proto b/rslib/backend.proto index 6960b6b70..79a7e367e 100644 --- a/rslib/backend.proto +++ b/rslib/backend.proto @@ -171,7 +171,7 @@ service BackendService { rpc NewNote(NoteTypeID) returns (Note); rpc AddNote(AddNoteIn) returns (NoteID); - rpc UpdateNote(Note) returns (Empty); + rpc UpdateNote(UpdateNoteIn) returns (Empty); rpc GetNote(NoteID) returns (Note); rpc RemoveNotes(RemoveNotesIn) returns (Empty); rpc AddNoteTags(AddNoteTagsIn) returns (UInt32); @@ -948,6 +948,11 @@ message AddNoteIn { int64 deck_id = 2; } +message UpdateNoteIn { + Note note = 1; + bool skip_undo_entry = 2; +} + message EmptyCardsReport { message NoteWithEmptyCards { int64 note_id = 1; diff --git a/rslib/src/backend/mod.rs b/rslib/src/backend/mod.rs index 3d21efc0f..068a3fbe0 100644 --- a/rslib/src/backend/mod.rs +++ b/rslib/src/backend/mod.rs @@ -50,6 +50,7 @@ use crate::{ text::{escape_anki_wildcards, extract_av_tags, sanitize_html_no_images, strip_av_tags, AVTag}, timestamp::TimestampSecs, types::Usn, + undo::UndoableOp, }; use fluent::FluentValue; use futures::future::{AbortHandle, AbortRegistration, Abortable}; @@ -1043,10 +1044,15 @@ impl BackendService for Backend { }) } - fn update_note(&self, input: pb::Note) -> BackendResult { + fn update_note(&self, input: pb::UpdateNoteIn) -> BackendResult { self.with_col(|col| { - let mut note: Note = input.into(); - col.update_note(&mut note) + let op = if input.skip_undo_entry { + None + } else { + Some(UndoableOp::UpdateNote) + }; + let mut note: Note = input.note.ok_or(AnkiError::NotFound)?.into(); + col.update_note_with_op(&mut note, op) }) .map(Into::into) } diff --git a/rslib/src/notes.rs b/rslib/src/notes.rs index b6d0b0c22..cca83051a 100644 --- a/rslib/src/notes.rs +++ b/rslib/src/notes.rs @@ -330,13 +330,21 @@ impl Collection { } pub fn update_note(&mut self, note: &mut Note) -> Result<()> { + self.update_note_with_op(note, Some(UndoableOp::UpdateNote)) + } + + pub(crate) fn update_note_with_op( + &mut self, + note: &mut Note, + op: Option, + ) -> Result<()> { let mut existing_note = self.storage.get_note(note.id)?.ok_or(AnkiError::NotFound)?; if !note_modified(&mut existing_note, note) { // nothing to do return Ok(()); } - self.transact(None, |col| { + self.transact(op, |col| { let nt = col .get_notetype(note.notetype_id)? .ok_or_else(|| AnkiError::invalid_input("missing note type"))?; diff --git a/rslib/src/undo.rs b/rslib/src/undo.rs index 7e2b57282..7f8b64c12 100644 --- a/rslib/src/undo.rs +++ b/rslib/src/undo.rs @@ -16,6 +16,7 @@ pub enum UndoableOp { AddNote, RemoveNote, UpdateTag, + UpdateNote, } impl UndoableOp { @@ -35,6 +36,7 @@ impl Collection { UndoableOp::AddNote => TR::UndoAddNote, UndoableOp::RemoveNote => TR::StudyingDeleteNote, UndoableOp::UpdateTag => TR::UndoUpdateTag, + UndoableOp::UpdateNote => TR::UndoUpdateNote, }; self.i18n.tr(key).to_string()