update template on field removals as well

rslib/src/notetype/mod.rs

@@ -214,20 +214,11 @@ impl NoteType {
         }
         let reqs = self.updated_requirements(&parsed_templates);
 
-        // handle renamed fields
+        // handle renamed+deleted fields
         if let Some(existing) = existing {
-            let renamed_fields = self.renamed_fields(existing);
+            let fields = self.renamed_and_removed_fields(existing);
-            if !renamed_fields.is_empty() {
+            if !fields.is_empty() {
-                let updated_templates =
+                self.update_templates_for_renamed_and_removed_fields(fields, parsed_templates);
-                    self.updated_templates_for_renamed_fields(renamed_fields, parsed_templates);
-                for (idx, (q, a)) in updated_templates.into_iter().enumerate() {
-                    if let Some(q) = q {
-                        self.templates[idx].config.q_format = q
-                    }
-                    if let Some(a) = a {
-                        self.templates[idx].config.a_format = a
-                    }
-                }
             }
         }
         self.config.reqs = reqs;
@@ -236,48 +227,51 @@ impl NoteType {
         Ok(())
     }
 
-    fn renamed_fields(&self, current: &NoteType) -> HashMap<String, String> {
+    fn renamed_and_removed_fields(&self, current: &NoteType) -> HashMap<String, Option<String>> {
-        self.fields
+        let mut remaining_ords = HashSet::new();
+        // gather renames
+        let mut map: HashMap<String, Option<String>> = self
+            .fields
             .iter()
             .filter_map(|field| {
                 if let Some(existing_ord) = field.ord {
+                    remaining_ords.insert(existing_ord);
                     if let Some(existing_field) = current.fields.get(existing_ord as usize) {
                         if existing_field.name != field.name {
-                            return Some((existing_field.name.clone(), field.name.clone()));
+                            return Some((existing_field.name.clone(), Some(field.name.clone())));
                         }
                     }
                 }
                 None
             })
-            .collect()
+            .collect();
+        // and add any fields that have been removed
+        for (idx, field) in current.fields.iter().enumerate() {
+            if !remaining_ords.contains(&(idx as u32)) {
+                map.insert(field.name.clone(), None);
+            }
         }
 
-    fn updated_templates_for_renamed_fields(
+        map
-        &self,
+    }
-        renamed_fields: HashMap<String, String>,
+
+    /// Update templates to reflect field deletions and renames.
+    /// Any templates that failed to parse will be ignored.
+    fn update_templates_for_renamed_and_removed_fields(
+        &mut self,
+        fields: HashMap<String, Option<String>>,
         parsed: Vec<(Option<ParsedTemplate>, Option<ParsedTemplate>)>,
-    ) -> Vec<(Option<String>, Option<String>)> {
+    ) {
-        parsed
+        for (idx, (q, a)) in parsed.into_iter().enumerate() {
-            .into_iter()
+            if let Some(q) = q {
-            .map(|(q, a)| {
+                let updated = q.rename_and_remove_fields(&fields);
-                let q = q.and_then(|mut q| {
+                self.templates[idx].config.q_format = updated.template_to_string();
-                    if q.rename_fields(&renamed_fields) {
+            }
-                        Some(q.template_to_string())
+            if let Some(a) = a {
-                    } else {
+                let updated = a.rename_and_remove_fields(&fields);
-                        None
+                self.templates[idx].config.a_format = updated.template_to_string();
             }
-                });
-                let a = a.and_then(|mut a| {
-                    if a.rename_fields(&renamed_fields) {
-                        Some(a.template_to_string())
-                    } else {
-                        None
         }
-                });
-
-                (q, a)
-            })
-            .collect()
     }
 
     fn parsed_templates(&self) -> Vec<(Option<ParsedTemplate>, Option<ParsedTemplate>)> {

rslib/src/notetype/schemachange.rs

@@ -222,19 +222,23 @@ mod test {
     }
 
     #[test]
-    fn field_renaming() -> Result<()> {
+    fn field_renaming_and_deleting() -> Result<()> {
         let mut col = open_test_collection();
         let mut nt = col
             .storage
             .get_notetype(col.get_current_notetype_id().unwrap())?
             .unwrap();
-        nt.templates[0].config.q_format += "\n{{#Front}}{{some:Front}}{{/Front}}";
+        nt.templates[0].config.q_format += "\n{{#Front}}{{some:Front}}{{Back}}{{/Front}}";
         nt.fields[0].name = "Test".into();
         col.update_notetype(&mut nt, false)?;
         assert_eq!(
             &nt.templates[0].config.q_format,
-            "{{Test}}\n{{#Test}}{{some:Test}}{{/Test}}"
+            "{{Test}}\n{{#Test}}{{some:Test}}{{Back}}{{/Test}}"
         );
+        nt.fields.remove(0);
+        col.update_notetype(&mut nt, false)?;
+        assert_eq!(&nt.templates[0].config.q_format, "\n{{Back}}");
+
         Ok(())
     }
 

rslib/src/template.rs

@@ -592,49 +592,73 @@ impl ParsedTemplate {
     }
 }
 
-// Renaming fields
+// Renaming & deleting fields
 //----------------------------------------
 
 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_fields(&mut self, fields: &HashMap<String, String>) -> bool {
+    pub(crate) fn rename_and_remove_fields(
-        rename_fields(&mut self.0, fields)
+        self,
+        fields: &HashMap<String, Option<String>>,
+    ) -> ParsedTemplate {
+        let out = rename_and_remove_fields(self.0, fields);
+        ParsedTemplate(out)
     }
 }
 
-fn rename_fields(nodes: &mut [ParsedNode], fields: &HashMap<String, String>) -> bool {
+fn rename_and_remove_fields(
-    let mut changed = false;
+    nodes: Vec<ParsedNode>,
+    fields: &HashMap<String, Option<String>>,
+) -> Vec<ParsedNode> {
+    let mut out = vec![];
     for node in nodes {
         match node {
-            ParsedNode::Text(_) => (),
+            ParsedNode::Text(text) => out.push(ParsedNode::Text(text)),
-            ParsedNode::Replacement { key, .. } => {
+            ParsedNode::Replacement { key, filters } => {
-                if let Some(new_name) = fields.get(key) {
+                match fields.get(&key) {
-                    *key = new_name.clone();
+                    // delete the field
-                    changed = true;
+                    Some(None) => (),
+                    // rename it
+                    Some(Some(new_name)) => out.push(ParsedNode::Replacement {
+                        key: new_name.into(),
+                        filters,
+                    }),
+                    // or leave it alone
+                    None => out.push(ParsedNode::Replacement { key, filters }),
                 }
             }
             ParsedNode::Conditional { key, children } => {
-                if let Some(new_name) = fields.get(key) {
+                let children = rename_and_remove_fields(children, fields);
-                    *key = new_name.clone();
+                match fields.get(&key) {
-                    changed = true;
+                    // remove the field, preserving children
-                };
+                    Some(None) => out.extend(children),
-                if rename_fields(children, fields) {
+                    // rename it
-                    changed = true;
+                    Some(Some(new_name)) => out.push(ParsedNode::Conditional {
+                        key: new_name.into(),
+                        children,
+                    }),
+                    // or leave it alone
+                    None => out.push(ParsedNode::Conditional { key, children }),
                 }
             }
             ParsedNode::NegatedConditional { key, children } => {
-                if let Some(new_name) = fields.get(key) {
+                let children = rename_and_remove_fields(children, fields);
-                    *key = new_name.clone();
+                match fields.get(&key) {
-                    changed = true;
+                    // remove the field, preserving children
-                };
+                    Some(None) => out.extend(children),
-                if rename_fields(children, fields) {
+                    // rename it
-                    changed = true;
+                    Some(Some(new_name)) => out.push(ParsedNode::Conditional {
+                        key: new_name.into(),
+                        children,
+                    }),
+                    // or leave it alone
+                    None => out.push(ParsedNode::Conditional { key, children }),
                 }
             }
         }
     }
-    changed
+    out
 }
 
 // Writing back to a string