From db9594818a2387f3a13766565dcd9c98affc0587 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Tue, 25 May 2021 20:58:43 +0200 Subject: [PATCH 01/14] Handle failure in CardLayout/accept() --- qt/aqt/clayout.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qt/aqt/clayout.py b/qt/aqt/clayout.py index 8b38c67c1..8b322de10 100644 --- a/qt/aqt/clayout.py +++ b/qt/aqt/clayout.py @@ -30,6 +30,7 @@ from aqt.utils import ( saveSplitter, shortcut, showInfo, + showWarning, tooltip, tr, ) @@ -791,7 +792,7 @@ class CardLayout(QDialog): update_notetype_legacy(parent=self.mw, notetype=self.model).success( on_done - ).run_in_background() + ).failure(lambda e: showWarning(str(e))).run_in_background() def reject(self) -> None: if self.change_tracker.changed(): From fa19f590e8cb4ac57720d1e69b58c4da4b952196 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Tue, 25 May 2021 21:01:03 +0200 Subject: [PATCH 02/14] Add details to TemplateSaveError --- ftl/core/card-templates.ftl | 1 + rslib/src/error/mod.rs | 17 ++++++++++++++--- rslib/src/notetype/mod.rs | 3 ++- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/ftl/core/card-templates.ftl b/ftl/core/card-templates.ftl index 544d6d3bd..6b0d8a1ef 100644 --- a/ftl/core/card-templates.ftl +++ b/ftl/core/card-templates.ftl @@ -21,6 +21,7 @@ card-templates-night-mode = Night Mode card-templates-add-mobile-class = Add Mobile Class card-templates-preview-settings = Options card-templates-invalid-template-number = Card template { $number } in notetype '{ $notetype }' has a problem. +card-templates-see-preview = See the render preview for more information. card-templates-changes-saved = Changes saved. card-templates-discard-changes = Discard changes? card-templates-add-card-type = Add Card Type... diff --git a/rslib/src/error/mod.rs b/rslib/src/error/mod.rs index 400a2a602..e623d94e2 100644 --- a/rslib/src/error/mod.rs +++ b/rslib/src/error/mod.rs @@ -61,9 +61,14 @@ impl AnkiError { // already localized info.into() } - AnkiError::TemplateSaveError(err) => tr - .card_templates_invalid_template_number(err.ordinal + 1, &err.notetype) - .into(), + AnkiError::TemplateSaveError(err) => { + let header = + tr.card_templates_invalid_template_number(err.ordinal + 1, &err.notetype); + let details = match err.details { + TemplateSaveErrorDetails::TemplateError => tr.card_templates_see_preview(), + }; + format!("{}
{}", header, details) + } AnkiError::DbError(err) => err.localized_description(tr), AnkiError::SearchError(kind) => kind.localized_description(&tr), AnkiError::InvalidInput(info) => { @@ -135,4 +140,10 @@ impl From for AnkiError { pub struct TemplateSaveError { pub notetype: String, pub ordinal: usize, + pub details: TemplateSaveErrorDetails, +} + +#[derive(Debug, PartialEq)] +pub enum TemplateSaveErrorDetails { + TemplateError, } diff --git a/rslib/src/notetype/mod.rs b/rslib/src/notetype/mod.rs index 052b3ddda..924de46e1 100644 --- a/rslib/src/notetype/mod.rs +++ b/rslib/src/notetype/mod.rs @@ -37,7 +37,7 @@ pub use crate::backend_proto::{ }; use crate::{ define_newtype, - error::TemplateSaveError, + error::{TemplateSaveError, TemplateSaveErrorDetails}, prelude::*, template::{FieldRequirements, ParsedTemplate}, text::ensure_string_in_nfc, @@ -330,6 +330,7 @@ impl Notetype { return Err(AnkiError::TemplateSaveError(TemplateSaveError { notetype: self.name.clone(), ordinal: idx, + details: TemplateSaveErrorDetails::TemplateError, })); } let reqs = self.updated_requirements(&parsed_templates); From 60131eab23fc5c161581353062c7731e29b7f094 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Tue, 25 May 2021 21:57:49 +0200 Subject: [PATCH 03/14] Check for identical templates before saving --- ftl/core/card-templates.ftl | 1 + rslib/src/error/mod.rs | 2 ++ rslib/src/notetype/mod.rs | 16 ++++++++++++++++ 3 files changed, 19 insertions(+) diff --git a/ftl/core/card-templates.ftl b/ftl/core/card-templates.ftl index 6b0d8a1ef..7a507fce8 100644 --- a/ftl/core/card-templates.ftl +++ b/ftl/core/card-templates.ftl @@ -21,6 +21,7 @@ card-templates-night-mode = Night Mode card-templates-add-mobile-class = Add Mobile Class card-templates-preview-settings = Options card-templates-invalid-template-number = Card template { $number } in notetype '{ $notetype }' has a problem. +card-templates-identical-front = Its front side is identical with the one of card template { $number }. card-templates-see-preview = See the render preview for more information. card-templates-changes-saved = Changes saved. card-templates-discard-changes = Discard changes? diff --git a/rslib/src/error/mod.rs b/rslib/src/error/mod.rs index e623d94e2..914074368 100644 --- a/rslib/src/error/mod.rs +++ b/rslib/src/error/mod.rs @@ -66,6 +66,7 @@ impl AnkiError { tr.card_templates_invalid_template_number(err.ordinal + 1, &err.notetype); let details = match err.details { TemplateSaveErrorDetails::TemplateError => tr.card_templates_see_preview(), + TemplateSaveErrorDetails::Duplicate(i) => tr.card_templates_identical_front(i), }; format!("{}
{}", header, details) } @@ -145,5 +146,6 @@ pub struct TemplateSaveError { #[derive(Debug, PartialEq)] pub enum TemplateSaveErrorDetails { + Duplicate(usize), TemplateError, } diff --git a/rslib/src/notetype/mod.rs b/rslib/src/notetype/mod.rs index 924de46e1..9e223d7af 100644 --- a/rslib/src/notetype/mod.rs +++ b/rslib/src/notetype/mod.rs @@ -272,6 +272,21 @@ impl Notetype { }); } + fn ensure_template_fronts_unique(&self) -> Result<()> { + for i in 1..self.templates.len() { + for j in 0..i { + if self.templates[i].config.q_format == self.templates[j].config.q_format { + return Err(AnkiError::TemplateSaveError(TemplateSaveError { + notetype: self.name.clone(), + ordinal: i, + details: TemplateSaveErrorDetails::Duplicate(j + 1), + })); + } + } + } + Ok(()) + } + pub(crate) fn normalize_names(&mut self) { ensure_string_in_nfc(&mut self.name); for f in &mut self.fields { @@ -314,6 +329,7 @@ impl Notetype { self.fix_template_names()?; self.ensure_names_unique(); self.reposition_sort_idx(); + self.ensure_template_fronts_unique()?; let parsed_templates = self.parsed_templates(); let invalid_card_idx = parsed_templates From 6fae0ea21f4c30818712299a2ec82c5321f3a2ed Mon Sep 17 00:00:00 2001 From: RumovZ Date: Tue, 25 May 2021 21:58:12 +0200 Subject: [PATCH 04/14] Update tests to avoid duplicate front templates --- pylib/tests/test_cards.py | 2 +- pylib/tests/test_schedv1.py | 2 +- pylib/tests/test_schedv2.py | 2 +- rslib/src/notetype/schemachange.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pylib/tests/test_cards.py b/pylib/tests/test_cards.py index 5728cf7e1..a4e9e9daf 100644 --- a/pylib/tests/test_cards.py +++ b/pylib/tests/test_cards.py @@ -45,7 +45,7 @@ def test_genrem(): mm = col.models # adding a new template should automatically create cards t = mm.newTemplate("rev") - t["qfmt"] = "{{Front}}" + t["qfmt"] = "{{Front}}2" t["afmt"] = "" mm.addTemplate(m, t) mm.save(m, templates=True) diff --git a/pylib/tests/test_schedv1.py b/pylib/tests/test_schedv1.py index 92a402b5f..0abb4d805 100644 --- a/pylib/tests/test_schedv1.py +++ b/pylib/tests/test_schedv1.py @@ -811,7 +811,7 @@ def test_ordcycle(): t["afmt"] = "{{Front}}" mm.addTemplate(m, t) t = mm.newTemplate("f2") - t["qfmt"] = "{{Front}}" + t["qfmt"] = "{{Front}}2" t["afmt"] = "{{Back}}" mm.addTemplate(m, t) mm.save(m) diff --git a/pylib/tests/test_schedv2.py b/pylib/tests/test_schedv2.py index 7624a9812..f25e20425 100644 --- a/pylib/tests/test_schedv2.py +++ b/pylib/tests/test_schedv2.py @@ -885,7 +885,7 @@ def test_ordcycle(): t["afmt"] = "{{Front}}" mm.addTemplate(m, t) t = mm.newTemplate("f2") - t["qfmt"] = "{{Front}}" + t["qfmt"] = "{{Front}}2" t["afmt"] = "{{Back}}" mm.addTemplate(m, t) mm.save(m) diff --git a/rslib/src/notetype/schemachange.rs b/rslib/src/notetype/schemachange.rs index 51e260cd5..033fbd494 100644 --- a/rslib/src/notetype/schemachange.rs +++ b/rslib/src/notetype/schemachange.rs @@ -311,7 +311,7 @@ mod test { ); // add an extra card template - nt.add_template("card 2", "{{Front}}", ""); + nt.add_template("card 2", "{{Front}}2", ""); col.update_notetype(&mut nt)?; assert_eq!( From 3dfa1de68b254c9fe3324c1ff0b361115c46b1bb Mon Sep 17 00:00:00 2001 From: RumovZ Date: Thu, 27 May 2021 12:01:05 +0200 Subject: [PATCH 05/14] Check for clozes when saving notetype Error if: - Cloze notetype lacks a cloze field on either template side. - Non-cloze notetype has a cloze field on any template. --- ftl/core/card-templates.ftl | 2 ++ rslib/src/error/mod.rs | 8 +++++- rslib/src/notetype/mod.rs | 52 +++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/ftl/core/card-templates.ftl b/ftl/core/card-templates.ftl index 7a507fce8..50e4e61b5 100644 --- a/ftl/core/card-templates.ftl +++ b/ftl/core/card-templates.ftl @@ -22,6 +22,8 @@ card-templates-add-mobile-class = Add Mobile Class card-templates-preview-settings = Options card-templates-invalid-template-number = Card template { $number } in notetype '{ $notetype }' has a problem. card-templates-identical-front = Its front side is identical with the one of card template { $number }. +card-templates-missing-cloze = The 'cloze' filter must be used on both sides of a cloze template. +card-templates-extraneous-cloze = The 'cloze' filter can only be used on cloze templates. card-templates-see-preview = See the render preview for more information. card-templates-changes-saved = Changes saved. card-templates-discard-changes = Discard changes? diff --git a/rslib/src/error/mod.rs b/rslib/src/error/mod.rs index 914074368..da6c48375 100644 --- a/rslib/src/error/mod.rs +++ b/rslib/src/error/mod.rs @@ -67,6 +67,10 @@ impl AnkiError { let details = match err.details { TemplateSaveErrorDetails::TemplateError => tr.card_templates_see_preview(), TemplateSaveErrorDetails::Duplicate(i) => tr.card_templates_identical_front(i), + TemplateSaveErrorDetails::MissingCloze => tr.card_templates_missing_cloze(), + TemplateSaveErrorDetails::ExtraneousCloze => { + tr.card_templates_extraneous_cloze() + } }; format!("{}
{}", header, details) } @@ -146,6 +150,8 @@ pub struct TemplateSaveError { #[derive(Debug, PartialEq)] pub enum TemplateSaveErrorDetails { - Duplicate(usize), TemplateError, + Duplicate(usize), + MissingCloze, + ExtraneousCloze, } diff --git a/rslib/src/notetype/mod.rs b/rslib/src/notetype/mod.rs index 9e223d7af..f472f27a7 100644 --- a/rslib/src/notetype/mod.rs +++ b/rslib/src/notetype/mod.rs @@ -287,6 +287,28 @@ impl Notetype { Ok(()) } + fn ensure_cloze_if_and_only_if_cloze_notetype( + &self, + parsed_templates: &[(Option, Option)], + ) -> Result<()> { + if self.is_cloze() { + if missing_cloze_filter(parsed_templates) { + return Err(AnkiError::TemplateSaveError(TemplateSaveError { + notetype: self.name.clone(), + ordinal: 0, + details: TemplateSaveErrorDetails::MissingCloze, + })); + } + } else if let Some(i) = find_cloze_filter(parsed_templates) { + return Err(AnkiError::TemplateSaveError(TemplateSaveError { + notetype: self.name.clone(), + ordinal: i, + details: TemplateSaveErrorDetails::ExtraneousCloze, + })); + } + Ok(()) + } + pub(crate) fn normalize_names(&mut self) { ensure_string_in_nfc(&mut self.name); for f in &mut self.fields { @@ -349,6 +371,7 @@ impl Notetype { details: TemplateSaveErrorDetails::TemplateError, })); } + self.ensure_cloze_if_and_only_if_cloze_notetype(&parsed_templates)?; let reqs = self.updated_requirements(&parsed_templates); // handle renamed+deleted fields @@ -448,6 +471,35 @@ impl Notetype { } } +/// True if the slice is empty or either template of the first tuple doesn't have a cloze field. +fn missing_cloze_filter( + parsed_templates: &[(Option, Option)], +) -> bool { + parsed_templates + .get(0) + .map_or(true, |t| !has_cloze(&t.0) || !has_cloze(&t.1)) +} + +/// Return the index of the first tuple with a cloze field on either template. +fn find_cloze_filter( + parsed_templates: &[(Option, Option)], +) -> Option { + parsed_templates.iter().enumerate().find_map(|(i, t)| { + if has_cloze(&t.0) || has_cloze(&t.1) { + Some(i) + } else { + None + } + }) +} + +/// True if the template is non-empty and has a cloze field. +fn has_cloze(template: &Option) -> bool { + template + .as_ref() + .map_or(false, |t| !t.cloze_fields().is_empty()) +} + impl From for NotetypeProto { fn from(nt: Notetype) -> Self { NotetypeProto { From c61587b1dec10c4943f80132bd43ceef4a397521 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Thu, 27 May 2021 12:45:17 +0200 Subject: [PATCH 06/14] Use HashMap in identical template check --- ftl/core/card-templates.ftl | 2 +- rslib/src/error/mod.rs | 4 +++- rslib/src/notetype/mod.rs | 25 ++++++++++++++----------- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/ftl/core/card-templates.ftl b/ftl/core/card-templates.ftl index 50e4e61b5..193be1b9c 100644 --- a/ftl/core/card-templates.ftl +++ b/ftl/core/card-templates.ftl @@ -21,7 +21,7 @@ card-templates-night-mode = Night Mode card-templates-add-mobile-class = Add Mobile Class card-templates-preview-settings = Options card-templates-invalid-template-number = Card template { $number } in notetype '{ $notetype }' has a problem. -card-templates-identical-front = Its front side is identical with the one of card template { $number }. +card-templates-identical-front = Its front side is identical to the one of card template { $number }. card-templates-missing-cloze = The 'cloze' filter must be used on both sides of a cloze template. card-templates-extraneous-cloze = The 'cloze' filter can only be used on cloze templates. card-templates-see-preview = See the render preview for more information. diff --git a/rslib/src/error/mod.rs b/rslib/src/error/mod.rs index da6c48375..3bb79c776 100644 --- a/rslib/src/error/mod.rs +++ b/rslib/src/error/mod.rs @@ -66,7 +66,9 @@ impl AnkiError { tr.card_templates_invalid_template_number(err.ordinal + 1, &err.notetype); let details = match err.details { TemplateSaveErrorDetails::TemplateError => tr.card_templates_see_preview(), - TemplateSaveErrorDetails::Duplicate(i) => tr.card_templates_identical_front(i), + TemplateSaveErrorDetails::Duplicate(i) => { + tr.card_templates_identical_front(i + 1) + } TemplateSaveErrorDetails::MissingCloze => tr.card_templates_missing_cloze(), TemplateSaveErrorDetails::ExtraneousCloze => { tr.card_templates_extraneous_cloze() diff --git a/rslib/src/notetype/mod.rs b/rslib/src/notetype/mod.rs index f472f27a7..9d1898468 100644 --- a/rslib/src/notetype/mod.rs +++ b/rslib/src/notetype/mod.rs @@ -273,18 +273,21 @@ impl Notetype { } fn ensure_template_fronts_unique(&self) -> Result<()> { - for i in 1..self.templates.len() { - for j in 0..i { - if self.templates[i].config.q_format == self.templates[j].config.q_format { - return Err(AnkiError::TemplateSaveError(TemplateSaveError { - notetype: self.name.clone(), - ordinal: i, - details: TemplateSaveErrorDetails::Duplicate(j + 1), - })); - } - } + let mut map = HashMap::new(); + if let Some((index_1, index_2)) = + self.templates.iter().enumerate().find_map(|(index, card)| { + map.insert(&card.config.q_format, index) + .map(|old_index| (old_index, index)) + }) + { + Err(AnkiError::TemplateSaveError(TemplateSaveError { + notetype: self.name.clone(), + ordinal: index_2, + details: TemplateSaveErrorDetails::Duplicate(index_1), + })) + } else { + Ok(()) } - Ok(()) } fn ensure_cloze_if_and_only_if_cloze_notetype( From e247da5ba841677a4830934402dc2322612aff86 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Thu, 27 May 2021 13:40:33 +0200 Subject: [PATCH 07/14] Apply suggestions from code review Tweak wording of template check errors. Co-authored-by: Damien Elmes --- ftl/core/card-templates.ftl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ftl/core/card-templates.ftl b/ftl/core/card-templates.ftl index 193be1b9c..10d947da7 100644 --- a/ftl/core/card-templates.ftl +++ b/ftl/core/card-templates.ftl @@ -21,10 +21,10 @@ card-templates-night-mode = Night Mode card-templates-add-mobile-class = Add Mobile Class card-templates-preview-settings = Options card-templates-invalid-template-number = Card template { $number } in notetype '{ $notetype }' has a problem. -card-templates-identical-front = Its front side is identical to the one of card template { $number }. -card-templates-missing-cloze = The 'cloze' filter must be used on both sides of a cloze template. -card-templates-extraneous-cloze = The 'cloze' filter can only be used on cloze templates. -card-templates-see-preview = See the render preview for more information. +card-templates-identical-front = The front side is identical to card template { $number }. +card-templates-missing-cloze = Expected to find '{{cloze:Text}}' or similar on the front and back of the card template. +card-templates-extraneous-cloze = 'cloze:' can only be used on cloze notetypes. +card-templates-see-preview = See the preview for more information. card-templates-changes-saved = Changes saved. card-templates-discard-changes = Discard changes? card-templates-add-card-type = Add Card Type... From f97b1559a2bbb7a11b575c519713d982f4349296 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Thu, 27 May 2021 16:46:33 +0200 Subject: [PATCH 08/14] Escape braces in fluent string --- ftl/core/card-templates.ftl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ftl/core/card-templates.ftl b/ftl/core/card-templates.ftl index 10d947da7..fff284210 100644 --- a/ftl/core/card-templates.ftl +++ b/ftl/core/card-templates.ftl @@ -22,7 +22,7 @@ card-templates-add-mobile-class = Add Mobile Class card-templates-preview-settings = Options card-templates-invalid-template-number = Card template { $number } in notetype '{ $notetype }' has a problem. card-templates-identical-front = The front side is identical to card template { $number }. -card-templates-missing-cloze = Expected to find '{{cloze:Text}}' or similar on the front and back of the card template. +card-templates-missing-cloze = Expected to find '{ "{{" }cloze:Text{ "}}" }' or similar on the front and back of the card template. card-templates-extraneous-cloze = 'cloze:' can only be used on cloze notetypes. card-templates-see-preview = See the preview for more information. card-templates-changes-saved = Changes saved. From 935fbb6289c5ab7c9bec03c8f676ab27dd478677 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Thu, 27 May 2021 16:51:03 +0200 Subject: [PATCH 09/14] Use implicit failure handling and self as parent --- qt/aqt/clayout.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/qt/aqt/clayout.py b/qt/aqt/clayout.py index 8b322de10..592b00965 100644 --- a/qt/aqt/clayout.py +++ b/qt/aqt/clayout.py @@ -30,7 +30,6 @@ from aqt.utils import ( saveSplitter, shortcut, showInfo, - showWarning, tooltip, tr, ) @@ -790,9 +789,9 @@ class CardLayout(QDialog): gui_hooks.sidebar_should_refresh_notetypes() QDialog.accept(self) - update_notetype_legacy(parent=self.mw, notetype=self.model).success( + update_notetype_legacy(parent=self, notetype=self.model).success( on_done - ).failure(lambda e: showWarning(str(e))).run_in_background() + ).run_in_background() def reject(self) -> None: if self.change_tracker.changed(): From 2e923db6bd58590627d1c94e13ff3edaac9cb227 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Fri, 28 May 2021 10:07:31 +0200 Subject: [PATCH 10/14] Add checks for parsed templates Combine existing check for unparsable templates with a check for unknown field names and a check for front sides without any field replacement. Updating the notetype's fields now mutates the parsed templates, so the checks can run on the final templates. --- ftl/core/card-templates.ftl | 1 + rslib/src/error/mod.rs | 6 ++- rslib/src/notetype/mod.rs | 99 ++++++++++++++++++++++++++----------- rslib/src/template.rs | 24 +++++---- 4 files changed, 91 insertions(+), 39 deletions(-) diff --git a/ftl/core/card-templates.ftl b/ftl/core/card-templates.ftl index fff284210..87652d405 100644 --- a/ftl/core/card-templates.ftl +++ b/ftl/core/card-templates.ftl @@ -22,6 +22,7 @@ card-templates-add-mobile-class = Add Mobile Class card-templates-preview-settings = Options card-templates-invalid-template-number = Card template { $number } in notetype '{ $notetype }' has a problem. card-templates-identical-front = The front side is identical to card template { $number }. +card-templates-no-front-field = Expected to find a field replacement on the front of the card template. card-templates-missing-cloze = Expected to find '{ "{{" }cloze:Text{ "}}" }' or similar on the front and back of the card template. card-templates-extraneous-cloze = 'cloze:' can only be used on cloze notetypes. card-templates-see-preview = See the preview for more information. diff --git a/rslib/src/error/mod.rs b/rslib/src/error/mod.rs index 3bb79c776..54af8e768 100644 --- a/rslib/src/error/mod.rs +++ b/rslib/src/error/mod.rs @@ -65,7 +65,9 @@ impl AnkiError { let header = tr.card_templates_invalid_template_number(err.ordinal + 1, &err.notetype); let details = match err.details { - TemplateSaveErrorDetails::TemplateError => tr.card_templates_see_preview(), + TemplateSaveErrorDetails::TemplateError + | TemplateSaveErrorDetails::NoSuchField => tr.card_templates_see_preview(), + TemplateSaveErrorDetails::NoFrontField => tr.card_templates_no_front_field(), TemplateSaveErrorDetails::Duplicate(i) => { tr.card_templates_identical_front(i + 1) } @@ -154,6 +156,8 @@ pub struct TemplateSaveError { pub enum TemplateSaveErrorDetails { TemplateError, Duplicate(usize), + NoFrontField, + NoSuchField, MissingCloze, ExtraneousCloze, } diff --git a/rslib/src/notetype/mod.rs b/rslib/src/notetype/mod.rs index 9d1898468..7dbb33845 100644 --- a/rslib/src/notetype/mod.rs +++ b/rslib/src/notetype/mod.rs @@ -290,6 +290,63 @@ impl Notetype { } } + /// Ensure no templates are None, every front template contains at least one + /// field, and all used field names belong to a field of this notetype. + fn ensure_valid_parsed_templates( + &self, + templates: &[(Option, Option)], + ) -> Result<()> { + if let Some((invalid_index, details)) = + templates.iter().enumerate().find_map(|(index, sides)| { + if let (Some(q), Some(a)) = sides { + let q_fields = q.fields(); + if q_fields.is_empty() { + Some((index, TemplateSaveErrorDetails::NoFrontField)) + } else if self.unknown_field_name(q_fields.union(&a.fields())) { + Some((index, TemplateSaveErrorDetails::NoSuchField)) + } else { + None + } + } else { + Some((index, TemplateSaveErrorDetails::TemplateError)) + } + }) + { + Err(AnkiError::TemplateSaveError(TemplateSaveError { + notetype: self.name.clone(), + ordinal: invalid_index, + details, + })) + } else { + Ok(()) + } + } + + /// True if any name in names does not belong to a field of this notetype. + fn unknown_field_name(&self, names: T) -> bool + where + T: IntoIterator, + I: AsRef, + { + let special_fields = [ + "FrontSide", + "Card", + "CardFlag", + "Deck", + "Subdeck", + "Tags", + "Type", + ]; + names.into_iter().any(|name| { + !special_fields.contains(&name.as_ref()) + && self + .fields + .iter() + .map(|field| &field.name) + .all(|field_name| field_name != name.as_ref()) + }) + } + fn ensure_cloze_if_and_only_if_cloze_notetype( &self, parsed_templates: &[(Option, Option)], @@ -354,37 +411,21 @@ impl Notetype { self.fix_template_names()?; self.ensure_names_unique(); self.reposition_sort_idx(); - self.ensure_template_fronts_unique()?; - let parsed_templates = self.parsed_templates(); - let invalid_card_idx = parsed_templates - .iter() - .enumerate() - .find_map(|(idx, (q, a))| { - if q.is_none() || a.is_none() { - Some(idx) - } else { - None - } - }); - if let Some(idx) = invalid_card_idx { - return Err(AnkiError::TemplateSaveError(TemplateSaveError { - notetype: self.name.clone(), - ordinal: idx, - details: TemplateSaveErrorDetails::TemplateError, - })); - } - self.ensure_cloze_if_and_only_if_cloze_notetype(&parsed_templates)?; + let mut parsed_templates = self.parsed_templates(); let reqs = self.updated_requirements(&parsed_templates); // handle renamed+deleted fields if let Some(existing) = existing { let fields = self.renamed_and_removed_fields(existing); if !fields.is_empty() { - self.update_templates_for_renamed_and_removed_fields(fields, parsed_templates); + self.update_templates_for_renamed_and_removed_fields(fields, &mut parsed_templates); } } self.config.reqs = reqs; + self.ensure_template_fronts_unique()?; + self.ensure_valid_parsed_templates(&parsed_templates)?; + self.ensure_cloze_if_and_only_if_cloze_notetype(&parsed_templates)?; Ok(()) } @@ -422,16 +463,16 @@ impl Notetype { fn update_templates_for_renamed_and_removed_fields( &mut self, fields: HashMap>, - parsed: Vec<(Option, Option)>, + parsed: &mut [(Option, Option)], ) { - for (idx, (q, a)) in parsed.into_iter().enumerate() { - if let Some(q) = q { - let updated = q.rename_and_remove_fields(&fields); - self.templates[idx].config.q_format = updated.template_to_string(); + for (idx, (q_opt, a_opt)) in parsed.iter_mut().enumerate() { + if let Some(q) = q_opt { + q.rename_and_remove_fields(&fields); + self.templates[idx].config.q_format = q.template_to_string(); } - if let Some(a) = a { - let updated = a.rename_and_remove_fields(&fields); - self.templates[idx].config.a_format = updated.template_to_string(); + if let Some(a) = a_opt { + a.rename_and_remove_fields(&fields); + self.templates[idx].config.a_format = a.template_to_string(); } } } diff --git a/rslib/src/template.rs b/rslib/src/template.rs index 338558139..f4fec2b24 100644 --- a/rslib/src/template.rs +++ b/rslib/src/template.rs @@ -655,12 +655,9 @@ impl ParsedTemplate { impl ParsedTemplate { /// Given a map of old to new field names, update references to the new names. /// Returns true if any changes made. - pub(crate) fn rename_and_remove_fields( - self, - fields: &HashMap>, - ) -> ParsedTemplate { - let out = rename_and_remove_fields(self.0, fields); - ParsedTemplate(out) + pub(crate) fn rename_and_remove_fields(&mut self, fields: &HashMap>) { + let old_nodes = std::mem::replace(&mut self.0, vec![]); + self.0 = rename_and_remove_fields(old_nodes, fields); } } @@ -765,25 +762,34 @@ fn nodes_to_string(buf: &mut String, nodes: &[ParsedNode]) { //---------------------------------------- impl ParsedTemplate { + /// A set of all field names. Field names may not be valid. + pub(crate) fn fields(&self) -> HashSet<&str> { + let mut set = HashSet::new(); + find_fields_with_filter(&self.0, &mut set, None); + set + } + /// A set of field names with a cloze filter attached. /// Field names may not be valid. pub(crate) fn cloze_fields(&self) -> HashSet<&str> { let mut set = HashSet::new(); - find_fields_with_filter(&self.0, &mut set, "cloze"); + find_fields_with_filter(&self.0, &mut set, Some("cloze")); set } } +/// Insert all fields in 'nodes' with 'filter' into 'fields'. If 'filter' is None, +/// all fields are collected. fn find_fields_with_filter<'a>( nodes: &'a [ParsedNode], fields: &mut HashSet<&'a str>, - filter: &str, + filter: Option<&str>, ) { for node in nodes { match node { ParsedNode::Text(_) => {} ParsedNode::Replacement { key, filters } => { - if filters.iter().any(|f| f == filter) { + if filter.is_none() || filters.iter().any(|f| f == filter.unwrap()) { fields.insert(key); } } From c6e78e6f21aeb7daa7626e1a46ef3604db74ba1e Mon Sep 17 00:00:00 2001 From: RumovZ Date: Fri, 28 May 2021 10:08:55 +0200 Subject: [PATCH 11/14] Adjusts tests to pass new template checks --- pylib/tests/test_collection.py | 4 ---- pylib/tests/test_models.py | 3 ++- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/pylib/tests/test_collection.py b/pylib/tests/test_collection.py index 95b941708..c3a0225c2 100644 --- a/pylib/tests/test_collection.py +++ b/pylib/tests/test_collection.py @@ -143,10 +143,6 @@ def test_furigana(): n["Front"] = "foo[sound:abc.mp3]" n.flush() assert "anki:play" in c.q(reload=True) - # it shouldn't throw an error while people are editing - m["tmpls"][0]["qfmt"] = "{{kana:}}" - mm.save(m) - c.q(reload=True) def test_translate(): diff --git a/pylib/tests/test_models.py b/pylib/tests/test_models.py index a5f947d88..a1bc0d4be 100644 --- a/pylib/tests/test_models.py +++ b/pylib/tests/test_models.py @@ -115,6 +115,7 @@ def test_templates(): assert stripHTML(c.q()) == "1" # it shouldn't be possible to orphan notes by removing templates t = mm.newTemplate("template name") + t["qfmt"] = "{{Front}}2" mm.addTemplate(m, t) col.models.remTemplate(m, m["tmpls"][0]) assert ( @@ -391,7 +392,7 @@ def test_req(): mm.save(opt, templates=True) assert opt["req"][1] == [1, "any", [1, 2]] # testing None - opt["tmpls"][1]["qfmt"] = "{{^Add Reverse}}{{/Add Reverse}}" + opt["tmpls"][1]["qfmt"] = "{{^Add Reverse}}{{Tags}}{{/Add Reverse}}" mm.save(opt, templates=True) assert opt["req"][1] == [1, "none", []] From 33bf39111441f50df691d8cb3531bcabbbc6dfa3 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Fri, 28 May 2021 10:35:07 +0200 Subject: [PATCH 12/14] Allow empty field name in templates --- pylib/tests/test_collection.py | 4 ++++ rslib/src/notetype/mod.rs | 10 ++++------ 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pylib/tests/test_collection.py b/pylib/tests/test_collection.py index c3a0225c2..95b941708 100644 --- a/pylib/tests/test_collection.py +++ b/pylib/tests/test_collection.py @@ -143,6 +143,10 @@ def test_furigana(): n["Front"] = "foo[sound:abc.mp3]" n.flush() assert "anki:play" in c.q(reload=True) + # it shouldn't throw an error while people are editing + m["tmpls"][0]["qfmt"] = "{{kana:}}" + mm.save(m) + c.q(reload=True) def test_translate(): diff --git a/rslib/src/notetype/mod.rs b/rslib/src/notetype/mod.rs index 7dbb33845..a9698c2dd 100644 --- a/rslib/src/notetype/mod.rs +++ b/rslib/src/notetype/mod.rs @@ -338,12 +338,10 @@ impl Notetype { "Type", ]; names.into_iter().any(|name| { - !special_fields.contains(&name.as_ref()) - && self - .fields - .iter() - .map(|field| &field.name) - .all(|field_name| field_name != name.as_ref()) + // The empty field name is allowed as it may be used by add-ons. + !name.as_ref().is_empty() + && !special_fields.contains(&name.as_ref()) + && self.fields.iter().all(|field| field.name != name.as_ref()) }) } From 329f89c0934819209dc09f78ec371c4bb2ce0970 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Fri, 28 May 2021 11:58:46 +0200 Subject: [PATCH 13/14] Add const for special fields and doc --- rslib/src/notetype/mod.rs | 24 +++++++++++++----------- rslib/src/notetype/render.rs | 4 +++- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/rslib/src/notetype/mod.rs b/rslib/src/notetype/mod.rs index a9698c2dd..e3605810f 100644 --- a/rslib/src/notetype/mod.rs +++ b/rslib/src/notetype/mod.rs @@ -48,6 +48,16 @@ define_newtype!(NotetypeId, i64); pub(crate) const DEFAULT_CSS: &str = include_str!("styling.css"); pub(crate) const DEFAULT_LATEX_HEADER: &str = include_str!("header.tex"); pub(crate) const DEFAULT_LATEX_FOOTER: &str = r"\end{document}"; +/// New entries must be handled in render.rs/add_special_fields(). +pub(crate) const SPECIAL_FIELDS: [&str; 7] = [ + "FrontSide", + "Card", + "CardFlag", + "Deck", + "Subdeck", + "Tags", + "Type", +]; #[derive(Debug, PartialEq, Clone)] pub struct Notetype { @@ -322,25 +332,17 @@ impl Notetype { } } - /// True if any name in names does not belong to a field of this notetype. + /// True if any non-empty name in names does not denote a special field or + /// a field of this notetype. fn unknown_field_name(&self, names: T) -> bool where T: IntoIterator, I: AsRef, { - let special_fields = [ - "FrontSide", - "Card", - "CardFlag", - "Deck", - "Subdeck", - "Tags", - "Type", - ]; names.into_iter().any(|name| { // The empty field name is allowed as it may be used by add-ons. !name.as_ref().is_empty() - && !special_fields.contains(&name.as_ref()) + && !SPECIAL_FIELDS.contains(&name.as_ref()) && self.fields.iter().all(|field| field.name != name.as_ref()) }) } diff --git a/rslib/src/notetype/render.rs b/rslib/src/notetype/render.rs index e232fa68c..b5538c16c 100644 --- a/rslib/src/notetype/render.rs +++ b/rslib/src/notetype/render.rs @@ -129,7 +129,9 @@ impl Collection { }) } - // Add special fields if they don't clobber note fields + /// Add special fields if they don't clobber note fields. + /// The fields supported here must coincide with SPECIAL_FIELDS in + /// notetype/mod.rs, apart from FrontSide which is handled by Python. fn add_special_fields( &mut self, map: &mut HashMap<&str, Cow>, From 44506136e2f3af1cf96ecf2e86cc80296c31737d Mon Sep 17 00:00:00 2001 From: RumovZ Date: Fri, 28 May 2021 15:42:09 +0200 Subject: [PATCH 14/14] Use HashSet for special fields --- rslib/src/notetype/mod.rs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/rslib/src/notetype/mod.rs b/rslib/src/notetype/mod.rs index e3605810f..cce6da6d9 100644 --- a/rslib/src/notetype/mod.rs +++ b/rslib/src/notetype/mod.rs @@ -11,8 +11,10 @@ mod stock; mod templates; pub(crate) mod undo; +use lazy_static::lazy_static; use std::{ collections::{HashMap, HashSet}, + iter::FromIterator, sync::Arc, }; @@ -48,16 +50,18 @@ define_newtype!(NotetypeId, i64); pub(crate) const DEFAULT_CSS: &str = include_str!("styling.css"); pub(crate) const DEFAULT_LATEX_HEADER: &str = include_str!("header.tex"); pub(crate) const DEFAULT_LATEX_FOOTER: &str = r"\end{document}"; -/// New entries must be handled in render.rs/add_special_fields(). -pub(crate) const SPECIAL_FIELDS: [&str; 7] = [ - "FrontSide", - "Card", - "CardFlag", - "Deck", - "Subdeck", - "Tags", - "Type", -]; +lazy_static! { + /// New entries must be handled in render.rs/add_special_fields(). + static ref SPECIAL_FIELDS: HashSet<&'static str> = HashSet::from_iter(vec![ + "FrontSide", + "Card", + "CardFlag", + "Deck", + "Subdeck", + "Tags", + "Type", + ]); +} #[derive(Debug, PartialEq, Clone)] pub struct Notetype {