From 50c6e72a101152c5f237f1648b47adf6e11cf861 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Tue, 4 Jan 2022 11:30:06 -0500 Subject: [PATCH] Only store Arc if 'druid' feature is enabled This was motivated by seeing some code where a user (@ctrlcctrlv) was jumping through hoops to get around the fact that we were using `Arc`. `Arc` was only used originally because it is important for druid, but I'm not sure it makes sense as a more general design. With this patch, we will only use `Arc` if the user enables the 'druid' feature. The default API now operates on `Glyph` objects directly; where needed there are new _raw methods, behind the 'druid' feature, that operaate on Arc. --- src/font.rs | 3 +- src/layer.rs | 108 ++++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 99 insertions(+), 12 deletions(-) diff --git a/src/font.rs b/src/font.rs index 0e69b24c..259e2b39 100644 --- a/src/font.rs +++ b/src/font.rs @@ -5,7 +5,6 @@ use std::borrow::Borrow; use std::fs; use std::path::{Path, PathBuf}; -use std::sync::Arc; use crate::datastore::{DataStore, ImageStore}; use crate::fontinfo::FontInfo; @@ -560,7 +559,7 @@ impl Font { /// Returns a reference to the glyph with the given name _in the default /// layer_. - pub fn get_glyph(&self, key: &K) -> Option<&Arc> + pub fn get_glyph(&self, key: &K) -> Option<&Glyph> where GlyphName: Borrow, K: Ord + ?Sized, diff --git a/src/layer.rs b/src/layer.rs index 30078f0e..4a72c976 100644 --- a/src/layer.rs +++ b/src/layer.rs @@ -4,6 +4,9 @@ use std::fs; use std::path::{Path, PathBuf}; use std::sync::Arc; +#[cfg(feature = "druid")] +use std::ops::Deref; + #[cfg(feature = "rayon")] use rayon::prelude::*; @@ -186,7 +189,10 @@ impl Default for LayerSet { /// [UFO layer]: http://unifiedfontobject.org/versions/ufo3/glyphs/ #[derive(Debug, Clone, PartialEq)] pub struct Layer { + #[cfg(feature = "druid")] pub(crate) glyphs: BTreeMap>, + #[cfg(not(feature = "druid"))] + pub(crate) glyphs: BTreeMap, pub(crate) name: LayerName, pub(crate) path: PathBuf, contents: BTreeMap, @@ -263,7 +269,14 @@ impl Layer { Glyph::load_with_names(&glyph_path, names) .map(|mut glyph| { glyph.name = name.clone(); - (name, Arc::new(glyph)) + #[cfg(feature = "druid")] + { + (name, Arc::new(glyph)) + } + #[cfg(not(feature = "druid"))] + { + (name, glyph) + } }) .map_err(|source| Error::GlifLoad { path: glyph_path, source }) }) @@ -376,7 +389,24 @@ impl Layer { } /// Returns a reference to the glyph with the given name, if it exists. - pub fn get_glyph(&self, glyph: &K) -> Option<&Arc> + pub fn get_glyph(&self, glyph: &K) -> Option<&Glyph> + where + GlyphName: Borrow, + K: Ord + ?Sized, + { + #[cfg(feature = "druid")] + { + return self.glyphs.get(glyph).map(|g| g.deref()); + } + #[cfg(not(feature = "druid"))] + { + self.glyphs.get(glyph) + } + } + + /// Returns a reference to the given glyph, behind an `Arc`, if it exists. + #[cfg(feature = "druid")] + pub fn get_glyph_raw(&self, glyph: &K) -> Option<&Arc> where GlyphName: Borrow, K: Ord + ?Sized, @@ -390,7 +420,14 @@ impl Layer { GlyphName: Borrow, K: Ord + ?Sized, { - self.glyphs.get_mut(glyph).map(Arc::make_mut) + #[cfg(feature = "druid")] + { + self.glyphs.get_mut(glyph).map(Arc::make_mut) + } + #[cfg(not(feature = "druid"))] + { + self.glyphs.get_mut(glyph) + } } /// Returns `true` if this layer contains a glyph with this `name`. @@ -402,7 +439,11 @@ impl Layer { /// /// If the glyph does not previously exist, the filename is calculated from /// the glyph's name. - pub fn insert_glyph(&mut self, glyph: impl Into>) { + pub fn insert_glyph( + &mut self, + #[cfg(feature = "druid")] glyph: impl Into>, + #[cfg(not(feature = "druid"))] glyph: impl Into, + ) { let glyph = glyph.into(); if !self.contents.contains_key(&glyph.name) { let path = crate::util::default_file_name_for_glyph_name(&glyph.name); @@ -418,8 +459,26 @@ impl Layer { } /// Remove the named glyph from this layer and return it, if it exists. - pub fn remove_glyph(&mut self, name: &str) -> Option> { + /// + /// **Note**: If the `druid` feature is enabled, this will not return the + /// removed `Glyph` if there are any other outstanding references to it, + /// although it will still be removed. In this case, consider using the + /// `remove_glyph_raw` method instead. + pub fn remove_glyph(&mut self, name: &str) -> Option { self.contents.remove(name); + #[cfg(feature = "druid")] + { + self.glyphs.remove(name).and_then(|g| Arc::try_unwrap(g).ok()) + } + #[cfg(not(feature = "druid"))] + { + self.glyphs.remove(name) + } + } + + /// Remove the named glyph and return it, including the containing `Arc`. + #[cfg(feature = "druid")] + pub fn remove_glyph_raw(&mut self, name: &str) -> Option> { self.glyphs.remove(name) } @@ -436,21 +495,50 @@ impl Layer { } else if !self.glyphs.contains_key(old) { Err(Error::MissingGlyph { glyph: old.into(), layer: self.name.to_string() }) } else { - let mut g = self.remove_glyph(old).unwrap(); - Arc::make_mut(&mut g).name = new.into(); - self.insert_glyph(g); + #[cfg(feature = "druid")] + { + let mut g = self.remove_glyph_raw(old).unwrap(); + Arc::make_mut(&mut g).name = new.into(); + self.insert_glyph(g); + } + #[cfg(not(feature = "druid"))] + { + let mut g = self.remove_glyph(old).unwrap(); + g.name = new.into(); + self.insert_glyph(g); + } Ok(()) } } /// Returns an iterator over the glyphs in this layer. - pub fn iter(&self) -> impl Iterator> + '_ { + pub fn iter(&self) -> impl Iterator + '_ { + #[cfg(feature = "druid")] + { + self.glyphs.values().map(|g| g.deref()) + } + #[cfg(not(feature = "druid"))] + { + self.glyphs.values() + } + } + + /// Returns an iterator over the glyphs in this layer. + #[cfg(feature = "druid")] + pub fn iter_raw(&self) -> impl Iterator> + '_ { self.glyphs.values() } /// Returns an iterator over the glyphs in this layer, mutably. pub fn iter_mut(&mut self) -> impl Iterator { - self.glyphs.values_mut().map(Arc::make_mut) + #[cfg(feature = "druid")] + { + self.glyphs.values_mut().map(Arc::make_mut) + } + #[cfg(not(feature = "druid"))] + { + self.glyphs.values_mut() + } } /// Returns the path to the .glif file of a given glyph `name`.