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

@@ -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

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:

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"):
+        if not entry.was_leech and card.note().has_tag("leech"):
-            card.note().delTag("leech")
+            card.note().remove_tag("leech")
             card.note().flush()
 
         # write old data

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:
+    def remove_tag(self, tag: str) -> None:
-        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:
         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:
+    def add_tag(self, tag: str) -> None:
-        # duplicates will be stripped on save
+        "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
     ##################################################
 

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"]

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:

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:

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 = col.get_note(col.db.scalar("select id from notes"))
-    n.addTag("test")
+    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.add_tag("four")
-    n.addTag("five")
+    n.add_tag("five")
     col.addNote(n)
 
     # https://stackoverflow.com/questions/23212435/permission-denied-to-write-to-my-temporary-file

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():

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:

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
     ######################################################################

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</button>""" % (
         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</button>""" % (
 
         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</button>""" % (
 
     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
     ##########################################################################

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._update_flag_icon()
-        self._drawMark()
+        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:
+    def _update_mark_icon(self) -> None:
-        self.web.eval(f"_drawMark({json.dumps(self.card.note().hasTag('marked'))});")
+        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:
+    def toggle_mark_on_current_note(self) -> None:
-        f = self.card.note()
+        note = self.card.note()
-        if f.hasTag("marked"):
+        if note.has_tag("marked"):
-            f.delTag("marked")
+            note.remove_tag("marked")
         else:
-            f.addTag("marked")
+            note.add_tag("marked")
-        f.flush()
+        self.mw.col.update_note(note)
-        self._drawMark()
+        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

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;

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<Empty> {
+    fn update_note(&self, input: pb::UpdateNoteIn) -> BackendResult<Empty> {
         self.with_col(|col| {
-            let mut note: Note = input.into();
+            let op = if input.skip_undo_entry {
-            col.update_note(&mut note)
+                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)
     }

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<UndoableOp>,
+    ) -> 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"))?;

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()