From 8c515e316ee2d63f570051782c31bc45a3821cd6 Mon Sep 17 00:00:00 2001 From: RumovZ Date: Sat, 9 Jul 2022 05:00:03 +0200 Subject: [PATCH] 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 | 2 + rslib/src/browser_table.rs | 124 ++++++++++++--------------- rslib/src/notetype/mod.rs | 10 ++- rslib/src/notetype/render.rs | 3 +- rslib/src/template.rs | 54 ++++++------ 5 files changed, 97 insertions(+), 96 deletions(-) diff --git a/ftl/core/card-template-rendering.ftl b/ftl/core/card-template-rendering.ftl index bff3327a0..534f40a8d 100644 --- a/ftl/core/card-template-rendering.ftl +++ b/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 }' diff --git a/rslib/src/browser_table.rs b/rslib/src/browser_table.rs index 28b83fa76..531a7d424 100644 --- a/rslib/src/browser_table.rs +++ b/rslib/src/browser_table.rs @@ -64,14 +64,18 @@ struct RowContext { original_deck: Option>, tr: I18n, timing: SchedTimingToday, - render_context: Option, + render_context: RenderContext, } -/// The answer string needs the question string but not the other way around, so only build the -/// answer string when needed. -struct RenderContext { - question: String, - answer_nodes: Vec, +enum RenderContext { + // The answer string needs the question string, but not the other way around, + // so only build the answer string when needed. + Ok { + question: String, + answer_nodes: Vec, + }, + 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 { - let render = col.render_card( - note, - card, - notetype, - notetype.get_template(card.template_idx)?, - true, - )?; - let qnodes_text = render - .qnodes - .iter() - .map(|node| match node { - RenderedNode::Text { text } => text, - RenderedNode::Replacement { - field_name: _, - current_text, - filters: _, - } => current_text, - }) - .join(""); - let question = prettify_av_tags(qnodes_text); - - Ok(RenderContext { - question, - answer_nodes: render.anodes, - }) + fn new(col: &mut Collection, card: &Card, note: &Note, notetype: &Notetype) -> Self { + match notetype + .get_template(card.template_idx) + .and_then(|template| col.render_card(note, card, notetype, template, true)) + { + Ok(render) => RenderContext::Ok { + question: rendered_nodes_to_str(&render.qnodes), + answer_nodes: render.anodes, + }, + 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 { current_text, .. } => current_text, + }) + .join(""); + prettify_av_tags(txt) } 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], ¬e, ¬etype)?) + RenderContext::new(col, &cards[0], ¬e, ¬etype) } else { - None + RenderContext::Unset }; Ok(RowContext { @@ -363,8 +383,8 @@ impl RowContext { fn get_cell_text(&self, column: Column) -> Result { Ok(match column { - Column::Question => self.question_str(), - Column::Answer => self.answer_str(), + Column::Question => self.render_context.side_str(false), + 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 { Ok(self.template()?.config.browser_font_name.to_owned()) } diff --git a/rslib/src/notetype/mod.rs b/rslib/src/notetype/mod.rs index be45b6eba..e450b7b5f 100644 --- a/rslib/src/notetype/mod.rs +++ b/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) -> 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 for NotetypeProto { diff --git a/rslib/src/notetype/render.rs b/rslib/src/notetype/render.rs index 344d9ba1f..2ef54cb07 100644 --- a/rslib/src/notetype/render.rs +++ b/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) { diff --git a/rslib/src/template.rs b/rslib/src/template.rs index 876a276e0..e8314bffb 100644 --- a/rslib/src/template.rs +++ b/rslib/src/template.rs @@ -254,11 +254,17 @@ fn parse_inner<'a, I: Iterator>>>( } } -fn template_error_to_anki_error(err: TemplateError, q_side: bool, tr: &I18n) -> AnkiError { - let header = if q_side { - tr.card_template_rendering_front_side_problem() - } else { - tr.card_template_rendering_back_side_problem() +fn template_error_to_anki_error( + err: TemplateError, + q_side: bool, + browser: bool, + 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>, card_ord: u16, is_cloze: bool, + browser: bool, tr: &I18n, ) -> Result<(Vec, Vec)> { // 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> { + /// Field names may not be valid. + pub(crate) fn all_referenced_field_names(&self) -> HashSet<&str> { let mut set = HashSet::new(); - find_fields_with_filter(&self.0, &mut set, None); + find_field_references(&self.0, &mut set, false, true); 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_cloze_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, true, false); set } } -/// Insert all fields in 'nodes' with 'filter' into 'fields'. If 'filter' is None, -/// all fields are collected. -fn find_fields_with_filter<'a>( +fn find_field_references<'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, .. } => { - find_fields_with_filter(children, fields, filter); - } - ParsedNode::NegatedConditional { children, .. } => { - find_fields_with_filter(children, fields, filter); + ParsedNode::Conditional { key, children } + | ParsedNode::NegatedConditional { key, children } => { + if with_conditionals { + fields.insert(key); + } + find_field_references(children, fields, cloze_only, with_conditionals); } } } @@ -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!(