Cleanup config include code and error handling

src/config/element.rs

@@ -18,7 +18,7 @@ pub struct WidgetDefinition {
 }
 
 impl WidgetDefinition {
-    pub fn from_xml_element(xml: XmlElement) -> Result<Self> {
+    pub fn from_xml_element(xml: &XmlElement) -> Result<Self> {
         with_text_pos_context! { xml =>
             if xml.tag_name() != "def" {
                 bail!(
@@ -189,7 +189,7 @@ mod test {
 
         assert_eq!(
             expected,
-            WidgetDefinition::from_xml_element(xml.as_element().unwrap().to_owned()).unwrap()
+            WidgetDefinition::from_xml_element(xml.as_element().unwrap()).unwrap()
         );
     }
 }

src/config/eww_config.rs

@@ -42,24 +42,33 @@ impl EwwConfig {
             let document = roxmltree::Document::parse(&content).map_err(|e| anyhow!(e))?;
             let root_node = XmlNode::from(document.root_element());
             let root_element = root_node.as_element()?;
-            EwwConfig::from_xml_element(root_element.clone(), path.as_ref())?
+
+            let (config, included_paths) = EwwConfig::from_xml_element(root_element.clone(), path.as_ref())
+                .with_context(|| format!("Error parsing eww config file {}", path.as_ref().display()))?;
+
+            let parsed_includes = included_paths
+                .into_iter()
+                .map(|included_path| EwwConfig::read_from_file(included_path))
+                .collect::<Result<Vec<_>>>()
+                .with_context(|| format!("Included in {}", path.as_ref().display()))?;
+
+            EwwConfig::merge_includes(config, parsed_includes)
+                .context("Failed to merge included files into parent configuration file")?
         };
-        result.with_context(|| format!("Failed to parse xml config in {}", path.as_ref().display()))
+        result.with_context(|| format!("Failed to load eww config file {}", path.as_ref().display()))
     }
 
-    pub fn from_xml_element<P: AsRef<std::path::Path>>(xml: XmlElement, path: P) -> Result<Self> {
+    pub fn from_xml_element<P: AsRef<std::path::Path>>(xml: XmlElement, path: P) -> Result<(Self, Vec<PathBuf>)> {
         let path = path.as_ref();
 
-        let includes = match xml.child("includes").ok() {
+        let included_paths = match xml.child("includes").ok() {
             Some(tag) => tag
                 .child_elements()
                 .map(|child| {
-                    let childpath = child.attr("path")?;
-                    let basepath = path.parent().unwrap();
-                    EwwConfig::read_from_file(basepath.join(childpath))
+                    crate::ensure_xml_tag_is!(child, "file");
+                    Ok(join_path_pretty(path, PathBuf::from(child.attr("path")?)))
                 })
-                .collect::<Result<Vec<_>>>()
-                .context(format!("error handling include definitions at: {}", path.display()))?,
+                .collect::<Result<Vec<_>>>()?,
             None => Default::default(),
         };
 
@@ -67,11 +76,12 @@ impl EwwConfig {
             Some(tag) => tag
                 .child_elements()
                 .map(|child| {
-                    let def = WidgetDefinition::from_xml_element(child)?;
+                    let def = WidgetDefinition::from_xml_element(&child).with_context(|| {
+                        format!("Error parsing widget definition at {}:{}", path.display(), &child.text_pos())
+                    })?;
                     Ok((def.name.clone(), def))
                 })
-                .collect::<Result<HashMap<_, _>>>()
-                .with_context(|| format!("error parsing widget definitions at: {}", path.display()))?,
+                .collect::<Result<HashMap<_, _>>>()?,
             None => Default::default(),
         };
 
@@ -79,11 +89,12 @@ impl EwwConfig {
             Some(tag) => tag
                 .child_elements()
                 .map(|child| {
-                    let def = EwwWindowDefinition::from_xml_element(child)?;
+                    let def = EwwWindowDefinition::from_xml_element(&child).with_context(|| {
+                        format!("Error parsing window definition at {}:{}", path.display(), &child.text_pos())
+                    })?;
                     Ok((def.name.to_owned(), def))
                 })
-                .collect::<Result<HashMap<_, _>>>()
-                .with_context(|| format!("error parsing window definitions at: {}", path.display()))?,
+                .collect::<Result<HashMap<_, _>>>()?,
             None => Default::default(),
         };
 
@@ -92,14 +103,14 @@ impl EwwConfig {
             None => Default::default(),
         };
 
-        let current_config = EwwConfig {
+        let config = EwwConfig {
             widgets: definitions,
             windows,
             initial_variables,
             script_vars,
             filepath: path.to_path_buf(),
         };
-        EwwConfig::merge_includes(current_config, includes)
+        Ok((config, included_paths))
     }
 
     // TODO this is kinda ugly
@@ -162,6 +173,20 @@ fn parse_variables_block(xml: XmlElement) -> Result<(HashMap<VarName, PrimitiveV
     Ok((normal_vars, script_vars))
 }
 
+/// Joins two paths while keeping it somewhat pretty.
+/// If the second path is absolute, this will just return the second path.
+/// If it is relative, it will return the second path joined onto the first path, removing any `./` if present.
+/// TODO this is not yet perfect, as it will still leave ../ and multiple ./ etc,... check for a Path::simplify or something.
+fn join_path_pretty<P: AsRef<std::path::Path>, P2: AsRef<std::path::Path>>(a: P, b: P2) -> PathBuf {
+    let a = a.as_ref();
+    let b = b.as_ref();
+    if b.is_absolute() {
+        b.to_path_buf()
+    } else {
+        a.parent().unwrap().join(b.strip_prefix("./").unwrap_or(&b))
+    }
+}
+
 #[cfg(test)]
 mod test {
     use crate::config::{EwwConfig, XmlNode};
@@ -219,8 +244,12 @@ mod test {
 
         let document1 = roxmltree::Document::parse(&input1).unwrap();
         let document2 = roxmltree::Document::parse(input2).unwrap();
-        let config1 = EwwConfig::from_xml_element(XmlNode::from(document1.root_element()).as_element().unwrap().clone(), "");
-        let config2 = EwwConfig::from_xml_element(XmlNode::from(document2.root_element()).as_element().unwrap().clone(), "");
+        let config1 = EwwConfig::from_xml_element(XmlNode::from(document1.root_element()).as_element().unwrap().clone(), "")
+            .unwrap()
+            .0;
+        let config2 = EwwConfig::from_xml_element(XmlNode::from(document2.root_element()).as_element().unwrap().clone(), "")
+            .unwrap()
+            .0;
         let base_config = EwwConfig {
             widgets: HashMap::new(),
             windows: HashMap::new(),
@@ -229,7 +258,7 @@ mod test {
             filepath: "test_path".into(),
         };
 
-        let merged_config = EwwConfig::merge_includes(base_config, vec![config1.unwrap(), config2.unwrap()]).unwrap();
+        let merged_config = EwwConfig::merge_includes(base_config, vec![config1, config2]).unwrap();
 
         assert_eq!(merged_config.widgets.len(), 2);
         assert_eq!(merged_config.windows.len(), 2);

src/config/window_definition.rs

@@ -18,7 +18,7 @@ pub struct EwwWindowDefinition {
 }
 
 impl EwwWindowDefinition {
-    pub fn from_xml_element(xml: XmlElement) -> Result<Self> {
+    pub fn from_xml_element(xml: &XmlElement) -> Result<Self> {
         ensure_xml_tag_is!(xml, "window");
         let stacking: WindowStacking = xml.parse_optional_attr("stacking")?.unwrap_or_default();
         let screen_number = xml.parse_optional_attr("screen")?;

src/config/xml_ext.rs

@@ -179,14 +179,20 @@ impl<'a, 'b> XmlElement<'a, 'b> {
 
     pub fn optional_attr<O, F: FnOnce(&str) -> Result<O>>(&self, key: &str, parse: F) -> Result<Option<O>> {
         match self.0.attribute(key) {
-            Some(value) => parse(value).map(Some),
+            Some(value) => parse(value)
+                .with_context(|| format!("Parsing the value of {}=\"{}\" in <{}>", key, value, self.tag_name()))
+                .map(Some),
             None => Ok(None),
         }
     }
 
-    pub fn parse_optional_attr<E, O: std::str::FromStr<Err = E>>(&self, key: &str) -> Result<Option<O>, E> {
+    pub fn parse_optional_attr<E: Into<anyhow::Error>, O: std::str::FromStr<Err = E>>(&self, key: &str) -> Result<Option<O>> {
         match self.0.attribute(key) {
-            Some(value) => value.parse::<O>().map(Some),
+            Some(value) => value
+                .parse::<O>()
+                .map_err(|e| anyhow!(e))
+                .with_context(|| format!("Parsing the value of {}=\"{}\" in <{}>", key, value, self.tag_name()))
+                .map(Some),
             None => Ok(None),
         }
     }