Merge pull request #1200 from RumovZ/template-checks

Template checks

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

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)

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

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)

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)

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

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!("{}<br>{}", 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<regex::Error> 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,
 }

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<ParsedTemplate>, Option<ParsedTemplate>)],
+    ) -> 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<T, I>(&self, names: T) -> bool
+    where
+        T: IntoIterator<Item = I>,
+        I: AsRef<str>,
+    {
+        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<ParsedTemplate>, Option<ParsedTemplate>)],
+    ) -> 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<String, Option<String>>,
-        parsed: Vec<(Option<ParsedTemplate>, Option<ParsedTemplate>)>,
+        parsed: &mut [(Option<ParsedTemplate>, Option<ParsedTemplate>)],
     ) {
-        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<ParsedTemplate>, Option<ParsedTemplate>)],
+) -> 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<ParsedTemplate>, Option<ParsedTemplate>)],
+) -> Option<usize> {
+    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<ParsedTemplate>) -> bool {
+    template
+        .as_ref()
+        .map_or(false, |t| !t.cloze_fields().is_empty())
+}
+
 impl From<Notetype> for NotetypeProto {
     fn from(nt: Notetype) -> Self {
         NotetypeProto {

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<str>>,

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

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<String, Option<String>>,
-    ) -> ParsedTemplate {
-        let out = rename_and_remove_fields(self.0, fields);
-        ParsedTemplate(out)
+    pub(crate) fn rename_and_remove_fields(&mut self, fields: &HashMap<String, Option<String>>) {
+        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);
                 }
             }