note deletion undo; refactoring

- transact() now automatically clears card queues unless an op
opts-out (and currently only AnswerCard does). This means there's no
risk of forgetting to clear the queues in an operation, or when undoing/
redoing
- CollectionOp->UndoableOp
- clear queues when redoing "answer card", instead of clearing redo
when clearing queues
This commit is contained in:
Damien Elmes 2021-03-05 19:09:08 +10:00
parent f4d931131b
commit 49a1970399
19 changed files with 142 additions and 134 deletions

View File

@ -3,9 +3,9 @@ undo-redo = Redo
# eg "Undo Answer Card"
undo-undo-action = Undo { $val }
# eg "Answer Card Undone"
undo-action-undone = { $action } Undone
undo-action-undone = { $action } undone
undo-redo-action = Redo { $action }
undo-action-redone = { $action } Redone
undo-action-redone = { $action } redone
undo-answer-card = Answer Card
undo-unbury-unsuspend = Unbury/Unsuspend
undo-add-note = Add Note

View File

@ -56,7 +56,8 @@ class Scheduler:
##########################################################################
def reset(self) -> None:
self.col._backend.clear_card_queues()
# backend automatically resets queues as operations are performed
pass
def get_queued_cards(
self,

View File

@ -1027,6 +1027,7 @@ title="%s" %s>%s</button>""" % (
def onUndo(self) -> None:
reviewing = self.state == "review"
result = self.col.undo()
just_refresh_reviewer = False
if result is None:
# should not happen
@ -1037,24 +1038,22 @@ title="%s" %s>%s</button>""" % (
elif isinstance(result, ReviewUndo):
name = tr(TR.SCHEDULING_REVIEW)
# restore the undone card if reviewing
if reviewing:
# push the undone card to the top of the queue
cid = result.card.id
card = self.col.getCard(cid)
self.reviewer.cardQueue.append(card)
self.reviewer.nextCard()
gui_hooks.review_did_undo(cid)
self.maybeEnableUndo()
return
just_refresh_reviewer = True
elif isinstance(result, BackendUndo):
name = result.name
# new scheduler takes care of rebuilding queue
if reviewing and self.col.sched.is_2021:
self.reviewer.nextCard()
self.maybeEnableUndo()
return
# new scheduler will have taken care of updating queue
just_refresh_reviewer = True
elif isinstance(result, Checkpoint):
name = result.name
@ -1063,8 +1062,13 @@ title="%s" %s>%s</button>""" % (
assert_exhaustive(result)
assert False
self.reset()
tooltip(tr(TR.QT_MISC_REVERTED_TO_STATE_PRIOR_TO, val=name))
if just_refresh_reviewer:
self.reviewer.nextCard()
else:
# full queue+gui reset required
self.reset()
tooltip(tr(TR.UNDO_ACTION_UNDONE, action=name))
gui_hooks.state_did_revert(name)
self.maybeEnableUndo()

View File

@ -296,7 +296,7 @@ class Reviewer:
("-", self.bury_current_card),
("!", self.suspend_current_note),
("@", self.suspend_current_card),
("Ctrl+Delete", self.onDelete),
("Ctrl+Delete", self.delete_current_note),
("Ctrl+Shift+D", self.on_set_due),
("v", self.onReplayRecorded),
("Shift+v", self.onRecordVoice),
@ -732,7 +732,7 @@ time = %(time)d;
[tr(TR.ACTIONS_SET_DUE_DATE), "Ctrl+Shift+D", self.on_set_due],
[tr(TR.ACTIONS_SUSPEND_CARD), "@", self.suspend_current_card],
[tr(TR.STUDYING_SUSPEND_NOTE), "!", self.suspend_current_note],
[tr(TR.STUDYING_DELETE_NOTE), "Ctrl+Delete", self.onDelete],
[tr(TR.STUDYING_DELETE_NOTE), "Ctrl+Delete", self.delete_current_note],
[tr(TR.ACTIONS_OPTIONS), "O", self.onOptions],
None,
[tr(TR.ACTIONS_REPLAY_AUDIO), "R", self.replayAudio],
@ -828,12 +828,11 @@ time = %(time)d;
self.mw.reset()
tooltip(tr(TR.STUDYING_NOTE_BURIED))
def onDelete(self) -> None:
def delete_current_note(self) -> None:
# need to check state because the shortcut is global to the main
# window
if self.mw.state != "review" or not self.card:
return
self.mw.checkpoint(tr(TR.ACTIONS_DELETE))
cnt = len(self.card.note().cards())
self.mw.col.remove_notes([self.card.note().id])
self.mw.reset()
@ -858,3 +857,4 @@ time = %(time)d;
onBuryNote = bury_current_note
onSuspend = suspend_current_note
onSuspendCard = suspend_current_card
onDelete = delete_current_note

View File

@ -121,7 +121,6 @@ service BackendService {
rpc AnswerCard(AnswerCardIn) returns (Empty);
rpc UpgradeScheduler(Empty) returns (Empty);
rpc GetQueuedCards(GetQueuedCardsIn) returns (GetQueuedCardsOut);
rpc ClearCardQueues(Empty) returns (Empty);
// stats

View File

@ -105,8 +105,8 @@ pub(super) fn db_command_bytes(col: &mut Collection, input: &[u8]) -> Result<Vec
fn maybe_clear_undo(col: &mut Collection, sql: &str) {
if !is_dql(sql) {
println!("clearing undo due to {}", sql);
col.state.undo.clear();
println!("clearing undo+study due to {}", sql);
col.discard_undo_and_study_queues();
}
}

View File

@ -705,13 +705,6 @@ impl BackendService for Backend {
self.with_col(|col| col.get_queued_cards(input.fetch_limit, input.intraday_learning_only))
}
fn clear_card_queues(&self, _input: pb::Empty) -> BackendResult<pb::Empty> {
self.with_col(|col| {
col.clear_study_queues();
Ok(().into())
})
}
// statistics
//-----------------------------------------------

View File

@ -1,19 +1,17 @@
// Copyright: Ankitects Pty Ltd and contributors
// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
mod op;
use crate::i18n::I18n;
use crate::log::Logger;
use crate::types::Usn;
use crate::{
decks::{Deck, DeckID},
notetype::{NoteType, NoteTypeID},
prelude::*,
storage::SqliteStorage,
undo::UndoManager,
};
use crate::{err::Result, scheduler::queue::CardQueues};
pub use op::CollectionOp;
use std::{collections::HashMap, path::PathBuf, sync::Arc};
pub fn open_collection<P: Into<PathBuf>>(
@ -84,12 +82,12 @@ pub struct Collection {
impl Collection {
/// Execute the provided closure in a transaction, rolling back if
/// an error is returned.
pub(crate) fn transact<F, R>(&mut self, op: Option<CollectionOp>, func: F) -> Result<R>
pub(crate) fn transact<F, R>(&mut self, op: Option<UndoableOp>, func: F) -> Result<R>
where
F: FnOnce(&mut Collection) -> Result<R>,
{
self.storage.begin_rust_trx()?;
self.state.undo.begin_step(op);
self.begin_undoable_operation(op);
let mut res = func(self);
@ -102,10 +100,10 @@ impl Collection {
}
if res.is_err() {
self.state.undo.discard_step();
self.discard_undo_and_study_queues();
self.storage.rollback_rust_trx()?;
} else {
self.state.undo.end_step();
self.end_undoable_operation();
}
res

View File

@ -1,31 +0,0 @@
// Copyright: Ankitects Pty Ltd and contributors
// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
use crate::prelude::*;
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum CollectionOp {
UpdateCard,
AnswerCard,
Bury,
Suspend,
UnburyUnsuspend,
AddNote,
RemoveNote,
}
impl Collection {
pub fn describe_collection_op(&self, op: CollectionOp) -> String {
let key = match op {
CollectionOp::UpdateCard => todo!(),
CollectionOp::AnswerCard => TR::UndoAnswerCard,
CollectionOp::Bury => TR::StudyingBury,
CollectionOp::Suspend => TR::StudyingSuspend,
CollectionOp::UnburyUnsuspend => TR::UndoUnburyUnsuspend,
CollectionOp::AddNote => TR::UndoAddNote,
CollectionOp::RemoveNote => TR::StudyingDeleteNote,
};
self.i18n.tr(key).to_string()
}
}

View File

@ -3,11 +3,11 @@
use crate::{
backend_proto as pb,
collection::{Collection, CollectionOp},
decks::DeckID,
define_newtype,
err::{AnkiError, Result},
notetype::{CardGenContext, NoteField, NoteType, NoteTypeID},
prelude::*,
template::field_is_empty,
text::{ensure_string_in_nfc, normalize_to_nfc, strip_html_preserving_media_filenames},
timestamp::TimestampSecs,
@ -298,7 +298,7 @@ impl Collection {
}
pub fn add_note(&mut self, note: &mut Note, did: DeckID) -> Result<()> {
self.transact(Some(CollectionOp::AddNote), |col| {
self.transact(Some(UndoableOp::AddNote), |col| {
let nt = col
.get_notetype(note.notetype_id)?
.ok_or_else(|| AnkiError::invalid_input("missing note type"))?;
@ -424,7 +424,7 @@ impl Collection {
/// Remove provided notes, and any cards that use them.
pub(crate) fn remove_notes(&mut self, nids: &[NoteID]) -> Result<()> {
let usn = self.usn()?;
self.transact(Some(CollectionOp::RemoveNote), |col| {
self.transact(Some(UndoableOp::RemoveNote), |col| {
for nid in nids {
let nid = *nid;
if let Some(_existing_note) = col.storage.get_note(nid)? {
@ -749,6 +749,7 @@ mod test {
col.storage.db_scalar::<u32>("select count() from graves")?,
0
);
assert_eq!(col.next_card()?.is_some(), false);
Ok(())
};
@ -759,6 +760,7 @@ mod test {
col.storage.db_scalar::<u32>("select count() from graves")?,
0
);
assert_eq!(col.next_card()?.is_some(), true);
Ok(())
};
@ -786,6 +788,7 @@ mod test {
col.storage.db_scalar::<u32>("select count() from graves")?,
3
);
assert_eq!(col.next_card()?.is_some(), false);
Ok(())
};

View File

@ -3,7 +3,7 @@
pub use crate::{
card::{Card, CardID},
collection::{Collection, CollectionOp},
collection::Collection,
deckconf::{DeckConf, DeckConfID},
decks::{Deck, DeckID, DeckKind},
err::{AnkiError, Result},
@ -13,5 +13,6 @@ pub use crate::{
revlog::RevlogID,
timestamp::{TimestampMillis, TimestampSecs},
types::Usn,
undo::UndoableOp,
};
pub use slog::{debug, Logger};

View File

@ -241,7 +241,7 @@ impl Collection {
/// Answer card, writing its new state to the database.
pub fn answer_card(&mut self, answer: &CardAnswer) -> Result<()> {
self.transact(Some(CollectionOp::AnswerCard), |col| {
self.transact(Some(UndoableOp::AnswerCard), |col| {
col.answer_card_inner(answer)
})
}

View File

@ -6,6 +6,7 @@ mod test {
use crate::{
card::{CardQueue, CardType},
collection::open_test_collection,
deckconf::LeechAction,
prelude::*,
scheduler::answering::{CardAnswer, Rating},
};
@ -22,9 +23,10 @@ mod test {
note.set_field(1, "two")?;
col.add_note(&mut note, DeckID(1))?;
// turn burying on
// turn burying and leech suspension on
let mut conf = col.storage.get_deck_config(DeckConfID(1))?.unwrap();
conf.inner.bury_new = true;
conf.inner.leech_action = LeechAction::Suspend as i32;
col.storage.update_deck_conf(&conf)?;
// get the first card
@ -95,6 +97,7 @@ mod test {
let deck = col.get_deck(DeckID(1))?.unwrap();
assert_eq!(deck.common.review_studied, 1);
dbg!(&col.next_card()?);
assert_eq!(col.next_card()?.is_some(), false);
Ok(())
@ -130,10 +133,8 @@ mod test {
assert_post_review_state(&mut col)?;
col.undo()?;
assert_pre_review_state(&mut col)?;
col.redo()?;
assert_post_review_state(&mut col)?;
col.undo()?;
assert_pre_review_state(&mut col)?;
col.undo()?;

View File

@ -69,8 +69,7 @@ impl Collection {
}
pub fn unbury_or_unsuspend_cards(&mut self, cids: &[CardID]) -> Result<()> {
self.transact(Some(CollectionOp::UnburyUnsuspend), |col| {
col.clear_study_queues();
self.transact(Some(UndoableOp::UnburyUnsuspend), |col| {
col.storage.set_search_table_to_card_ids(cids, false)?;
col.unsuspend_or_unbury_searched_cards()
})
@ -127,11 +126,10 @@ impl Collection {
mode: BuryOrSuspendMode,
) -> Result<()> {
let op = match mode {
BuryOrSuspendMode::Suspend => CollectionOp::Suspend,
BuryOrSuspendMode::BurySched | BuryOrSuspendMode::BuryUser => CollectionOp::Bury,
BuryOrSuspendMode::Suspend => UndoableOp::Suspend,
BuryOrSuspendMode::BurySched | BuryOrSuspendMode::BuryUser => UndoableOp::Bury,
};
self.transact(Some(op), |col| {
col.clear_study_queues();
col.storage.set_search_table_to_card_ids(cids, false)?;
col.bury_or_suspend_searched_cards(mode)
})

View File

@ -132,12 +132,11 @@ impl Collection {
}
}
/// This is automatically done when transact() is called for everything
/// except card answers, so unless you are modifying state outside of a
/// transaction, you probably don't need this.
pub(crate) fn clear_study_queues(&mut self) {
self.state.card_queues = None;
// clearing the queue will remove any undone reviews from the undo queue,
// causing problems if we then try to redo them, so we need to clear the
// redo queue as well
self.state.undo.clear_redo();
}
pub(crate) fn update_queues_after_answering_card(

View File

@ -34,16 +34,13 @@ pub(super) struct QueueUpdateAfterUndoingAnswer {
impl Undo for QueueUpdateAfterUndoingAnswer {
fn undo(self: Box<Self>, col: &mut Collection) -> Result<()> {
let timing = col.timing_today()?;
let queues = col.get_queues()?;
let mut modified_learning = None;
if let Some(learning) = self.learning_requeue {
modified_learning = Some(queues.requeue_learning_entry(learning, timing));
}
queues.pop_undo_entry(self.entry.card_id());
// don't try to update existing queue when redoing; just
// rebuild it instead
col.clear_study_queues();
// but preserve undo state for a subsequent undo
col.save_undo(Box::new(QueueUpdateAfterAnsweringCard {
entry: self.entry,
learning_requeue: modified_learning,
learning_requeue: self.learning_requeue,
}));
Ok(())

View File

@ -326,7 +326,7 @@ where
SyncActionRequired::NoChanges => Ok(state.into()),
SyncActionRequired::FullSyncRequired { .. } => Ok(state.into()),
SyncActionRequired::NormalSyncRequired => {
self.col.state.undo.clear();
self.col.discard_undo_and_study_queues();
self.col.storage.begin_trx()?;
self.col
.unbury_if_day_rolled_over(self.col.timing_today()?)?;

View File

@ -102,7 +102,7 @@ impl SyncServer for LocalServer {
self.client_usn = client_usn;
self.client_is_newer = client_is_newer;
self.col.state.undo.clear();
self.col.discard_undo_and_study_queues();
self.col.storage.begin_rust_trx()?;
// make sure any pending cards have been unburied first if necessary

View File

@ -2,13 +2,42 @@
// License: GNU AGPL, version 3 or later; http://www.gnu.org/licenses/agpl.html
use crate::backend_proto as pb;
use crate::{
collection::{Collection, CollectionOp},
err::Result,
};
use crate::{collection::Collection, err::Result, prelude::*};
use std::{collections::VecDeque, fmt};
const UNDO_LIMIT: usize = 30;
#[derive(Debug, Clone, Copy, PartialEq)]
pub enum UndoableOp {
UpdateCard,
AnswerCard,
Bury,
Suspend,
UnburyUnsuspend,
AddNote,
RemoveNote,
}
impl UndoableOp {
pub(crate) fn needs_study_queue_reset(self) -> bool {
self != UndoableOp::AnswerCard
}
}
impl Collection {
pub fn describe_collection_op(&self, op: UndoableOp) -> String {
let key = match op {
UndoableOp::UpdateCard => todo!(),
UndoableOp::AnswerCard => TR::UndoAnswerCard,
UndoableOp::Bury => TR::StudyingBury,
UndoableOp::Suspend => TR::StudyingSuspend,
UndoableOp::UnburyUnsuspend => TR::UndoUnburyUnsuspend,
UndoableOp::AddNote => TR::UndoAddNote,
UndoableOp::RemoveNote => TR::StudyingDeleteNote,
};
self.i18n.tr(key).to_string()
}
}
pub(crate) trait Undo: fmt::Debug + Send {
/// Undo the recorded action.
@ -17,7 +46,7 @@ pub(crate) trait Undo: fmt::Debug + Send {
#[derive(Debug)]
struct UndoStep {
kind: CollectionOp,
kind: UndoableOp,
changes: Vec<Box<dyn Undo>>,
}
@ -46,15 +75,17 @@ pub(crate) struct UndoManager {
}
impl UndoManager {
pub(crate) fn save(&mut self, item: Box<dyn Undo>) {
fn save(&mut self, item: Box<dyn Undo>) {
if let Some(step) = self.current_step.as_mut() {
step.changes.push(item)
}
}
pub(crate) fn begin_step(&mut self, op: Option<CollectionOp>) {
fn begin_step(&mut self, op: Option<UndoableOp>) {
println!("begin: {:?}", op);
if op.is_none() {
self.clear();
self.undo_steps.clear();
self.redo_steps.clear();
} else if self.mode == UndoMode::NormalOp {
// a normal op clears the redo queue
self.redo_steps.clear();
@ -65,16 +96,7 @@ impl UndoManager {
});
}
pub(crate) fn clear(&mut self) {
self.undo_steps.clear();
self.redo_steps.clear();
}
pub(crate) fn clear_redo(&mut self) {
self.redo_steps.clear();
}
pub(crate) fn end_step(&mut self) {
fn end_step(&mut self) {
if let Some(step) = self.current_step.take() {
if self.mode == UndoMode::Undoing {
self.redo_steps.push(step);
@ -83,27 +105,31 @@ impl UndoManager {
self.undo_steps.push_front(step);
}
}
println!("ended, undo steps count now {}", self.undo_steps.len());
}
pub(crate) fn discard_step(&mut self) {
self.begin_step(None)
fn current_step_requires_study_queue_reset(&self) -> bool {
self.current_step
.as_ref()
.map(|s| s.kind.needs_study_queue_reset())
.unwrap_or(true)
}
fn can_undo(&self) -> Option<CollectionOp> {
fn can_undo(&self) -> Option<UndoableOp> {
self.undo_steps.front().map(|s| s.kind)
}
fn can_redo(&self) -> Option<CollectionOp> {
fn can_redo(&self) -> Option<UndoableOp> {
self.redo_steps.last().map(|s| s.kind)
}
}
impl Collection {
pub fn can_undo(&self) -> Option<CollectionOp> {
pub fn can_undo(&self) -> Option<UndoableOp> {
self.state.undo.can_undo()
}
pub fn can_redo(&self) -> Option<CollectionOp> {
pub fn can_redo(&self) -> Option<UndoableOp> {
self.state.undo.can_redo()
}
@ -139,11 +165,6 @@ impl Collection {
Ok(())
}
#[inline]
pub(crate) fn save_undo(&mut self, item: Box<dyn Undo>) {
self.state.undo.save(item)
}
pub fn undo_status(&self) -> pb::UndoStatus {
pb::UndoStatus {
undo: self
@ -156,12 +177,36 @@ impl Collection {
.unwrap_or_default(),
}
}
/// If op is None, clears the undo/redo queues.
pub(crate) fn begin_undoable_operation(&mut self, op: Option<UndoableOp>) {
self.state.undo.begin_step(op);
}
/// Called at the end of a successful transaction.
/// In most instances, this will also clear the study queues.
pub(crate) fn end_undoable_operation(&mut self) {
if self.state.undo.current_step_requires_study_queue_reset() {
self.clear_study_queues();
}
self.state.undo.end_step();
}
pub(crate) fn discard_undo_and_study_queues(&mut self) {
self.state.undo.begin_step(None);
self.clear_study_queues();
}
#[inline]
pub(crate) fn save_undo(&mut self, item: Box<dyn Undo>) {
self.state.undo.save(item)
}
}
#[cfg(test)]
mod test {
use crate::card::Card;
use crate::collection::{open_test_collection, CollectionOp};
use crate::{collection::open_test_collection, prelude::*};
#[test]
fn undo() {
@ -188,7 +233,7 @@ mod test {
// record a few undo steps
for i in 3..=4 {
col.transact(Some(CollectionOp::UpdateCard), |col| {
col.transact(Some(UndoableOp::UpdateCard), |col| {
col.get_and_update_card(cid, |card| {
card.interval = i;
Ok(())
@ -200,41 +245,41 @@ mod test {
}
assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 4);
assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard));
assert_eq!(col.can_undo(), Some(UndoableOp::UpdateCard));
assert_eq!(col.can_redo(), None);
// undo a step
col.undo().unwrap();
assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 3);
assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard));
assert_eq!(col.can_redo(), Some(CollectionOp::UpdateCard));
assert_eq!(col.can_undo(), Some(UndoableOp::UpdateCard));
assert_eq!(col.can_redo(), Some(UndoableOp::UpdateCard));
// and again
col.undo().unwrap();
assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 2);
assert_eq!(col.can_undo(), None);
assert_eq!(col.can_redo(), Some(CollectionOp::UpdateCard));
assert_eq!(col.can_redo(), Some(UndoableOp::UpdateCard));
// redo a step
col.redo().unwrap();
assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 3);
assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard));
assert_eq!(col.can_redo(), Some(CollectionOp::UpdateCard));
assert_eq!(col.can_undo(), Some(UndoableOp::UpdateCard));
assert_eq!(col.can_redo(), Some(UndoableOp::UpdateCard));
// and another
col.redo().unwrap();
assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 4);
assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard));
assert_eq!(col.can_undo(), Some(UndoableOp::UpdateCard));
assert_eq!(col.can_redo(), None);
// and undo the redo
col.undo().unwrap();
assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 3);
assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard));
assert_eq!(col.can_redo(), Some(CollectionOp::UpdateCard));
assert_eq!(col.can_undo(), Some(UndoableOp::UpdateCard));
assert_eq!(col.can_redo(), Some(UndoableOp::UpdateCard));
// if any action is performed, it should clear the redo queue
col.transact(Some(CollectionOp::UpdateCard), |col| {
col.transact(Some(UndoableOp::UpdateCard), |col| {
col.get_and_update_card(cid, |card| {
card.interval = 5;
Ok(())
@ -244,7 +289,7 @@ mod test {
})
.unwrap();
assert_eq!(col.storage.get_card(cid).unwrap().unwrap().interval, 5);
assert_eq!(col.can_undo(), Some(CollectionOp::UpdateCard));
assert_eq!(col.can_undo(), Some(UndoableOp::UpdateCard));
assert_eq!(col.can_redo(), None);
// and any action that doesn't support undoing will clear both queues