diff --git a/ftl/core/card-templates.ftl b/ftl/core/card-templates.ftl index 544d6d3bd..87652d405 100644 --- a/ftl/core/card-templates.ftl +++ b/ftl/core/card-templates.ftl @@ -21,6 +21,11 @@ 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 = 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. card-templates-changes-saved = Changes saved. card-templates-discard-changes = Discard changes? card-templates-add-card-type = Add Card Type... 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_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", []] 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 70993aa0d..5184e112a 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/qt/aqt/clayout.py b/qt/aqt/clayout.py index 8b38c67c1..592b00965 100644 --- a/qt/aqt/clayout.py +++ b/qt/aqt/clayout.py @@ -789,7 +789,7 @@ 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 ).run_in_background() diff --git a/rslib/src/error/mod.rs b/rslib/src/error/mod.rs index 400a2a602..54af8e768 100644 --- a/rslib/src/error/mod.rs +++ b/rslib/src/error/mod.rs @@ -61,9 +61,23 @@ 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 + | 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) + } + TemplateSaveErrorDetails::MissingCloze => tr.card_templates_missing_cloze(), + TemplateSaveErrorDetails::ExtraneousCloze => { + tr.card_templates_extraneous_cloze() + } + }; + format!("{}
{}", header, details) + } AnkiError::DbError(err) => err.localized_description(tr), AnkiError::SearchError(kind) => kind.localized_description(&tr), AnkiError::InvalidInput(info) => { @@ -135,4 +149,15 @@ impl From for AnkiError { pub struct TemplateSaveError { pub notetype: String, pub ordinal: usize, + pub details: TemplateSaveErrorDetails, +} + +#[derive(Debug, PartialEq)] +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 052b3ddda..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, }; @@ -37,7 +39,7 @@ pub use crate::backend_proto::{ }; use crate::{ define_newtype, - error::TemplateSaveError, + error::{TemplateSaveError, TemplateSaveErrorDetails}, prelude::*, template::{FieldRequirements, ParsedTemplate}, text::ensure_string_in_nfc, @@ -48,6 +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}"; +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 { @@ -272,6 +286,93 @@ impl Notetype { }); } + fn ensure_template_fronts_unique(&self) -> Result<()> { + 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(()) + } + } + + /// 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 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, + { + 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()) + && self.fields.iter().all(|field| field.name != name.as_ref()) + }) + } + + 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 { @@ -315,33 +416,20 @@ impl Notetype { self.ensure_names_unique(); self.reposition_sort_idx(); - 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, - })); - } + 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(()) } @@ -379,16 +467,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(); } } } @@ -431,6 +519,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 { 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>, 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!( diff --git a/rslib/src/template.rs b/rslib/src/template.rs index d2618d8bb..e6994f4fd 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); } }