Template err improvements (#1953)

* Throw error for unknown condition fields as well

So if 'foo' is not a field, refuse to save a template containing
`{{#foo}}bar{{/foo}}`. Previously, only `{{foo}}` would be checked.
As a side effect, templates which *only* contain fields as conditions
may be saved. Meh.

* Display template errors in q/a columns only

So the affected browser row remains active and the user can fix the
template more easily.

* Specify if error occured in a browser template

* Minor wording tweak (dae)

There's an argument for using the exact wording as well, but this just
reads a little more naturally to me.

ftl/core/card-template-rendering.ftl

@@ -6,6 +6,8 @@
 card-template-rendering-more-info = More information
 card-template-rendering-front-side-problem = Front template has a problem:
 card-template-rendering-back-side-problem = Back template has a problem:
+card-template-rendering-browser-front-side-problem = Browser-specific front template has a problem:
+card-template-rendering-browser-back-side-problem = Browser-specific back template has a problem:
 # when the user forgot to close a field reference,
 # eg, Missing '}}' in '{{Field'
 card-template-rendering-no-closing-brackets = Missing '{ $missing }' in '{ $tag }'

rslib/src/browser_table.rs

@@ -64,14 +64,18 @@ struct RowContext {
     original_deck: Option<Arc<Deck>>,
     tr: I18n,
     timing: SchedTimingToday,
-    render_context: Option<RenderContext>,
+    render_context: RenderContext,
 }
 
-/// The answer string needs the question string but not the other way around, so only build the
+enum RenderContext {
-/// answer string when needed.
+    // The answer string needs the question string, but not the other way around,
-struct RenderContext {
+    // so only build the answer string when needed.
+    Ok {
         question: String,
         answer_nodes: Vec<RenderedNode>,
+    },
+    Err(String),
+    Unset,
 }
 
 fn card_render_required(columns: &[Column]) -> bool {
@@ -251,33 +255,49 @@ impl Collection {
 }
 
 impl RenderContext {
-    fn new(col: &mut Collection, card: &Card, note: &Note, notetype: &Notetype) -> Result<Self> {
+    fn new(col: &mut Collection, card: &Card, note: &Note, notetype: &Notetype) -> Self {
-        let render = col.render_card(
+        match notetype
-            note,
+            .get_template(card.template_idx)
-            card,
+            .and_then(|template| col.render_card(note, card, notetype, template, true))
-            notetype,
+        {
-            notetype.get_template(card.template_idx)?,
+            Ok(render) => RenderContext::Ok {
-            true,
+                question: rendered_nodes_to_str(&render.qnodes),
-        )?;
+                answer_nodes: render.anodes,
-        let qnodes_text = render
+            },
-            .qnodes
+            Err(err) => RenderContext::Err(err.localized_description(&col.tr)),
+        }
+    }
+
+    fn side_str(&self, is_answer: bool) -> String {
+        let back;
+        let html = match self {
+            Self::Ok {
+                question,
+                answer_nodes,
+            } => {
+                if is_answer {
+                    back = rendered_nodes_to_str(answer_nodes);
+                    back.strip_prefix(question).unwrap_or(&back)
+                } else {
+                    question
+                }
+            }
+            Self::Err(err) => err,
+            Self::Unset => "Invalid input: RenderContext unset",
+        };
+        html_to_text_line(html, true).into()
+    }
+}
+
+fn rendered_nodes_to_str(nodes: &[RenderedNode]) -> String {
+    let txt = nodes
         .iter()
         .map(|node| match node {
             RenderedNode::Text { text } => text,
-                RenderedNode::Replacement {
+            RenderedNode::Replacement { current_text, .. } => current_text,
-                    field_name: _,
-                    current_text,
-                    filters: _,
-                } => current_text,
         })
         .join("");
-        let question = prettify_av_tags(qnodes_text);
+    prettify_av_tags(txt)
-
-        Ok(RenderContext {
-            question,
-            answer_nodes: render.anodes,
-        })
-    }
 }
 
 impl RowContext {
@@ -324,9 +344,9 @@ impl RowContext {
         };
         let timing = col.timing_today()?;
         let render_context = if with_card_render {
-            Some(RenderContext::new(col, &cards[0], &note, &notetype)?)
+            RenderContext::new(col, &cards[0], &note, &notetype)
         } else {
-            None
+            RenderContext::Unset
         };
 
         Ok(RowContext {
@@ -363,8 +383,8 @@ impl RowContext {
 
     fn get_cell_text(&self, column: Column) -> Result<String> {
         Ok(match column {
-            Column::Question => self.question_str(),
+            Column::Question => self.render_context.side_str(false),
-            Column::Answer => self.answer_str(),
+            Column::Answer => self.render_context.side_str(true),
             Column::Deck => self.deck_str(),
             Column::Due => self.due_str(),
             Column::Ease => self.ease_str(),
@@ -405,32 +425,6 @@ impl RowContext {
         self.notetype.get_template(self.cards[0].template_idx)
     }
 
-    fn answer_str(&self) -> String {
-        let render_context = self.render_context.as_ref().unwrap();
-        let answer = render_context
-            .answer_nodes
-            .iter()
-            .map(|node| match node {
-                RenderedNode::Text { text } => text,
-                RenderedNode::Replacement {
-                    field_name: _,
-                    current_text,
-                    filters: _,
-                } => current_text,
-            })
-            .join("");
-        let answer = prettify_av_tags(answer);
-        html_to_text_line(
-            if let Some(stripped) = answer.strip_prefix(&render_context.question) {
-                stripped
-            } else {
-                &answer
-            },
-            true,
-        )
-        .to_string()
-    }
-
     fn due_str(&self) -> String {
         if self.notes_mode {
             self.note_due_str()
@@ -514,7 +508,7 @@ impl RowContext {
             .iter()
             .map(|c| c.mtime)
             .max()
-            .unwrap()
+            .expect("cards missing from RowContext")
             .date_string()
     }
 
@@ -545,10 +539,6 @@ impl RowContext {
         })
     }
 
-    fn question_str(&self) -> String {
-        html_to_text_line(&self.render_context.as_ref().unwrap().question, true).to_string()
-    }
-
     fn get_row_font_name(&self) -> Result<String> {
         Ok(self.template()?.config.browser_font_name.to_owned())
     }

rslib/src/notetype/mod.rs

@@ -391,10 +391,12 @@ impl Notetype {
         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();
+                    let q_fields = q.all_referenced_field_names();
                     if q_fields.is_empty() {
                         Some((index, CardTypeErrorDetails::NoFrontField))
-                    } else if self.unknown_field_name(q_fields.union(&a.fields())) {
+                    } else if self
+                        .unknown_field_name(q_fields.union(&a.all_referenced_field_names()))
+                    {
                         Some((index, CardTypeErrorDetails::NoSuchField))
                     } else {
                         None
@@ -601,7 +603,7 @@ impl Notetype {
             HashSet::new()
         } else if let Some((Some(front), _)) = self.parsed_templates().get(0) {
             front
-                .cloze_fields()
+                .all_referenced_cloze_field_names()
                 .iter()
                 .filter_map(|name| self.get_field_ord(name))
                 .collect()
@@ -624,7 +626,7 @@ fn missing_cloze_filter(
 fn has_cloze(template: &Option<ParsedTemplate>) -> bool {
     template
         .as_ref()
-        .map_or(false, |t| !t.cloze_fields().is_empty())
+        .map_or(false, |t| !t.all_referenced_cloze_field_names().is_empty())
 }
 
 impl From<Notetype> for NotetypeProto {

rslib/src/notetype/render.rs

@@ -118,6 +118,7 @@ impl Collection {
             &field_map,
             card.template_idx,
             nt.is_cloze(),
+            browser,
             &self.tr,
         )?;
         Ok(RenderCardOutput {
@@ -172,7 +173,7 @@ fn flag_name(n: u8) -> &'static str {
 
 fn fill_empty_fields(note: &mut Note, qfmt: &str, nt: &Notetype, tr: &I18n) {
     if let Ok(tmpl) = ParsedTemplate::from_text(qfmt) {
-        let cloze_fields = tmpl.cloze_fields();
+        let cloze_fields = tmpl.all_referenced_cloze_field_names();
 
         for (val, field) in note.fields_mut().iter_mut().zip(nt.fields.iter()) {
             if field_is_empty(val) {

rslib/src/template.rs

@@ -254,11 +254,17 @@ fn parse_inner<'a, I: Iterator<Item = TemplateResult<Token<'a>>>>(
     }
 }
 
-fn template_error_to_anki_error(err: TemplateError, q_side: bool, tr: &I18n) -> AnkiError {
+fn template_error_to_anki_error(
-    let header = if q_side {
+    err: TemplateError,
-        tr.card_template_rendering_front_side_problem()
+    q_side: bool,
-    } else {
+    browser: bool,
-        tr.card_template_rendering_back_side_problem()
+    tr: &I18n,
+) -> AnkiError {
+    let header = match (q_side, browser) {
+        (true, false) => tr.card_template_rendering_front_side_problem(),
+        (false, false) => tr.card_template_rendering_back_side_problem(),
+        (true, true) => tr.card_template_rendering_browser_front_side_problem(),
+        (false, true) => tr.card_template_rendering_browser_back_side_problem(),
     };
     let details = htmlescape::encode_minimal(&localized_template_error(tr, err));
     let more_info = tr.card_template_rendering_more_info();
@@ -569,6 +575,7 @@ pub fn render_card(
     field_map: &HashMap<&str, Cow<str>>,
     card_ord: u16,
     is_cloze: bool,
+    browser: bool,
     tr: &I18n,
 ) -> Result<(Vec<RenderedNode>, Vec<RenderedNode>)> {
     // prepare context
@@ -582,7 +589,7 @@ pub fn render_card(
     // question side
     let (mut qnodes, qtmpl) = ParsedTemplate::from_text(qfmt)
         .and_then(|tmpl| Ok((tmpl.render(&context, tr)?, tmpl)))
-        .map_err(|e| template_error_to_anki_error(e, true, tr))?;
+        .map_err(|e| template_error_to_anki_error(e, true, browser, tr))?;
 
     // check if the front side was empty
     let empty_message = if is_cloze && cloze_is_empty(field_map, card_ord) {
@@ -611,7 +618,7 @@ pub fn render_card(
     context.question_side = false;
     let anodes = ParsedTemplate::from_text(afmt)
         .and_then(|tmpl| tmpl.render(&context, tr))
-        .map_err(|e| template_error_to_anki_error(e, false, tr))?;
+        .map_err(|e| template_error_to_anki_error(e, false, browser, tr))?;
 
     Ok((qnodes, anodes))
 }
@@ -790,42 +797,41 @@ 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> {
+    pub(crate) fn all_referenced_field_names(&self) -> HashSet<&str> {
         let mut set = HashSet::new();
-        find_fields_with_filter(&self.0, &mut set, Some("cloze"));
+        find_field_references(&self.0, &mut set, false, true);
+        set
+    }
+
+    /// Field names may not be valid.
+    pub(crate) fn all_referenced_cloze_field_names(&self) -> HashSet<&str> {
+        let mut set = HashSet::new();
+        find_field_references(&self.0, &mut set, true, false);
         set
     }
 }
 
-/// Insert all fields in 'nodes' with 'filter' into 'fields'. If 'filter' is None,
+fn find_field_references<'a>(
-/// all fields are collected.
-fn find_fields_with_filter<'a>(
     nodes: &'a [ParsedNode],
     fields: &mut HashSet<&'a str>,
-    filter: Option<&str>,
+    cloze_only: bool,
+    with_conditionals: bool,
 ) {
     for node in nodes {
         match node {
             ParsedNode::Text(_) => {}
             ParsedNode::Replacement { key, filters } => {
-                if filter.is_none() || filters.iter().any(|f| f == filter.unwrap()) {
+                if !cloze_only || filters.iter().any(|f| f == "cloze") {
                     fields.insert(key);
                 }
             }
-            ParsedNode::Conditional { children, .. } => {
+            ParsedNode::Conditional { key, children }
-                find_fields_with_filter(children, fields, filter);
+            | ParsedNode::NegatedConditional { key, children } => {
+                if with_conditionals {
+                    fields.insert(key);
                 }
-            ParsedNode::NegatedConditional { children, .. } => {
+                find_field_references(children, fields, cloze_only, with_conditionals);
-                find_fields_with_filter(children, fields, filter);
             }
         }
     }
@@ -1168,7 +1174,7 @@ mod test {
         let tr = I18n::template_only();
         use crate::template::RenderedNode as FN;
 
-        let qnodes = super::render_card("test{{E}}", "", &map, 1, false, &tr)
+        let qnodes = super::render_card("test{{E}}", "", &map, 1, false, false, &tr)
             .unwrap()
             .0;
         assert_eq!(