From 6c5c483f8caabd2fcaf4b7dad63d1b9a8482e437 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Wed, 24 Nov 2021 12:34:27 -0500 Subject: [PATCH 01/11] Track path of glif errors This also removes the 'position' param from these errors, since it was both not very helpful and not very accurate. It would be nice to be able to do better error reporting of this stuff, but quick_xml doesn't really expose enough info to be useful. --- src/error.rs | 78 ++++++-------- src/glyph/mod.rs | 22 ++-- src/glyph/parse.rs | 253 +++++++++++++++++++++------------------------ src/layer.rs | 13 +-- 4 files changed, 166 insertions(+), 200 deletions(-) diff --git a/src/error.rs b/src/error.rs index 5677cc96..b0d72396 100644 --- a/src/error.rs +++ b/src/error.rs @@ -43,9 +43,16 @@ pub enum Error { IoError(IoError), /// An error returned when there is an XML parsing problem. ParseError(XmlError), - /// An error that wraps a [GlifError]. - Glif(GlifError), - /// An error that wraps a [GlifWriteError]. + /// A `.glif` file could not be loaded. + GlifLoad { + /// The path of the relevant `.glif` file. + path: PathBuf, + /// The underlying error. + inner: GlifLoadError, + }, + /// An error that occurs when attempting to write a [`Glyph`] to disk. + /// + /// [`Glyph`]: crate::Glyph GlifWrite(GlifWriteError), /// An error that wraps a [PlistError]. PlistError(PlistError), @@ -79,6 +86,17 @@ pub enum Error { InvalidStoreEntry(PathBuf, StoreError), } +/// An error that occurs while attempting to read a .glif file from disk. +#[derive(Debug)] +pub enum GlifLoadError { + /// An [`std::io::Error`]. + IoError(IoError), + /// A [`quick_xml::Error`]. + Xml(XmlError), + /// The .glif file was malformed. + Parse(ErrorKind), +} + /// An error representing a failure to insert content into a [`crate::datastore::Store`]. #[derive(Clone, Debug)] #[non_exhaustive] @@ -141,8 +159,7 @@ pub struct GlifError { pub kind: ErrorKind, } -/// An error representing a failure during .glif file serialization. This -/// error wraps [GlyphName] and [WriteError] types. +/// An error when attempting to write a .glif file. #[derive(Debug)] pub struct GlifWriteError { /// The name of the glif where the error occured. @@ -169,16 +186,6 @@ pub enum WriteError { Plist(PlistError), } -/// Errors that happen when parsing `glif` files. This is converted into either -/// `Error::ParseError` or `Error::Glif` at the parse boundary. -#[derive(Debug)] -pub(crate) enum GlifErrorInternal { - /// A problem with the xml data. - Xml(XmlError), - /// A violation of the ufo spec. - Spec { kind: ErrorKind, position: usize }, -} - /// The reason for a glif parse failure. #[derive(Debug, Clone, Copy)] pub enum ErrorKind { @@ -272,8 +279,8 @@ impl std::fmt::Display for Error { Error::IoError(e) => e.fmt(f), Error::ParseError(e) => e.fmt(f), Error::InvalidColor(e) => e.fmt(f), - Error::Glif(GlifError { path, position, kind }) => { - write!(f, "Glif error in {:?} index {}: '{}", path, position, kind) + Error::GlifLoad { path, inner } => { + write!(f, "Error reading glif '{}': '{}'", path.display(), inner) } Error::GlifWrite(GlifWriteError { name, inner }) => { write!(f, "Failed to save glyph {}, error: '{}'", name, inner) @@ -309,6 +316,16 @@ impl std::fmt::Display for Error { } } +impl std::fmt::Display for GlifLoadError { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + GlifLoadError::Xml(err) => err.fmt(f), + GlifLoadError::IoError(err) => err.fmt(f), + GlifLoadError::Parse(err) => err.fmt(f), + } + } +} + impl std::fmt::Display for StoreError { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { use StoreError::*; @@ -448,12 +465,6 @@ impl std::fmt::Display for InvalidColorString { impl std::error::Error for InvalidColorString {} -impl ErrorKind { - pub(crate) fn to_error(self, position: usize) -> GlifErrorInternal { - GlifErrorInternal::Spec { kind: self, position } - } -} - #[doc(hidden)] impl From for Error { fn from(src: InvalidColorString) -> Error { @@ -468,13 +479,6 @@ impl From for Error { } } -#[doc(hidden)] -impl From<(ErrorKind, usize)> for GlifErrorInternal { - fn from(src: (ErrorKind, usize)) -> GlifErrorInternal { - GlifErrorInternal::Spec { kind: src.0, position: src.1 } - } -} - #[doc(hidden)] impl From for Error { fn from(src: XmlError) -> Error { @@ -496,20 +500,6 @@ impl From for Error { } } -#[doc(hidden)] -impl From for Error { - fn from(src: GlifError) -> Error { - Error::Glif(src) - } -} - -#[doc(hidden)] -impl From for GlifErrorInternal { - fn from(src: XmlError) -> GlifErrorInternal { - GlifErrorInternal::Xml(src) - } -} - #[doc(hidden)] impl From for WriteError { fn from(src: XmlError) -> WriteError { diff --git a/src/glyph/mod.rs b/src/glyph/mod.rs index 30724569..49d0b1d3 100644 --- a/src/glyph/mod.rs +++ b/src/glyph/mod.rs @@ -12,7 +12,7 @@ use std::sync::Arc; #[cfg(feature = "druid")] use druid::{Data, Lens}; -use crate::error::{Error, ErrorKind, GlifError, GlifErrorInternal}; +use crate::error::{Error, ErrorKind, GlifLoadError}; use crate::names::NameList; use crate::shared_types::PUBLIC_OBJECT_LIBS_KEY; use crate::{Color, Guideline, Identifier, Line, Plist, WriteOptions}; @@ -62,22 +62,20 @@ impl Glyph { /// When loading glyphs in bulk, [`Glyph::load_with_names`] should be preferred, /// since it will allow glyph names (in glyphs and components) to be shared /// between instances. - pub fn load(path: impl AsRef) -> Result { + pub fn load(path: impl AsRef) -> Result { let path = path.as_ref(); let names = NameList::default(); Glyph::load_with_names(path, &names) } - /// Returns a [`Glyph`] at this `path` with glyph and component glyph name sharing - /// between instances. - pub fn load_with_names(path: &Path, names: &NameList) -> Result { - let data = std::fs::read(path)?; - parse::GlifParser::from_xml(&data, Some(names)).map_err(|e| match e { - GlifErrorInternal::Xml(e) => e.into(), - GlifErrorInternal::Spec { kind, position } => { - GlifError { kind, position, path: Some(path.to_owned()) }.into() - } - }) + /// Attempt to load the glyph at `path`, reusing names from the `NameList`. + /// + /// This uses string interning to reuse allocations when a glyph name + /// occurs multiple times (such as in components or in different layers). + pub fn load_with_names(path: &Path, names: &NameList) -> Result { + std::fs::read(path) + .map_err(GlifLoadError::IoError) + .and_then(|data| parse::GlifParser::from_xml(&data, Some(names))) } #[doc(hidden)] diff --git a/src/glyph/parse.rs b/src/glyph/parse.rs index 7126a92e..5f945b22 100644 --- a/src/glyph/parse.rs +++ b/src/glyph/parse.rs @@ -6,28 +6,20 @@ use std::str::FromStr; use std::sync::Arc; use super::*; -use crate::error::{ErrorKind, GlifErrorInternal}; +use crate::error::{ErrorKind, GlifLoadError}; use crate::glyph::builder::{GlyphBuilder, Outline, OutlineBuilder}; use crate::names::NameList; use quick_xml::{ events::{BytesStart, Event}, - Reader, + Error as XmlError, Reader, }; #[cfg(test)] -pub(crate) fn parse_glyph(xml: &[u8]) -> Result { +pub(crate) fn parse_glyph(xml: &[u8]) -> Result { GlifParser::from_xml(xml, None) } -macro_rules! err { - ($r:expr, $errtype:expr) => { - GlifErrorInternal::Spec { kind: $errtype, position: $r.buffer_position() } - }; -} - -type Error = GlifErrorInternal; - pub(crate) struct GlifParser<'names> { builder: GlyphBuilder, /// Optional set of glyph names to be reused between glyphs. @@ -35,14 +27,17 @@ pub(crate) struct GlifParser<'names> { } impl<'names> GlifParser<'names> { - pub(crate) fn from_xml(xml: &[u8], names: Option<&'names NameList>) -> Result { + pub(crate) fn from_xml( + xml: &[u8], + names: Option<&'names NameList>, + ) -> Result { let mut reader = Reader::from_reader(xml); let mut buf = Vec::new(); reader.trim_text(true); - let builder = start(&mut reader, &mut buf)?; - let this = GlifParser { builder, names }; - this.parse_body(&mut reader, xml, &mut buf) + start(&mut reader, &mut buf).and_then(|builder| { + GlifParser { builder, names }.parse_body(&mut reader, xml, &mut buf) + }) } fn parse_body( @@ -50,7 +45,7 @@ impl<'names> GlifParser<'names> { reader: &mut Reader<&[u8]>, raw_xml: &[u8], buf: &mut Vec, - ) -> Result { + ) -> Result { loop { match reader.read_event(buf)? { // outline, lib and note are expected to be start element tags. @@ -60,7 +55,7 @@ impl<'names> GlifParser<'names> { "outline" => self.parse_outline(reader, buf)?, "lib" => self.parse_lib(reader, raw_xml, buf)?, // do this at some point? "note" => self.parse_note(reader, buf)?, - _other => return Err(err!(reader, ErrorKind::UnexpectedTag)), + _other => return Err(ErrorKind::UnexpectedTag.into()), } } // The rest are expected to be empty element tags (exception: outline) with attributes. @@ -69,26 +64,23 @@ impl<'names> GlifParser<'names> { match tag_name.borrow() { "outline" => { // ufoLib parses `` as an empty outline. - self.builder - .outline(Outline::default(), HashSet::new()) - .map_err(|e| err!(reader, e))?; + self.builder.outline(Outline::default(), HashSet::new())?; } "advance" => self.parse_advance(reader, start)?, "unicode" => self.parse_unicode(reader, start)?, "anchor" => self.parse_anchor(reader, start)?, "guideline" => self.parse_guideline(reader, start)?, "image" => self.parse_image(reader, start)?, - _other => return Err(err!(reader, ErrorKind::UnexpectedTag)), + _other => return Err(ErrorKind::UnexpectedTag.into()), } } Event::End(ref end) if end.name() == b"glyph" => break, - _other => return Err(err!(reader, ErrorKind::MissingCloseTag)), + _other => return Err(ErrorKind::MissingCloseTag.into()), } } - let mut glyph = self.builder.finish().map_err(|e| err!(reader, e))?; - // FIXME: Error returns the end of the byte stream as the location, which is misleading. - glyph.load_object_libs().map_err(|e| err!(reader, e))?; + let mut glyph = self.builder.finish()?; + glyph.load_object_libs()?; Ok(glyph) } @@ -97,7 +89,7 @@ impl<'names> GlifParser<'names> { &mut self, reader: &mut Reader<&[u8]>, buf: &mut Vec, - ) -> Result<(), Error> { + ) -> Result<(), GlifLoadError> { let mut outline_builder = OutlineBuilder::new(); loop { @@ -109,7 +101,7 @@ impl<'names> GlifParser<'names> { "contour" => { self.parse_contour(start, reader, &mut new_buf, &mut outline_builder)? } - _other => return Err(err!(reader, ErrorKind::UnexpectedTag)), + _other => return Err(ErrorKind::UnexpectedTag.into()), } } Event::Empty(start) => { @@ -119,17 +111,17 @@ impl<'names> GlifParser<'names> { // https://github.com/unified-font-object/ufo-spec/issues/150 "contour" => (), "component" => self.parse_component(reader, start, &mut outline_builder)?, - _other => return Err(err!(reader, ErrorKind::UnexpectedTag)), + _other => return Err(ErrorKind::UnexpectedTag.into()), } } Event::End(ref end) if end.name() == b"outline" => break, - Event::Eof => return Err(err!(reader, ErrorKind::UnexpectedEof)), - _other => return Err(err!(reader, ErrorKind::UnexpectedElement)), + Event::Eof => return Err(ErrorKind::UnexpectedEof.into()), + _other => return Err(ErrorKind::UnexpectedElement.into()), } } - let (outline, identifiers) = outline_builder.finish().map_err(|e| err!(reader, e))?; - self.builder.outline(outline, identifiers).map_err(|e| err!(reader, e))?; + let (outline, identifiers) = outline_builder.finish()?; + self.builder.outline(outline, identifiers)?; Ok(()) } @@ -140,34 +132,34 @@ impl<'names> GlifParser<'names> { reader: &mut Reader<&[u8]>, buf: &mut Vec, outline_builder: &mut OutlineBuilder, - ) -> Result<(), Error> { + ) -> Result<(), GlifLoadError> { let mut identifier = None; for attr in data.attributes() { if self.builder.get_format() == &GlifVersion::V1 { - return Err(err!(reader, ErrorKind::UnexpectedAttribute)); + return Err(ErrorKind::UnexpectedAttribute.into()); } let attr = attr?; match attr.key { b"identifier" => { let ident = attr.unescape_and_decode_value(reader)?; - identifier = Some(Identifier::new(ident).map_err(|kind| err!(reader, kind))?); + identifier = Some(Identifier::new(ident)?); } - _other => return Err(err!(reader, ErrorKind::UnexpectedAttribute)), + _other => return Err(ErrorKind::UnexpectedAttribute.into()), } } - outline_builder.begin_path(identifier).map_err(|e| err!(reader, e))?; + outline_builder.begin_path(identifier)?; loop { match reader.read_event(buf)? { Event::End(ref end) if end.name() == b"contour" => break, Event::Empty(ref start) if start.name() == b"point" => { self.parse_point(reader, start, outline_builder)?; } - Event::Eof => return Err(err!(reader, ErrorKind::UnexpectedEof)), - _other => return Err(err!(reader, ErrorKind::UnexpectedElement)), + Event::Eof => return Err(ErrorKind::UnexpectedEof.into()), + _other => return Err(ErrorKind::UnexpectedElement.into()), } } - outline_builder.end_path().map_err(|e| err!(reader, e))?; + outline_builder.end_path()?; Ok(()) } @@ -177,7 +169,7 @@ impl<'names> GlifParser<'names> { reader: &mut Reader<&[u8]>, start: BytesStart, outline_builder: &mut OutlineBuilder, - ) -> Result<(), Error> { + ) -> Result<(), GlifLoadError> { let mut base: Option = None; let mut identifier: Option = None; let mut transform = AffineTransform::default(); @@ -186,15 +178,14 @@ impl<'names> GlifParser<'names> { let attr = attr?; let value = attr.unescaped_value()?; let value = reader.decode(&value)?; - let pos = reader.buffer_position(); let kind = ErrorKind::BadNumber; match attr.key { - b"xScale" => transform.x_scale = value.parse().map_err(|_| (kind, pos))?, - b"xyScale" => transform.xy_scale = value.parse().map_err(|_| (kind, pos))?, - b"yxScale" => transform.yx_scale = value.parse().map_err(|_| (kind, pos))?, - b"yScale" => transform.y_scale = value.parse().map_err(|_| (kind, pos))?, - b"xOffset" => transform.x_offset = value.parse().map_err(|_| (kind, pos))?, - b"yOffset" => transform.y_offset = value.parse().map_err(|_| (kind, pos))?, + b"xScale" => transform.x_scale = value.parse().map_err(|_| kind)?, + b"xyScale" => transform.xy_scale = value.parse().map_err(|_| kind)?, + b"yxScale" => transform.yx_scale = value.parse().map_err(|_| kind)?, + b"yScale" => transform.y_scale = value.parse().map_err(|_| kind)?, + b"xOffset" => transform.x_offset = value.parse().map_err(|_| kind)?, + b"yOffset" => transform.y_offset = value.parse().map_err(|_| kind)?, b"base" => { let name: Arc = value.into(); let name = match self.names.as_ref() { @@ -204,19 +195,17 @@ impl<'names> GlifParser<'names> { base = Some(name); } b"identifier" => { - identifier = Some(value.parse().map_err(|kind| err!(reader, kind))?); + identifier = Some(value.parse()?); } - _other => return Err(err!(reader, ErrorKind::UnexpectedComponentField)), + _other => return Err(ErrorKind::UnexpectedComponentField.into()), } } if base.is_none() { - return Err(err!(reader, ErrorKind::BadComponent)); + return Err(ErrorKind::BadComponent.into()); } - outline_builder - .add_component(base.unwrap(), transform, identifier) - .map_err(|e| err!(reader, e))?; + outline_builder.add_component(base.unwrap(), transform, identifier)?; Ok(()) } @@ -225,13 +214,13 @@ impl<'names> GlifParser<'names> { reader: &mut Reader<&[u8]>, raw_xml: &[u8], buf: &mut Vec, - ) -> Result<(), Error> { + ) -> Result<(), GlifLoadError> { let start = reader.buffer_position(); let mut end = start; loop { match reader.read_event(buf)? { Event::End(ref end) if end.name() == b"lib" => break, - Event::Eof => return Err(err!(reader, ErrorKind::UnexpectedEof)), + Event::Eof => return Err(ErrorKind::UnexpectedEof.into()), _other => end = reader.buffer_position(), } } @@ -240,21 +229,23 @@ impl<'names> GlifParser<'names> { let dict = plist::Value::from_reader_xml(plist_slice) .ok() .and_then(plist::Value::into_dictionary) - .ok_or_else(|| err!(reader, ErrorKind::BadLib))?; - self.builder.lib(dict).map_err(|e| err!(reader, e))?; + .ok_or(ErrorKind::BadLib)?; + self.builder.lib(dict)?; Ok(()) } - fn parse_note(&mut self, reader: &mut Reader<&[u8]>, buf: &mut Vec) -> Result<(), Error> { + fn parse_note( + &mut self, + reader: &mut Reader<&[u8]>, + buf: &mut Vec, + ) -> Result<(), GlifLoadError> { loop { match reader.read_event(buf)? { Event::End(ref end) if end.name() == b"note" => break, Event::Text(text) => { - self.builder - .note(text.unescape_and_decode(reader)?) - .map_err(|e| err!(reader, e))?; + self.builder.note(text.unescape_and_decode(reader)?)?; } - Event::Eof => return Err(err!(reader, ErrorKind::UnexpectedEof)), + Event::Eof => return Err(ErrorKind::UnexpectedEof.into()), _other => (), } } @@ -266,7 +257,7 @@ impl<'names> GlifParser<'names> { reader: &Reader<&[u8]>, data: &BytesStart<'a>, outline_builder: &mut OutlineBuilder, - ) -> Result<(), Error> { + ) -> Result<(), GlifLoadError> { let mut name: Option = None; let mut x: Option = None; let mut y: Option = None; @@ -278,33 +269,28 @@ impl<'names> GlifParser<'names> { let attr = attr?; let value = attr.unescaped_value()?; let value = reader.decode(&value)?; - let pos = reader.buffer_position(); match attr.key { b"x" => { - x = Some(value.parse().map_err(|_| (ErrorKind::BadNumber, pos))?); + x = Some(value.parse().map_err(|_| ErrorKind::BadNumber)?); } b"y" => { - y = Some(value.parse().map_err(|_| (ErrorKind::BadNumber, pos))?); + y = Some(value.parse().map_err(|_| ErrorKind::BadNumber)?); } b"name" => name = Some(value.to_string()), b"type" => { - typ = value - .parse() - .map_err(|e: ErrorKind| e.to_error(reader.buffer_position()))? + typ = value.parse()?; } b"smooth" => smooth = value == "yes", b"identifier" => { - identifier = Some(value.parse().map_err(|kind| err!(reader, kind))?); + identifier = Some(value.parse()?); } - _other => return Err(err!(reader, ErrorKind::UnexpectedPointField)), + _other => return Err(ErrorKind::UnexpectedPointField.into()), } } if x.is_none() || y.is_none() { - return Err(err!(reader, ErrorKind::BadPoint)); + return Err(ErrorKind::BadPoint.into()); } - outline_builder - .add_point((x.unwrap(), y.unwrap()), typ, smooth, name, identifier) - .map_err(|e| err!(reader, e))?; + outline_builder.add_point((x.unwrap(), y.unwrap()), typ, smooth, name, identifier)?; Ok(()) } @@ -313,7 +299,7 @@ impl<'names> GlifParser<'names> { &mut self, reader: &Reader<&[u8]>, data: BytesStart<'a>, - ) -> Result<(), Error> { + ) -> Result<(), GlifLoadError> { let mut width: f64 = 0.0; let mut height: f64 = 0.0; for attr in data.attributes() { @@ -322,22 +308,17 @@ impl<'names> GlifParser<'names> { b"width" | b"height" => { let value = attr.unescaped_value()?; let value = reader.decode(&value)?; - let value: f64 = - value.parse().map_err(|_| err!(reader, ErrorKind::BadNumber))?; + let value: f64 = value.parse().map_err(|_| ErrorKind::BadNumber)?; match attr.key { b"width" => width = value, b"height" => height = value, _other => unreachable!(), }; } - _other => return Err(err!(reader, ErrorKind::UnexpectedAttribute)), + _other => return Err(ErrorKind::UnexpectedAttribute.into()), } } - self.builder - .width(width) - .map_err(|e| err!(reader, e))? - .height(height) - .map_err(|e| err!(reader, e))?; + self.builder.width(width)?.height(height)?; Ok(()) } @@ -345,7 +326,7 @@ impl<'names> GlifParser<'names> { &mut self, reader: &Reader<&[u8]>, data: BytesStart<'a>, - ) -> Result<(), Error> { + ) -> Result<(), GlifLoadError> { for attr in data.attributes() { let attr = attr?; match attr.key { @@ -355,10 +336,10 @@ impl<'names> GlifParser<'names> { let chr = u32::from_str_radix(value, 16) .map_err(|_| value.to_string()) .and_then(|n| char::try_from(n).map_err(|_| value.to_string())) - .map_err(|_| err!(reader, ErrorKind::BadHexValue))?; + .map_err(|_| ErrorKind::BadHexValue)?; self.builder.unicode(chr); } - _other => return Err(err!(reader, ErrorKind::UnexpectedAttribute)), + _other => return Err(ErrorKind::UnexpectedAttribute.into()), } } Ok(()) @@ -368,7 +349,7 @@ impl<'names> GlifParser<'names> { &mut self, reader: &Reader<&[u8]>, data: BytesStart<'a>, - ) -> Result<(), Error> { + ) -> Result<(), GlifLoadError> { let mut x: Option = None; let mut y: Option = None; let mut name: Option = None; @@ -381,28 +362,24 @@ impl<'names> GlifParser<'names> { let value = reader.decode(&value)?; match attr.key { b"x" => { - x = Some(value.parse().map_err(|_| err!(reader, ErrorKind::BadNumber))?); + x = Some(value.parse().map_err(|_| ErrorKind::BadNumber)?); } b"y" => { - y = Some(value.parse().map_err(|_| err!(reader, ErrorKind::BadNumber))?); + y = Some(value.parse().map_err(|_| ErrorKind::BadNumber)?); } b"name" => name = Some(value.to_string()), - b"color" => { - color = Some(value.parse().map_err(|_| err!(reader, ErrorKind::BadColor))?) - } + b"color" => color = Some(value.parse().map_err(|_| ErrorKind::BadColor)?), b"identifier" => { - identifier = Some(value.parse().map_err(|kind| err!(reader, kind))?); + identifier = Some(value.parse()?); } - _other => return Err(err!(reader, ErrorKind::UnexpectedAnchorField)), + _other => return Err(ErrorKind::UnexpectedAnchorField.into()), } } if x.is_none() || y.is_none() { - return Err(err!(reader, ErrorKind::BadAnchor)); + return Err(ErrorKind::BadAnchor.into()); } - self.builder - .anchor(Anchor::new(x.unwrap(), y.unwrap(), name, color, identifier, None)) - .map_err(|e| err!(reader, e))?; + self.builder.anchor(Anchor::new(x.unwrap(), y.unwrap(), name, color, identifier, None))?; Ok(()) } @@ -410,7 +387,7 @@ impl<'names> GlifParser<'names> { &mut self, reader: &Reader<&[u8]>, data: BytesStart<'a>, - ) -> Result<(), Error> { + ) -> Result<(), GlifLoadError> { let mut x: Option = None; let mut y: Option = None; let mut angle: Option = None; @@ -424,22 +401,20 @@ impl<'names> GlifParser<'names> { let value = reader.decode(&value)?; match attr.key { b"x" => { - x = Some(value.parse().map_err(|_| err!(reader, ErrorKind::BadNumber))?); + x = Some(value.parse().map_err(|_| ErrorKind::BadNumber)?); } b"y" => { - y = Some(value.parse().map_err(|_| err!(reader, ErrorKind::BadNumber))?); + y = Some(value.parse().map_err(|_| ErrorKind::BadNumber)?); } b"angle" => { - angle = Some(value.parse().map_err(|_| err!(reader, ErrorKind::BadNumber))?); + angle = Some(value.parse().map_err(|_| ErrorKind::BadNumber)?); } b"name" => name = Some(value.to_string()), - b"color" => { - color = Some(value.parse().map_err(|_| err!(reader, ErrorKind::BadColor))?) - } + b"color" => color = Some(value.parse().map_err(|_| ErrorKind::BadColor)?), b"identifier" => { - identifier = Some(value.parse().map_err(|kind| err!(reader, kind))?); + identifier = Some(value.parse()?); } - _other => return Err(err!(reader, ErrorKind::UnexpectedGuidelineField)), + _other => return Err(ErrorKind::UnexpectedGuidelineField.into()), } } @@ -448,15 +423,13 @@ impl<'names> GlifParser<'names> { (None, Some(y), None) => Line::Horizontal(y), (Some(x), Some(y), Some(degrees)) => { if !(0.0..=360.0).contains(°rees) { - return Err(err!(reader, ErrorKind::BadGuideline)); + return Err(ErrorKind::BadGuideline.into()); } Line::Angle { x, y, degrees } } - _other => return Err(err!(reader, ErrorKind::BadGuideline)), + _other => return Err(ErrorKind::BadGuideline.into()), }; - self.builder - .guideline(Guideline::new(line, name, color, identifier, None)) - .map_err(|e| err!(reader, e))?; + self.builder.guideline(Guideline::new(line, name, color, identifier, None))?; Ok(()) } @@ -465,7 +438,7 @@ impl<'names> GlifParser<'names> { &mut self, reader: &Reader<&[u8]>, data: BytesStart<'a>, - ) -> Result<(), Error> { + ) -> Result<(), GlifLoadError> { let mut filename: Option = None; let mut color: Option = None; let mut transform = AffineTransform::default(); @@ -474,36 +447,31 @@ impl<'names> GlifParser<'names> { let attr = attr?; let value = attr.unescaped_value()?; let value = reader.decode(&value)?; - let pos = reader.buffer_position(); let kind = ErrorKind::BadNumber; match attr.key { - b"xScale" => transform.x_scale = value.parse().map_err(|_| (kind, pos))?, - b"xyScale" => transform.xy_scale = value.parse().map_err(|_| (kind, pos))?, - b"yxScale" => transform.yx_scale = value.parse().map_err(|_| (kind, pos))?, - b"yScale" => transform.y_scale = value.parse().map_err(|_| (kind, pos))?, - b"xOffset" => transform.x_offset = value.parse().map_err(|_| (kind, pos))?, - b"yOffset" => transform.y_offset = value.parse().map_err(|_| (kind, pos))?, - b"color" => { - color = Some(value.parse().map_err(|_| err!(reader, ErrorKind::BadColor))?) - } + b"xScale" => transform.x_scale = value.parse().map_err(|_| kind)?, + b"xyScale" => transform.xy_scale = value.parse().map_err(|_| kind)?, + b"yxScale" => transform.yx_scale = value.parse().map_err(|_| kind)?, + b"yScale" => transform.y_scale = value.parse().map_err(|_| kind)?, + b"xOffset" => transform.x_offset = value.parse().map_err(|_| kind)?, + b"yOffset" => transform.y_offset = value.parse().map_err(|_| kind)?, + b"color" => color = Some(value.parse().map_err(|_| ErrorKind::BadColor)?), b"fileName" => filename = Some(PathBuf::from(value.to_string())), - _other => return Err(err!(reader, ErrorKind::UnexpectedImageField)), + _other => return Err(ErrorKind::UnexpectedImageField.into()), } } if filename.is_none() { - return Err(err!(reader, ErrorKind::BadImage)); + return Err(ErrorKind::BadImage.into()); } - self.builder - .image(Image { file_name: filename.unwrap(), color, transform }) - .map_err(|e| err!(reader, e))?; + self.builder.image(Image { file_name: filename.unwrap(), color, transform })?; Ok(()) } } -fn start(reader: &mut Reader<&[u8]>, buf: &mut Vec) -> Result { +fn start(reader: &mut Reader<&[u8]>, buf: &mut Vec) -> Result { loop { match reader.read_event(buf)? { Event::Comment(_) => (), @@ -511,6 +479,7 @@ fn start(reader: &mut Reader<&[u8]>, buf: &mut Vec) -> Result { let mut name = String::new(); let mut format: Option = None; + //let mut pos for attr in start.attributes() { let attr = attr?; // XXX: support `formatMinor` @@ -521,21 +490,18 @@ fn start(reader: &mut Reader<&[u8]>, buf: &mut Vec) -> Result { let value = attr.unescaped_value()?; let value = reader.decode(&value)?; - format = - Some(value.parse().map_err(|e: ErrorKind| { - e.to_error(reader.buffer_position()) - })?); + format = Some(value.parse()?); } - _other => return Err(err!(reader, ErrorKind::UnexpectedAttribute)), + _other => return Err(ErrorKind::UnexpectedAttribute.into()), } } if !name.is_empty() && format.is_some() { return Ok(GlyphBuilder::new(name, format.take().unwrap())); } else { - return Err(err!(reader, ErrorKind::WrongFirstElement)); + return Err(ErrorKind::WrongFirstElement.into()); } } - _other => return Err(err!(reader, ErrorKind::WrongFirstElement)), + _other => return Err(ErrorKind::WrongFirstElement.into()), } } } @@ -550,3 +516,14 @@ impl FromStr for GlifVersion { } } } + +impl From for GlifLoadError { + fn from(src: ErrorKind) -> GlifLoadError { + GlifLoadError::Parse(src) + } +} +impl From for GlifLoadError { + fn from(src: XmlError) -> GlifLoadError { + GlifLoadError::Xml(src) + } +} diff --git a/src/layer.rs b/src/layer.rs index bd7e4072..45713bd2 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -258,12 +258,13 @@ impl Layer { let name = names.get(name); let glyph_path = path.join(glyph_path); - Glyph::load_with_names(&glyph_path, names).map(|mut glyph| { - glyph.name = name.clone(); - (name, Arc::new(glyph)) - }) + Glyph::load_with_names(&glyph_path, names) + .map(|mut glyph| { + glyph.name = name.clone(); + (name, Arc::new(glyph)) + }) + .map_err(|e| Error::GlifLoad { path: glyph_path, inner: e }) }) - //FIXME: come up with a better way of reporting errors than just aborting at first failure .collect::>()?; let layerinfo_path = path.join(LAYER_INFO_FILE); @@ -439,7 +440,7 @@ impl Layer { /// be replaced. /// /// Returns an error if `overwrite` is false but a glyph with the new - /// name exists, or if no glyph with the old name exists. + /// name exists, or if no glyph with the old name exists pub fn rename_glyph(&mut self, old: &str, new: &str, overwrite: bool) -> Result<(), Error> { if !overwrite && self.glyphs.contains_key(new) { Err(Error::DuplicateGlyph { glyph: new.into(), layer: self.name.to_string() }) From c754e22cef3780396239e1866e32c67e7a2122d5 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Wed, 24 Nov 2021 12:50:34 -0500 Subject: [PATCH 02/11] Split PlistError read/write variants, report path The read/write split might not be strictly necessary, but doesn't hurt. Tracking the path feels like a good addition. --- src/error.rs | 32 +++++++++++++++++++++----------- src/font.rs | 12 ++++++++---- src/fontinfo.rs | 10 +++++++--- src/layer.rs | 8 +++++--- src/upconversion.rs | 3 ++- src/write.rs | 7 +++++-- 6 files changed, 48 insertions(+), 24 deletions(-) diff --git a/src/error.rs b/src/error.rs index b0d72396..48aefdf2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -54,8 +54,20 @@ pub enum Error { /// /// [`Glyph`]: crate::Glyph GlifWrite(GlifWriteError), - /// An error that wraps a [PlistError]. - PlistError(PlistError), + /// A plist file could not be read. + PlistLoadError { + /// The path of the relevant file. + path: PathBuf, + /// The underlying error. + error: PlistError, + }, + /// A plist file could not be written. + PlistWriteError { + /// The path of the relevant file. + path: PathBuf, + /// The underlying error. + error: PlistError, + }, /// An error returned when there is invalid fontinfo.plist data. InvalidFontInfo, /// An error returned when there is a problem during fontinfo.plist version up-conversion. @@ -285,7 +297,12 @@ impl std::fmt::Display for Error { Error::GlifWrite(GlifWriteError { name, inner }) => { write!(f, "Failed to save glyph {}, error: '{}'", name, inner) } - Error::PlistError(e) => e.fmt(f), + Error::PlistLoadError { path, error } => { + write!(f, "Error reading plist at path '{}': {}", path.display(), error) + } + Error::PlistWriteError { path, error } => { + write!(f, "Error writing plist to path '{}': {}", path.display(), error) + } Error::InvalidFontInfo => write!(f, "FontInfo contains invalid data"), Error::FontInfoUpconversion => { write!(f, "FontInfo contains invalid data after upconversion") @@ -450,7 +467,7 @@ impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { Error::IoError(inner) => Some(inner), - Error::PlistError(inner) => Some(inner), + Error::PlistLoadError { error, .. } => Some(error), Error::GlifWrite(inner) => Some(&inner.inner), _ => None, } @@ -486,13 +503,6 @@ impl From for Error { } } -#[doc(hidden)] -impl From for Error { - fn from(src: PlistError) -> Error { - Error::PlistError(src) - } -} - #[doc(hidden)] impl From for Error { fn from(src: IoError) -> Error { diff --git a/src/font.rs b/src/font.rs index 28e957a3..680ee961 100644 --- a/src/font.rs +++ b/src/font.rs @@ -202,7 +202,8 @@ impl Font { if !meta_path.exists() { return Err(Error::MissingFile(meta_path.display().to_string())); } - let mut meta: MetaInfo = plist::from_file(meta_path)?; + let mut meta: MetaInfo = plist::from_file(&meta_path) + .map_err(|error| Error::PlistLoadError { path: meta_path, error })?; let lib_path = path.join(LIB_FILE); let mut lib = @@ -569,7 +570,8 @@ impl Font { } fn load_lib(lib_path: &Path) -> Result { - plist::Value::from_file(lib_path)? + plist::Value::from_file(lib_path) + .map_err(|error| Error::PlistLoadError { path: lib_path.to_owned(), error })? .into_dictionary() .ok_or_else(|| Error::ExpectedPlistDictionary(lib_path.to_string_lossy().into_owned())) } @@ -584,13 +586,15 @@ fn load_fontinfo( } fn load_groups(groups_path: &Path) -> Result { - let groups: Groups = plist::from_file(groups_path)?; + let groups: Groups = plist::from_file(groups_path) + .map_err(|error| Error::PlistLoadError { path: groups_path.to_owned(), error })?; validate_groups(&groups).map_err(Error::InvalidGroups)?; Ok(groups) } fn load_kerning(kerning_path: &Path) -> Result { - let kerning: Kerning = plist::from_file(kerning_path)?; + let kerning: Kerning = plist::from_file(kerning_path) + .map_err(|error| Error::PlistLoadError { path: kerning_path.to_owned(), error })?; Ok(kerning) } diff --git a/src/fontinfo.rs b/src/fontinfo.rs index 53a0d9b7..267bd7b2 100644 --- a/src/fontinfo.rs +++ b/src/fontinfo.rs @@ -460,15 +460,18 @@ impl FontInfo { format_version: FormatVersion, lib: &mut Plist, ) -> Result { + let path = path.as_ref(); match format_version { FormatVersion::V3 => { - let mut fontinfo: FontInfo = plist::from_file(path)?; + let mut fontinfo: FontInfo = plist::from_file(path) + .map_err(|error| Error::PlistLoadError { path: path.to_owned(), error })?; fontinfo.validate()?; fontinfo.load_object_libs(lib)?; Ok(fontinfo) } FormatVersion::V2 => { - let fontinfo_v2: FontInfoV2 = plist::from_file(path)?; + let fontinfo_v2: FontInfoV2 = plist::from_file(path) + .map_err(|error| Error::PlistLoadError { path: path.to_owned(), error })?; let fontinfo = FontInfo { ascender: fontinfo_v2.ascender, cap_height: fontinfo_v2.capHeight, @@ -622,7 +625,8 @@ impl FontInfo { Ok(fontinfo) } FormatVersion::V1 => { - let fontinfo_v1: FontInfoV1 = plist::from_file(path)?; + let fontinfo_v1: FontInfoV1 = plist::from_file(path) + .map_err(|error| Error::PlistLoadError { path: path.to_owned(), error })?; let fontinfo = FontInfo { ascender: fontinfo_v1.ascender, cap_height: fontinfo_v1.capHeight, diff --git a/src/layer.rs b/src/layer.rs index 45713bd2..61d270fe 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -44,7 +44,8 @@ impl LayerSet { pub fn load(base_dir: &Path, glyph_names: &NameList) -> Result { let layer_contents_path = base_dir.join(LAYER_CONTENTS_FILE); let to_load: Vec<(LayerName, PathBuf)> = if layer_contents_path.exists() { - plist::from_file(layer_contents_path)? + plist::from_file(&layer_contents_path) + .map_err(|error| Error::PlistLoadError { path: layer_contents_path, error })? } else { vec![(Arc::from(DEFAULT_LAYER_NAME), PathBuf::from(DEFAULT_GLYPHS_DIRNAME))] }; @@ -246,7 +247,8 @@ impl Layer { } // these keys are never used; a future optimization would be to skip the // names and deserialize to a vec; that would not be a one-liner, though. - let contents: BTreeMap = plist::from_file(contents_path)?; + let contents: BTreeMap = plist::from_file(&contents_path) + .map_err(|error| Error::PlistLoadError { path: contents_path, error })?; #[cfg(feature = "rayon")] let iter = contents.par_iter(); @@ -284,7 +286,7 @@ impl Layer { // Ser/de must be done manually... fn layerinfo_from_file(path: &Path) -> Result<(Option, Plist), Error> { let mut info_content = plist::Value::from_file(path) - .map_err(Error::PlistError)? + .map_err(|error| Error::PlistLoadError { path: path.to_owned(), error })? .into_dictionary() .ok_or_else(|| Error::ExpectedPlistDictionary(path.to_string_lossy().into_owned()))?; diff --git a/src/upconversion.rs b/src/upconversion.rs index f2242d63..2afdf92e 100644 --- a/src/upconversion.rs +++ b/src/upconversion.rs @@ -149,7 +149,8 @@ pub(crate) fn upconvert_ufov1_robofab_data( } // Read lib.plist again because it is easier than pulling out the data manually. - let lib_data: LibData = plist::from_file(lib_path)?; + let lib_data: LibData = plist::from_file(lib_path) + .map_err(|error| Error::PlistLoadError { path: lib_path.to_owned(), error })?; // Convert features. let mut features = String::new(); diff --git a/src/write.rs b/src/write.rs index c97f6bd8..78e8d5dc 100644 --- a/src/write.rs +++ b/src/write.rs @@ -117,7 +117,9 @@ pub fn write_plist_value_to_file( ) -> Result<(), Error> { let mut file = File::create(path)?; let writer = BufWriter::new(&mut file); - value.to_writer_xml_with_options(writer, options.xml_options())?; + value + .to_writer_xml_with_options(writer, options.xml_options()) + .map_err(|error| Error::PlistWriteError { path: path.to_owned(), error })?; write_quote_style(&file, options)?; file.sync_all()?; Ok(()) @@ -131,7 +133,8 @@ pub fn write_xml_to_file( ) -> Result<(), Error> { let mut file = File::create(path)?; let buf_writer = BufWriter::new(&mut file); - plist::to_writer_xml_with_options(buf_writer, value, options.xml_options())?; + plist::to_writer_xml_with_options(buf_writer, value, options.xml_options()) + .map_err(|error| Error::PlistWriteError { path: path.to_owned(), error })?; write_quote_style(&file, options)?; file.sync_all()?; Ok(()) From 64356df0b365cea4c9c9e104474c1cf225629e2f Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Fri, 26 Nov 2021 18:20:00 +0000 Subject: [PATCH 03/11] Remove redundant Error::ParseError --- src/error.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/error.rs b/src/error.rs index 48aefdf2..c98dd76c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -41,8 +41,6 @@ pub enum Error { }, /// An error returned when there is an input/output problem during processing IoError(IoError), - /// An error returned when there is an XML parsing problem. - ParseError(XmlError), /// A `.glif` file could not be loaded. GlifLoad { /// The path of the relevant `.glif` file. @@ -289,7 +287,6 @@ impl std::fmt::Display for Error { write!(f, "Glyph '{}' missing from layer '{}'", glyph, layer) } Error::IoError(e) => e.fmt(f), - Error::ParseError(e) => e.fmt(f), Error::InvalidColor(e) => e.fmt(f), Error::GlifLoad { path, inner } => { write!(f, "Error reading glif '{}': '{}'", path.display(), inner) @@ -496,13 +493,6 @@ impl From for Error { } } -#[doc(hidden)] -impl From for Error { - fn from(src: XmlError) -> Error { - Error::ParseError(src) - } -} - #[doc(hidden)] impl From for Error { fn from(src: IoError) -> Error { From 21ab3352141b3e1700d84a496f4ce68e8aa71dfb Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Fri, 26 Nov 2021 18:21:13 +0000 Subject: [PATCH 04/11] Shorten IoError to Io for some variants --- src/error.rs | 14 +++++++------- src/glyph/mod.rs | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/error.rs b/src/error.rs index c98dd76c..96cdcf57 100644 --- a/src/error.rs +++ b/src/error.rs @@ -100,7 +100,7 @@ pub enum Error { #[derive(Debug)] pub enum GlifLoadError { /// An [`std::io::Error`]. - IoError(IoError), + Io(IoError), /// A [`quick_xml::Error`]. Xml(XmlError), /// The .glif file was malformed. @@ -190,8 +190,8 @@ pub enum WriteError { /// If for some reason the implementation of that crate changes, we could /// be affected, although this is very unlikely. InternalLibWriteError, - /// Generic serialization error. Wraps an [IoError]. - IoError(IoError), + /// An error originating in [`std::io`]. + Io(IoError), /// Plist serialization error. Wraps a [PlistError]. Plist(PlistError), } @@ -334,7 +334,7 @@ impl std::fmt::Display for GlifLoadError { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { match self { GlifLoadError::Xml(err) => err.fmt(f), - GlifLoadError::IoError(err) => err.fmt(f), + GlifLoadError::Io(err) => err.fmt(f), GlifLoadError::Parse(err) => err.fmt(f), } } @@ -427,7 +427,7 @@ impl std::fmt::Display for ErrorKind { impl std::fmt::Display for WriteError { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { match self { - WriteError::IoError(err) => err.fmt(f), + WriteError::Io(err) => err.fmt(f), WriteError::Xml(err) => err.fmt(f), WriteError::Plist(err) => err.fmt(f), WriteError::InternalLibWriteError => { @@ -446,7 +446,7 @@ impl std::fmt::Display for GlifWriteError { impl std::error::Error for WriteError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { - WriteError::IoError(inner) => Some(inner), + WriteError::Io(inner) => Some(inner), WriteError::Xml(inner) => Some(inner), WriteError::Plist(inner) => Some(inner), WriteError::InternalLibWriteError => None, @@ -510,7 +510,7 @@ impl From for WriteError { #[doc(hidden)] impl From for WriteError { fn from(src: IoError) -> WriteError { - WriteError::IoError(src) + WriteError::Io(src) } } diff --git a/src/glyph/mod.rs b/src/glyph/mod.rs index 49d0b1d3..f1e7b72b 100644 --- a/src/glyph/mod.rs +++ b/src/glyph/mod.rs @@ -74,7 +74,7 @@ impl Glyph { /// occurs multiple times (such as in components or in different layers). pub fn load_with_names(path: &Path, names: &NameList) -> Result { std::fs::read(path) - .map_err(GlifLoadError::IoError) + .map_err(GlifLoadError::Io) .and_then(|data| parse::GlifParser::from_xml(&data, Some(names))) } From 3fa7185ff1bbabad601a972c52b4b1a8f2fd291f Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Wed, 15 Dec 2021 14:39:54 +0000 Subject: [PATCH 05/11] Strip redundant `Error` suffix from Error variants --- src/error.rs | 18 +++++++++--------- src/font.rs | 8 ++++---- src/fontinfo.rs | 6 +++--- src/layer.rs | 6 +++--- src/upconversion.rs | 2 +- src/write.rs | 4 ++-- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/error.rs b/src/error.rs index 96cdcf57..ad35ecd1 100644 --- a/src/error.rs +++ b/src/error.rs @@ -40,7 +40,7 @@ pub enum Error { glyph: String, }, /// An error returned when there is an input/output problem during processing - IoError(IoError), + Io(IoError), /// A `.glif` file could not be loaded. GlifLoad { /// The path of the relevant `.glif` file. @@ -53,14 +53,14 @@ pub enum Error { /// [`Glyph`]: crate::Glyph GlifWrite(GlifWriteError), /// A plist file could not be read. - PlistLoadError { + PlistLoad { /// The path of the relevant file. path: PathBuf, /// The underlying error. error: PlistError, }, /// A plist file could not be written. - PlistWriteError { + PlistWrite { /// The path of the relevant file. path: PathBuf, /// The underlying error. @@ -286,7 +286,7 @@ impl std::fmt::Display for Error { Error::MissingGlyph { layer, glyph } => { write!(f, "Glyph '{}' missing from layer '{}'", glyph, layer) } - Error::IoError(e) => e.fmt(f), + Error::Io(e) => e.fmt(f), Error::InvalidColor(e) => e.fmt(f), Error::GlifLoad { path, inner } => { write!(f, "Error reading glif '{}': '{}'", path.display(), inner) @@ -294,10 +294,10 @@ impl std::fmt::Display for Error { Error::GlifWrite(GlifWriteError { name, inner }) => { write!(f, "Failed to save glyph {}, error: '{}'", name, inner) } - Error::PlistLoadError { path, error } => { + Error::PlistLoad { path, error } => { write!(f, "Error reading plist at path '{}': {}", path.display(), error) } - Error::PlistWriteError { path, error } => { + Error::PlistWrite { path, error } => { write!(f, "Error writing plist to path '{}': {}", path.display(), error) } Error::InvalidFontInfo => write!(f, "FontInfo contains invalid data"), @@ -463,8 +463,8 @@ impl std::error::Error for GlifWriteError { impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { - Error::IoError(inner) => Some(inner), - Error::PlistLoadError { error, .. } => Some(error), + Error::Io(inner) => Some(inner), + Error::PlistLoad { error, .. } => Some(error), Error::GlifWrite(inner) => Some(&inner.inner), _ => None, } @@ -496,7 +496,7 @@ impl From for Error { #[doc(hidden)] impl From for Error { fn from(src: IoError) -> Error { - Error::IoError(src) + Error::Io(src) } } diff --git a/src/font.rs b/src/font.rs index 680ee961..c3b5712e 100644 --- a/src/font.rs +++ b/src/font.rs @@ -203,7 +203,7 @@ impl Font { return Err(Error::MissingFile(meta_path.display().to_string())); } let mut meta: MetaInfo = plist::from_file(&meta_path) - .map_err(|error| Error::PlistLoadError { path: meta_path, error })?; + .map_err(|error| Error::PlistLoad { path: meta_path, error })?; let lib_path = path.join(LIB_FILE); let mut lib = @@ -571,7 +571,7 @@ impl Font { fn load_lib(lib_path: &Path) -> Result { plist::Value::from_file(lib_path) - .map_err(|error| Error::PlistLoadError { path: lib_path.to_owned(), error })? + .map_err(|error| Error::PlistLoad { path: lib_path.to_owned(), error })? .into_dictionary() .ok_or_else(|| Error::ExpectedPlistDictionary(lib_path.to_string_lossy().into_owned())) } @@ -587,14 +587,14 @@ fn load_fontinfo( fn load_groups(groups_path: &Path) -> Result { let groups: Groups = plist::from_file(groups_path) - .map_err(|error| Error::PlistLoadError { path: groups_path.to_owned(), error })?; + .map_err(|error| Error::PlistLoad { path: groups_path.to_owned(), error })?; validate_groups(&groups).map_err(Error::InvalidGroups)?; Ok(groups) } fn load_kerning(kerning_path: &Path) -> Result { let kerning: Kerning = plist::from_file(kerning_path) - .map_err(|error| Error::PlistLoadError { path: kerning_path.to_owned(), error })?; + .map_err(|error| Error::PlistLoad { path: kerning_path.to_owned(), error })?; Ok(kerning) } diff --git a/src/fontinfo.rs b/src/fontinfo.rs index 267bd7b2..6a12211a 100644 --- a/src/fontinfo.rs +++ b/src/fontinfo.rs @@ -464,14 +464,14 @@ impl FontInfo { match format_version { FormatVersion::V3 => { let mut fontinfo: FontInfo = plist::from_file(path) - .map_err(|error| Error::PlistLoadError { path: path.to_owned(), error })?; + .map_err(|error| Error::PlistLoad { path: path.to_owned(), error })?; fontinfo.validate()?; fontinfo.load_object_libs(lib)?; Ok(fontinfo) } FormatVersion::V2 => { let fontinfo_v2: FontInfoV2 = plist::from_file(path) - .map_err(|error| Error::PlistLoadError { path: path.to_owned(), error })?; + .map_err(|error| Error::PlistLoad { path: path.to_owned(), error })?; let fontinfo = FontInfo { ascender: fontinfo_v2.ascender, cap_height: fontinfo_v2.capHeight, @@ -626,7 +626,7 @@ impl FontInfo { } FormatVersion::V1 => { let fontinfo_v1: FontInfoV1 = plist::from_file(path) - .map_err(|error| Error::PlistLoadError { path: path.to_owned(), error })?; + .map_err(|error| Error::PlistLoad { path: path.to_owned(), error })?; let fontinfo = FontInfo { ascender: fontinfo_v1.ascender, cap_height: fontinfo_v1.capHeight, diff --git a/src/layer.rs b/src/layer.rs index 61d270fe..04cd335d 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -45,7 +45,7 @@ impl LayerSet { let layer_contents_path = base_dir.join(LAYER_CONTENTS_FILE); let to_load: Vec<(LayerName, PathBuf)> = if layer_contents_path.exists() { plist::from_file(&layer_contents_path) - .map_err(|error| Error::PlistLoadError { path: layer_contents_path, error })? + .map_err(|error| Error::PlistLoad { path: layer_contents_path, error })? } else { vec![(Arc::from(DEFAULT_LAYER_NAME), PathBuf::from(DEFAULT_GLYPHS_DIRNAME))] }; @@ -248,7 +248,7 @@ impl Layer { // these keys are never used; a future optimization would be to skip the // names and deserialize to a vec; that would not be a one-liner, though. let contents: BTreeMap = plist::from_file(&contents_path) - .map_err(|error| Error::PlistLoadError { path: contents_path, error })?; + .map_err(|error| Error::PlistLoad { path: contents_path, error })?; #[cfg(feature = "rayon")] let iter = contents.par_iter(); @@ -286,7 +286,7 @@ impl Layer { // Ser/de must be done manually... fn layerinfo_from_file(path: &Path) -> Result<(Option, Plist), Error> { let mut info_content = plist::Value::from_file(path) - .map_err(|error| Error::PlistLoadError { path: path.to_owned(), error })? + .map_err(|error| Error::PlistLoad { path: path.to_owned(), error })? .into_dictionary() .ok_or_else(|| Error::ExpectedPlistDictionary(path.to_string_lossy().into_owned()))?; diff --git a/src/upconversion.rs b/src/upconversion.rs index 2afdf92e..7616d6c8 100644 --- a/src/upconversion.rs +++ b/src/upconversion.rs @@ -150,7 +150,7 @@ pub(crate) fn upconvert_ufov1_robofab_data( // Read lib.plist again because it is easier than pulling out the data manually. let lib_data: LibData = plist::from_file(lib_path) - .map_err(|error| Error::PlistLoadError { path: lib_path.to_owned(), error })?; + .map_err(|error| Error::PlistLoad { path: lib_path.to_owned(), error })?; // Convert features. let mut features = String::new(); diff --git a/src/write.rs b/src/write.rs index 78e8d5dc..bd7fe440 100644 --- a/src/write.rs +++ b/src/write.rs @@ -119,7 +119,7 @@ pub fn write_plist_value_to_file( let writer = BufWriter::new(&mut file); value .to_writer_xml_with_options(writer, options.xml_options()) - .map_err(|error| Error::PlistWriteError { path: path.to_owned(), error })?; + .map_err(|error| Error::PlistWrite { path: path.to_owned(), error })?; write_quote_style(&file, options)?; file.sync_all()?; Ok(()) @@ -134,7 +134,7 @@ pub fn write_xml_to_file( let mut file = File::create(path)?; let buf_writer = BufWriter::new(&mut file); plist::to_writer_xml_with_options(buf_writer, value, options.xml_options()) - .map_err(|error| Error::PlistWriteError { path: path.to_owned(), error })?; + .map_err(|error| Error::PlistWrite { path: path.to_owned(), error })?; write_quote_style(&file, options)?; file.sync_all()?; Ok(()) From 1bbbe89e62820dbec014632fb9d5fc568db5a968 Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Wed, 15 Dec 2021 14:46:29 +0000 Subject: [PATCH 06/11] Fix unrelated lint --- src/layer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/layer.rs b/src/layer.rs index 04cd335d..084b2269 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -403,7 +403,7 @@ impl Layer { GlyphName: Borrow, K: Ord + ?Sized, { - self.glyphs.get_mut(glyph).map(|g| Arc::make_mut(g)) + self.glyphs.get_mut(glyph).map(Arc::make_mut) } /// Returns `true` if this layer contains a glyph with this `name`. From 538903c301355bc7b9144635340dc70381405f20 Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Wed, 15 Dec 2021 15:38:34 +0000 Subject: [PATCH 07/11] Impl Error for GlifLoadError --- src/error.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/error.rs b/src/error.rs index ad35ecd1..5c000d62 100644 --- a/src/error.rs +++ b/src/error.rs @@ -460,6 +460,16 @@ impl std::error::Error for GlifWriteError { } } +impl std::error::Error for GlifLoadError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + GlifLoadError::Io(e) => Some(e), + GlifLoadError::Xml(e) => Some(e), + GlifLoadError::Parse(_) => None, + } + } +} + impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { From 1ea2faece8a18282b5bc2a86310b3f4b3aaa0b08 Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Wed, 15 Dec 2021 19:26:38 +0000 Subject: [PATCH 08/11] Fix typo in example program --- examples/load_save.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/load_save.rs b/examples/load_save.rs index 45b68bc1..6528a92a 100644 --- a/examples/load_save.rs +++ b/examples/load_save.rs @@ -9,7 +9,7 @@ use norad::Font; static HELP: &str = " USAGE: - open_ufo PATH [OUTPATH] + load_save PATH [OUTPATH] If an OUTPATH is provided, the UFO will be saved after opening. "; From 9f4ce2436446873b959bc61f86294e106f090176 Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Wed, 15 Dec 2021 19:27:45 +0000 Subject: [PATCH 09/11] Split Error::Io into Ufo(Load|Write) --- src/error.rs | 39 ++++++++++++++++++---------- src/font.rs | 31 +++++++++++++++-------- src/glyph/mod.rs | 3 ++- src/layer.rs | 2 +- src/write.rs | 66 +++++++++++++++++++++++++----------------------- 5 files changed, 85 insertions(+), 56 deletions(-) diff --git a/src/error.rs b/src/error.rs index 5c000d62..7b198b99 100644 --- a/src/error.rs +++ b/src/error.rs @@ -39,8 +39,20 @@ pub enum Error { /// The glyph name. glyph: String, }, - /// An error returned when there is an input/output problem during processing - Io(IoError), + /// An error returned when there is an input problem during processing + UfoLoad { + /// The path of the relevant file. + path: PathBuf, + /// The underlying error. + inner: IoError, + }, + /// An error returned when there is an output problem during processing + UfoWrite { + /// The path of the relevant file. + path: PathBuf, + /// The underlying error. + inner: IoError, + }, /// A `.glif` file could not be loaded. GlifLoad { /// The path of the relevant `.glif` file. @@ -286,10 +298,15 @@ impl std::fmt::Display for Error { Error::MissingGlyph { layer, glyph } => { write!(f, "Glyph '{}' missing from layer '{}'", glyph, layer) } - Error::Io(e) => e.fmt(f), + Error::UfoLoad { path, inner } => { + write!(f, "Error reading UFO file or directory '{}': '{}'", path.display(), inner) + } + Error::UfoWrite { path, inner } => { + write!(f, "Error writing UFO file or directory '{}': '{}'", path.display(), inner) + } Error::InvalidColor(e) => e.fmt(f), Error::GlifLoad { path, inner } => { - write!(f, "Error reading glif '{}': '{}'", path.display(), inner) + write!(f, "Error reading glif file '{}': '{}'", path.display(), inner) } Error::GlifWrite(GlifWriteError { name, inner }) => { write!(f, "Failed to save glyph {}, error: '{}'", name, inner) @@ -473,9 +490,12 @@ impl std::error::Error for GlifLoadError { impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match self { - Error::Io(inner) => Some(inner), - Error::PlistLoad { error, .. } => Some(error), + Error::GlifLoad { inner, .. } => Some(inner), Error::GlifWrite(inner) => Some(&inner.inner), + Error::PlistLoad { error, .. } => Some(error), + Error::PlistWrite { error, .. } => Some(error), + Error::UfoLoad { inner, .. } => Some(inner), + Error::UfoWrite { inner, .. } => Some(inner), _ => None, } } @@ -503,13 +523,6 @@ impl From for Error { } } -#[doc(hidden)] -impl From for Error { - fn from(src: IoError) -> Error { - Error::Io(src) - } -} - #[doc(hidden)] impl From for WriteError { fn from(src: XmlError) -> WriteError { diff --git a/src/font.rs b/src/font.rs index c3b5712e..37278fe8 100644 --- a/src/font.rs +++ b/src/font.rs @@ -417,9 +417,10 @@ impl Font { for _ in self.images.iter() {} if path.exists() { - fs::remove_dir_all(path)?; + fs::remove_dir_all(path) + .map_err(|inner| Error::UfoWrite { path: path.into(), inner })?; } - fs::create_dir(path)?; + fs::create_dir(path).map_err(|inner| Error::UfoWrite { path: path.into(), inner })?; // we want to always set ourselves as the creator when serializing, // but we also don't have mutable access to self. @@ -464,10 +465,13 @@ impl Font { if !self.features.is_empty() { // Normalize feature files with line feed line endings // This is consistent with the line endings serialized in glif and plist files + let feature_file_path = path.join(FEATURES_FILE); if self.features.as_bytes().contains(&b'\r') { - fs::write(path.join(FEATURES_FILE), self.features.replace("\r\n", "\n"))?; + fs::write(&feature_file_path, self.features.replace("\r\n", "\n")) + .map_err(|inner| Error::UfoWrite { path: feature_file_path, inner })?; } else { - fs::write(path.join(FEATURES_FILE), &self.features)?; + fs::write(&feature_file_path, &self.features) + .map_err(|inner| Error::UfoWrite { path: feature_file_path, inner })?; } } @@ -486,8 +490,12 @@ impl Font { match contents { Ok(data) => { let destination = data_dir.join(data_path); - fs::create_dir_all(&destination.parent().unwrap())?; - fs::write(destination, &*data)?; + let destination_parent = destination.parent().unwrap(); + fs::create_dir_all(&destination_parent).map_err(|inner| { + Error::UfoWrite { path: destination_parent.into(), inner } + })?; + fs::write(&destination, &*data) + .map_err(|inner| Error::UfoWrite { path: destination, inner })?; } Err(e) => return Err(Error::InvalidStoreEntry(data_path.clone(), e)), } @@ -496,12 +504,14 @@ impl Font { if !self.images.is_empty() { let images_dir = path.join(IMAGES_DIR); - fs::create_dir(&images_dir)?; // Only a flat directory. + fs::create_dir(&images_dir) // Only a flat directory. + .map_err(|inner| Error::UfoWrite { path: images_dir.to_owned(), inner })?; for (image_path, contents) in self.images.iter() { match contents { Ok(data) => { let destination = images_dir.join(image_path); - fs::write(destination, &*data)?; + fs::write(&destination, &*data) + .map_err(|inner| Error::UfoWrite { path: destination, inner })?; } Err(e) => return Err(Error::InvalidStoreEntry(image_path.clone(), e)), } @@ -599,7 +609,8 @@ fn load_kerning(kerning_path: &Path) -> Result { } fn load_features(features_path: &Path) -> Result { - let features = fs::read_to_string(features_path)?; + let features = fs::read_to_string(features_path) + .map_err(|inner| Error::UfoLoad { path: features_path.into(), inner })?; Ok(features) } @@ -839,7 +850,7 @@ mod tests { fn save_with_options_with_writeoptions_parameter() -> Result<(), Error> { let opt = WriteOptions::default(); let ufo = Font::default(); - let tmp = TempDir::new("test")?; + let tmp = TempDir::new("test").unwrap(); ufo.save_with_options(tmp, &opt) } } diff --git a/src/glyph/mod.rs b/src/glyph/mod.rs index f1e7b72b..d212cd0a 100644 --- a/src/glyph/mod.rs +++ b/src/glyph/mod.rs @@ -94,7 +94,8 @@ impl Glyph { } let data = self.encode_xml_with_options(opts)?; - std::fs::write(path, &data)?; + std::fs::write(path, &data) + .map_err(|inner| Error::UfoWrite { path: path.into(), inner })?; Ok(()) } diff --git a/src/layer.rs b/src/layer.rs index 084b2269..f55c6dff 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -346,7 +346,7 @@ impl Layer { /// /// The path should not exist. pub fn save_with_options(&self, path: &Path, opts: &WriteOptions) -> Result<(), Error> { - fs::create_dir(&path)?; + fs::create_dir(&path).map_err(|inner| Error::UfoWrite { path: path.into(), inner })?; crate::write::write_xml_to_file(&path.join(CONTENTS_FILE), &self.contents, opts)?; self.layerinfo_to_file_if_needed(path, opts)?; diff --git a/src/write.rs b/src/write.rs index bd7fe440..ade81635 100644 --- a/src/write.rs +++ b/src/write.rs @@ -115,13 +115,15 @@ pub fn write_plist_value_to_file( value: &plist::Value, options: &WriteOptions, ) -> Result<(), Error> { - let mut file = File::create(path)?; + let mut file = + File::create(path).map_err(|inner| Error::UfoWrite { path: path.into(), inner })?; let writer = BufWriter::new(&mut file); value .to_writer_xml_with_options(writer, options.xml_options()) .map_err(|error| Error::PlistWrite { path: path.to_owned(), error })?; - write_quote_style(&file, options)?; - file.sync_all()?; + write_quote_style(&file, options) + .map_err(|inner| Error::UfoWrite { path: path.into(), inner })?; + file.sync_all().map_err(|inner| Error::UfoWrite { path: path.into(), inner })?; Ok(()) } @@ -131,17 +133,19 @@ pub fn write_xml_to_file( value: &impl serde::Serialize, options: &WriteOptions, ) -> Result<(), Error> { - let mut file = File::create(path)?; + let mut file = + File::create(path).map_err(|inner| Error::UfoWrite { path: path.into(), inner })?; let buf_writer = BufWriter::new(&mut file); plist::to_writer_xml_with_options(buf_writer, value, options.xml_options()) .map_err(|error| Error::PlistWrite { path: path.to_owned(), error })?; - write_quote_style(&file, options)?; - file.sync_all()?; + write_quote_style(&file, options) + .map_err(|inner| Error::UfoWrite { path: path.into(), inner })?; + file.sync_all().map_err(|inner| Error::UfoWrite { path: path.into(), inner })?; Ok(()) } /// Write XML declarations with custom quote formatting options. -pub fn write_quote_style(file: &File, options: &WriteOptions) -> Result<(), Error> { +pub fn write_quote_style(file: &File, options: &WriteOptions) -> Result<(), std::io::Error> { // Optionally modify the XML declaration quote style match options.quote_style { QuoteChar::Single => { @@ -169,16 +173,16 @@ mod tests { let opt = WriteOptions::default(); let plist_read = Value::from_file("testdata/MutatorSansLightWide.ufo/lib.plist") .expect("failed to read plist"); - let tmp = TempDir::new("test")?; + let tmp = TempDir::new("test").unwrap(); let filepath = tmp.path().join("lib.plist"); write_plist_value_to_file(&filepath, &plist_read, &opt)?; - let plist_write = fs::read_to_string(filepath)?; + let plist_write = fs::read_to_string(filepath).unwrap(); let str_list = plist_write.split('\n').collect::>(); assert_eq!(str_list[0], ""); // default uses double quotes assert_eq!(str_list[3], ""); // no space char at first dict tag assert_eq!(str_list[4], "\tcom.defcon.sortDescriptor"); // single tab spacing by default assert_eq!(str_list[6], "\t\t"); // second level should use two tab char - tmp.close()?; + tmp.close().unwrap(); Ok(()) } @@ -187,16 +191,16 @@ mod tests { let opt = WriteOptions::default().whitespace(" "); let plist_read = Value::from_file("testdata/MutatorSansLightWide.ufo/lib.plist") .expect("failed to read plist"); - let tmp = TempDir::new("test")?; + let tmp = TempDir::new("test").unwrap(); let filepath = tmp.path().join("lib.plist"); write_plist_value_to_file(&filepath, &plist_read, &opt)?; - let plist_write = fs::read_to_string(filepath)?; + let plist_write = fs::read_to_string(filepath).unwrap(); let str_list = plist_write.split('\n').collect::>(); assert_eq!(str_list[0], ""); // default uses double quotes assert_eq!(str_list[3], ""); // no space char at first dict tag assert_eq!(str_list[4], " com.defcon.sortDescriptor"); // should use two space char assert_eq!(str_list[6], " "); // second level should use four space char - tmp.close()?; + tmp.close().unwrap(); Ok(()) } @@ -205,16 +209,16 @@ mod tests { let opt = WriteOptions::default().whitespace(" ").quote_char(QuoteChar::Single); let plist_read = Value::from_file("testdata/MutatorSansLightWide.ufo/lib.plist") .expect("failed to read plist"); - let tmp = TempDir::new("test")?; + let tmp = TempDir::new("test").unwrap(); let filepath = tmp.path().join("lib.plist"); write_plist_value_to_file(&filepath, &plist_read, &opt)?; - let plist_write = fs::read_to_string(filepath)?; + let plist_write = fs::read_to_string(filepath).unwrap(); let str_list = plist_write.split('\n').collect::>(); assert_eq!(str_list[0], ""); // should use single quotes assert_eq!(str_list[3], ""); // no space char at first dict tag assert_eq!(str_list[4], " com.defcon.sortDescriptor"); // should use two space char assert_eq!(str_list[6], " "); // second level should use four space char - tmp.close()?; + tmp.close().unwrap(); Ok(()) } @@ -223,12 +227,12 @@ mod tests { let opt = WriteOptions::default(); let plist_read = Value::from_file("testdata/lineendings/Tester-LineEndings.ufo/lib.plist") .expect("failed to read plist"); - let tmp = TempDir::new("test")?; + let tmp = TempDir::new("test").unwrap(); let filepath = tmp.path().join("lib.plist"); write_plist_value_to_file(&filepath, &plist_read, &opt)?; - let plist_write = fs::read_to_string(filepath)?; + let plist_write = fs::read_to_string(filepath).unwrap(); assert!(plist_write.starts_with("\n\n")); - tmp.close()?; + tmp.close().unwrap(); Ok(()) } @@ -237,15 +241,15 @@ mod tests { let opt = WriteOptions::default(); let plist_read = Value::from_file("testdata/MutatorSansLightWide.ufo/fontinfo.plist") .expect("failed to read plist"); - let tmp = TempDir::new("test")?; + let tmp = TempDir::new("test").unwrap(); let filepath = tmp.path().join("fontinfo.plist"); write_plist_value_to_file(&filepath, &plist_read, &opt)?; - let plist_write = fs::read_to_string(filepath)?; + let plist_write = fs::read_to_string(filepath).unwrap(); let str_list = plist_write.split('\n').collect::>(); assert_eq!(str_list[0], ""); // default uses double quotes assert_eq!(str_list[3], ""); // no space char at first dict tag assert_eq!(str_list[4], "\tascender"); // single tab level spacing by default - tmp.close()?; + tmp.close().unwrap(); Ok(()) } @@ -254,15 +258,15 @@ mod tests { let opt = WriteOptions::default().whitespace(" "); let plist_read = Value::from_file("testdata/MutatorSansLightWide.ufo/fontinfo.plist") .expect("failed to read plist"); - let tmp = TempDir::new("test")?; + let tmp = TempDir::new("test").unwrap(); let filepath = tmp.path().join("fontinfo.plist"); write_plist_value_to_file(&filepath, &plist_read, &opt)?; - let plist_write = fs::read_to_string(filepath)?; + let plist_write = fs::read_to_string(filepath).unwrap(); let str_list = plist_write.split('\n').collect::>(); assert_eq!(str_list[0], ""); // default uses double quotes assert_eq!(str_list[3], ""); // no space char at first dict tag assert_eq!(str_list[4], " ascender"); // should use two space char - tmp.close()?; + tmp.close().unwrap(); Ok(()) } @@ -271,15 +275,15 @@ mod tests { let opt = WriteOptions::default().whitespace(" ").quote_char(QuoteChar::Single); let plist_read = Value::from_file("testdata/MutatorSansLightWide.ufo/fontinfo.plist") .expect("failed to read plist"); - let tmp = TempDir::new("test")?; + let tmp = TempDir::new("test").unwrap(); let filepath = tmp.path().join("fontinfo.plist"); write_plist_value_to_file(&filepath, &plist_read, &opt)?; - let plist_write = fs::read_to_string(filepath)?; + let plist_write = fs::read_to_string(filepath).unwrap(); let str_list = plist_write.split('\n').collect::>(); assert_eq!(str_list[0], ""); // should use single quotes assert_eq!(str_list[3], ""); // no space char at first dict tag assert_eq!(str_list[4], " ascender"); // should use two space char - tmp.close()?; + tmp.close().unwrap(); Ok(()) } @@ -289,12 +293,12 @@ mod tests { let plist_read = Value::from_file("testdata/lineendings/Tester-LineEndings.ufo/fontinfo.plist") .expect("failed to read plist"); - let tmp = TempDir::new("test")?; + let tmp = TempDir::new("test").unwrap(); let filepath = tmp.path().join("fontinfo.plist"); write_plist_value_to_file(&filepath, &plist_read, &opt)?; - let plist_write = fs::read_to_string(filepath)?; + let plist_write = fs::read_to_string(filepath).unwrap(); assert!(plist_write.starts_with("\n\n")); - tmp.close()?; + tmp.close().unwrap(); Ok(()) } } From 60abac5b545d820463db312f70f2287f350afdb2 Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Thu, 16 Dec 2021 17:25:09 +0000 Subject: [PATCH 10/11] Try and properly chain errors --- src/error.rs | 58 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/src/error.rs b/src/error.rs index 7b198b99..64c01d9e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -298,24 +298,24 @@ impl std::fmt::Display for Error { Error::MissingGlyph { layer, glyph } => { write!(f, "Glyph '{}' missing from layer '{}'", glyph, layer) } - Error::UfoLoad { path, inner } => { - write!(f, "Error reading UFO file or directory '{}': '{}'", path.display(), inner) + Error::InvalidColor(e) => e.fmt(f), + Error::UfoLoad { path, .. } => { + write!(f, "Failed to read file or directory '{}'", path.display()) } - Error::UfoWrite { path, inner } => { - write!(f, "Error writing UFO file or directory '{}': '{}'", path.display(), inner) + Error::UfoWrite { path, .. } => { + write!(f, "Failed to write file or directory '{}'", path.display()) } - Error::InvalidColor(e) => e.fmt(f), - Error::GlifLoad { path, inner } => { - write!(f, "Error reading glif file '{}': '{}'", path.display(), inner) + Error::GlifLoad { path, .. } => { + write!(f, "Failed to read glyph file from '{}'", path.display()) } - Error::GlifWrite(GlifWriteError { name, inner }) => { - write!(f, "Failed to save glyph {}, error: '{}'", name, inner) + Error::GlifWrite(GlifWriteError { name, .. }) => { + write!(f, "Failed to write out glyph '{}'", name) } - Error::PlistLoad { path, error } => { - write!(f, "Error reading plist at path '{}': {}", path.display(), error) + Error::PlistLoad { path, .. } => { + write!(f, "Failed to read Plist file from '{}'", path.display()) } - Error::PlistWrite { path, error } => { - write!(f, "Error writing plist to path '{}': {}", path.display(), error) + Error::PlistWrite { path, .. } => { + write!(f, "Failed to write Plist file to '{}'", path.display()) } Error::InvalidFontInfo => write!(f, "FontInfo contains invalid data"), Error::FontInfoUpconversion => { @@ -338,8 +338,8 @@ impl std::fmt::Display for Error { Error::MissingUfoDir(path) => { write!(f, "{} directory was not found", path) } - Error::InvalidStoreEntry(path, e) => { - write!(f, "Store entry '{}' error: {}", path.display(), e) + Error::InvalidStoreEntry(path, _) => { + write!(f, "Store entry '{}' is invalid", path.display()) } #[cfg(feature = "kurbo")] Error::ConvertContour(cause) => write!(f, "Failed to convert contour: '{}'", cause), @@ -350,9 +350,9 @@ impl std::fmt::Display for Error { impl std::fmt::Display for GlifLoadError { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { match self { - GlifLoadError::Xml(err) => err.fmt(f), - GlifLoadError::Io(err) => err.fmt(f), - GlifLoadError::Parse(err) => err.fmt(f), + GlifLoadError::Xml(_) => write!(f, "Failed to read or parse XML structure"), + GlifLoadError::Io(_) => write!(f, "Failed to read file"), + GlifLoadError::Parse(e) => write!(f, "Failed to parse glyph data: {}", e), } } } @@ -373,13 +373,22 @@ impl std::fmt::Display for StoreError { NotPlainFile => write!(f, "Only plain files are allowed, no symlinks."), Subdir => write!(f, "Subdirectories are not allowed in the image store."), InvalidImage => write!(f, "An image must be a valid PNG."), - Io(e) => { - write!(f, "Encountered an IO error while trying to load content: {}.", e) + Io(_) => { + write!(f, "Encountered an IO error while trying to load content") } } } } +impl std::error::Error for StoreError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + match self { + StoreError::Io(e) => Some(e), + _ => None, + } + } +} + impl std::fmt::Display for GroupsValidationError { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { match self { @@ -444,9 +453,9 @@ impl std::fmt::Display for ErrorKind { impl std::fmt::Display for WriteError { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { match self { - WriteError::Io(err) => err.fmt(f), - WriteError::Xml(err) => err.fmt(f), - WriteError::Plist(err) => err.fmt(f), + WriteError::Io(_) => write!(f, "Error writing to disk"), + WriteError::Xml(_) => write!(f, "Error writing an XML file to disk"), + WriteError::Plist(_) => write!(f, "Error writing a Plist file to disk"), WriteError::InternalLibWriteError => { write!(f, "Internal error while writing lib data. Please open an issue.") } @@ -456,7 +465,7 @@ impl std::fmt::Display for WriteError { impl std::fmt::Display for GlifWriteError { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - write!(f, "Failed to write glyph '{}': {}", self.name, self.inner) + write!(f, "Failed to write glyph '{}'", self.name) } } @@ -496,6 +505,7 @@ impl std::error::Error for Error { Error::PlistWrite { error, .. } => Some(error), Error::UfoLoad { inner, .. } => Some(inner), Error::UfoWrite { inner, .. } => Some(inner), + Error::InvalidStoreEntry(_, e) => Some(e), _ => None, } } From 7c8688699d6bfd50f18951df89adbac6cfc86229 Mon Sep 17 00:00:00 2001 From: Nikolaus Waxweiler Date: Thu, 16 Dec 2021 18:55:12 +0000 Subject: [PATCH 11/11] Remove unused GlifError --- src/error.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/error.rs b/src/error.rs index 64c01d9e..d808d053 100644 --- a/src/error.rs +++ b/src/error.rs @@ -170,17 +170,6 @@ impl InvalidColorString { } } -/// An error representing a failure during .glif file parsing. -#[derive(Debug)] -pub struct GlifError { - /// The glif file path. - pub path: Option, - /// The buffer position. - pub position: usize, - /// The kind of error. - pub kind: ErrorKind, -} - /// An error when attempting to write a .glif file. #[derive(Debug)] pub struct GlifWriteError {