The previous change in 1871b57663 failed
to consider the browser refreshing case, as reported here:
https://forums.ankiweb.net/t/anki-2-1-50-beta-3-4/17501/30
I previously attempted to solve this by having SetFlag skip the queue
rebuild, then mutating the captured mtimes in the queues. That didn't
work correctly when undoing, as the queue mutations weren't recorded.
This approach combines that attempt and the previous change: flag
setting is an undoable operation again, but does not change the card's
modification time, so it can be applied/undone without a queue build
being required. Instead of special-casing flag changes in the review
screen, we now just redraw the flag on changes.card, as any other card
op will have triggered a queue rebuild.
* Replace Card.data with .original_position
* Use and update original position in v3
* Show original position in card info
* Revert restoring original position for now
* Fix pb card to/from pylib card
* Try original_position as the last pb field
* minor wording tweaks (dae)
This is not ideal, but I struggled to come up with a better solution.
Background:
- The scheduler records the mtime of cards as it's building the queues,
and will throw an error in get_queued_cards() if the card on the DB
has a different mtime. This is to catch bugs - any operation that modifies
cards should be triggering a queue rebuild, or should adjust the queues
appropriately.
- The review screen skips the usual queue rebuild redraw, and directly
updates the flag icon. This is because a rebuild could cause a different
card to appear, or the answer side to switch back to the question side,
neither of which the user expects when they flag a card.
The current behaviour was broken: the queue rebuilding was still happening
on the backend, and the frontend was just failing to reflect it.
I initially tried to special-case Op::SetFlag, having it skip the queue
rebuild, and having set_card_flag() update the mtimes in the active
queue. But those mutations weren't captured by the undo log, so they
didn't get undone when undoing the set flag operation. We could perhaps
work around it by adding a separate undo entry to capture the mutation,
but it started to feel like it would be a pain to maintain moving forward.
By skipping the undo queue and retaining the same mtime, no queue
rebuild is required. Because we're setting usn, the cards will still
sync, but as mtime is not bumped, in the case of a conflict, an older
unsynced change from another client may revert the flag change.
Fixes https://forums.ankiweb.net/t/anki-2-1-50-beta-1-2/15608/145
- make sure we set flag in changes when config var changed
- move current deck get/set into backend
- set_config() now returns a bool indicating whether a change was
made, so other operations can be gated off it
- active decks generation is deferred until sched.reset()
So, this is fun. Apparently "DeckId" is considered preferable to the
"DeckID" were were using until now, and the latest clippy will start
warning about it. We could of course disable the warning, but probably
better to bite the bullet and switch to the naming that's generally
considered best.
- Introduced a new transact() method that wraps the return value
in a separate struct that describes the changes that were made.
- Changes are now gathered from the undo log, so we don't need to
guess at what was changed - eg if update_note() is called with identical
note contents, no changes are returned. Card changes will only be set
if cards were actually generated by the update_note() call, and tag
will only be set if a new tag was added.
- mw.perform_op() has been updated to expect the op to return the changes,
or a structure with the changes in it, and it will use them to fire the
change hook, instead of fetching the changes from undo_status(), so there
is no risk of race conditions.
- the various calls to mw.perform_op() have been split into separate
files like card_ops.py. Aside from making the code cleaner, this works
around a rather annoying issue with mypy. Because we run it with
no_strict_optional, mypy is happy to accept an operation that returns None,
despite the type signature saying it requires changes to be returned.
Turning no_strict_optional on for the whole codebase is not practical
at the moment, but we can enable it for individual files.
Still todo:
- The cursor keeps moving back to the start of a field when typing -
we need to ignore the refresh hook when we are the initiator.
- The busy cursor icon should probably be delayed a few hundreds ms.
- Still need to think about a nicer way of handling saveNow()
- op_made_changes(), op_affects_study_queue() might be better embedded
as properties in the object instead
'card modified' covers the common case where we need to rebuild the
study queue, but is also set when changing the card flags. We want to
avoid a queue rebuild in that case, as it causes UI flicker, and may
result in a different card being shown. Note marking doesn't trigger
a queue build, but still causes flicker, and may return the user back
to the front side when they were looking at the answer.
I still think entity-based change tracking is the simplest in the
common case, but to solve the above, I've introduced an enum describing
the last operation that was taken. This currently is not trying to list
out all possible operations, and just describes the ones we want to
special-case.
Other changes:
- Fire the old 'state_did_reset' hook after an operation is performed,
so legacy code can refresh itself after an operation is performed.
- Fire the new `operation_did_execute` hook when mw.reset() is called,
so that as the UI is updated to the use the new hook, it will still
be able to refresh after legacy code calls mw.reset()
- Update the deck browser, overview and review screens to listen to
the new hook, instead of relying on the main window to call moveToState()
- Add a 'set flag' backend action, so we can distinguish it from a
normal card update.
- Drop the separate added/modified entries in the change list in
favour of a single entry per entity.
- Add typing to mw.state
- Tweak perform_op()
- Convert a few more actions to use perform_op()
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
To coalesce successive note edits into a single undo op we'll need to
be able to get the original Undoable type, which is awkward to do with
a trait object.
