refactor fontsets making them optional on the symbolizer and removing the dangerous default contructor - closes #1483 (TODO - consider modifying insert_fontset to only take single arg of fontset instance)

bindings/python/mapnik_fontset.cpp

@@ -33,8 +33,13 @@ using mapnik::font_set;
 void export_fontset ()
 {
     using namespace boost::python;
-    class_<font_set>("FontSet", init<>("default fontset constructor")
+    class_<font_set>("FontSet", init<std::string const&>("default fontset constructor")
         )
+        .add_property("name",
+                       make_function(&font_set::get_name,return_value_policy<copy_const_reference>()),
+                       &font_set::set_name,
+                      "Get/Set the name of the FontSet.\n"
+            )
         .def("add_face_name",&font_set::add_face_name,
              (arg("name")),
              "Add a face-name to the fontset.\n"

bindings/python/mapnik_python.cpp

@@ -617,6 +617,7 @@ BOOST_PYTHON_MODULE(_mapnik)
     def("has_pycairo", &has_pycairo, "Get pycairo module status");
 
     python_optional<mapnik::stroke>();
+    python_optional<mapnik::font_set>();
     python_optional<mapnik::color>();
     python_optional<mapnik::box2d<double> >();
     python_optional<mapnik::composite_mode_e>();

bindings/python/mapnik_text_placement.cpp

@@ -402,9 +402,9 @@ void export_text_placement()
     class_with_converter<char_properties>
         ("CharProperties")
         .def_readwrite_convert("text_transform", &char_properties::text_transform)
+        .def_readwrite_convert("fontset", &char_properties::fontset)
         .def(init<char_properties const&>()) //Copy constructor
         .def_readwrite("face_name", &char_properties::face_name)
-        .def_readwrite("fontset", &char_properties::fontset)
         .def_readwrite("text_size", &char_properties::text_size)
         .def_readwrite("character_spacing", &char_properties::character_spacing)
         .def_readwrite("line_spacing", &char_properties::line_spacing)

include/mapnik/font_engine_freetype.hpp

@@ -333,11 +333,11 @@ public:
         return face_set;
     }
 
-    face_set_ptr get_face_set(std::string const& name, font_set const& fset)
+    face_set_ptr get_face_set(std::string const& name, boost::optional<font_set> fset)
     {
-        if (fset.size() > 0)
+        if (fset && fset->size() > 0)
         {
-            return get_face_set(fset);
+            return get_face_set(*fset);
         }
         else
         {

include/mapnik/font_set.hpp

@@ -35,7 +35,6 @@ namespace mapnik
 class MAPNIK_DECL font_set
 {
 public:
-    font_set();
     font_set(std::string const& name);
     font_set(font_set const& rhs);
     font_set& operator=(font_set const& rhs);

include/mapnik/text_properties.hpp

@@ -33,6 +33,7 @@
 #include <map>
 
 // boost
+#include <boost/optional.hpp>
 #include <boost/property_tree/ptree.hpp>
 
 namespace mapnik
@@ -58,7 +59,7 @@ struct char_properties
     /** Write object to XML ptree. */
     void to_xml(boost::property_tree::ptree &node, bool explicit_defaults, char_properties const& dfl=char_properties()) const;
     std::string face_name;
-    font_set fontset;
+    boost::optional<font_set> fontset;
     double text_size;
     double character_spacing;
     double line_spacing; //Largest total height (fontsize+line_spacing) per line is chosen

include/mapnik/text_symbolizer.hpp

@@ -92,7 +92,7 @@ struct MAPNIK_DECL text_symbolizer : public symbolizer_base
     void set_text_size(double size);
     std::string const& get_face_name() const func_deprecated;
     void set_face_name(std::string face_name);
-    font_set const& get_fontset() const func_deprecated;
+    boost::optional<font_set> const& get_fontset() const func_deprecated;
     void set_fontset(font_set const& fset);
     color const& get_fill() const func_deprecated;
     void set_fill(color const& fill);

src/font_set.cpp

@@ -29,8 +29,6 @@
 
 namespace mapnik
 {
-font_set::font_set()
-    : name_("") {}
 
 font_set::font_set(std::string const& name)
     : name_(name) {}

src/load_map.cpp

@@ -1130,7 +1130,7 @@ void map_parser::parse_text_symbolizer(rule & rule, xml_node const& sym)
             placement_finder->defaults.from_xml(sym, fontsets_);
         }
         if (strict_ &&
-            !placement_finder->defaults.format.fontset.size())
+            !placement_finder->defaults.format.fontset->size())
         {
             ensure_font_face(placement_finder->defaults.format.face_name);
         }
@@ -1159,7 +1159,7 @@ void map_parser::parse_shield_symbolizer(rule & rule, xml_node const& sym)
         }
         placement_finder->defaults.from_xml(sym, fontsets_);
         if (strict_ &&
-            !placement_finder->defaults.format.fontset.size())
+            !placement_finder->defaults.format.fontset->size())
         {
             ensure_font_face(placement_finder->defaults.format.face_name);
         }

src/map.cpp

@@ -159,6 +159,10 @@ boost::optional<feature_type_style const&> Map::find_style(std::string const& na
 
 bool Map::insert_fontset(std::string const& name, font_set const& fontset)
 {
+    if (fontset.get_name() != name)
+    {
+        throw mapnik::config_error("Fontset name must match the name used to reference it on the map");
+    }
     return fontsets_.insert(make_pair(name, fontset)).second;
 }
 

src/processed_text.cpp

@@ -65,22 +65,22 @@ string_info &processed_text::get_string_info()
         face_set_ptr faces = font_manager_.get_face_set(p.face_name, p.fontset);
         if (faces->size() == 0)
         {
-            if (!p.fontset.get_name().empty())
+            if (p.fontset && !p.fontset->get_name().empty())
             {
-                if (p.fontset.size())
+                if (p.fontset->size())
                 {
                     if (!p.face_name.empty())
                     {
-                        throw config_error("Unable to find specified font face '" + p.face_name + "' in font set: '" + p.fontset.get_name() + "'");
+                        throw config_error("Unable to find specified font face '" + p.face_name + "' in font set: '" + p.fontset->get_name() + "'");
                     }
                     else
                     {
-                        throw config_error("No valid font face could be loaded for font set: '" + p.fontset.get_name() + "'");
+                        throw config_error("No valid font face could be loaded for font set: '" + p.fontset->get_name() + "'");
                     }
                 }
                 else
                 {
-                    throw config_error("Font set '" + p.fontset.get_name() + "' does not contain any Font face-name entries");           
+                    throw config_error("Font set '" + p.fontset->get_name() + "' does not contain any Font face-name entries");
                 }
             }
             else if (!p.face_name.empty())

src/text_properties.cpp

@@ -293,11 +293,11 @@ void char_properties::from_xml(xml_node const& sym, fontset_map const& fontsets)
             throw config_error("Unable to find any fontset named '" + *fontset_name_ + "'", sym);
         }
     }
-    if (!face_name.empty() && !fontset.get_name().empty())
+    if (!face_name.empty() && fontset)
     {
         throw config_error("Can't have both face-name and fontset-name", sym);
     }
-    if (face_name.empty() && fontset.get_name().empty())
+    if (face_name.empty() && !fontset)
     {
         throw config_error("Must have face-name or fontset-name", sym);
     }
@@ -305,11 +305,9 @@ void char_properties::from_xml(xml_node const& sym, fontset_map const& fontsets)
 
 void char_properties::to_xml(boost::property_tree::ptree &node, bool explicit_defaults, char_properties const &dfl) const
 {
-    std::string const& fontset_name = fontset.get_name();
-    std::string const& dfl_fontset_name = dfl.fontset.get_name();
-    if (fontset_name != dfl_fontset_name || explicit_defaults)
+    if (fontset)
     {
-        set_attr(node, "fontset-name", fontset_name);
+        set_attr(node, "fontset-name", fontset->get_name());
     }
 
     if (face_name != dfl.face_name || explicit_defaults)

src/text_symbolizer.cpp

@@ -173,7 +173,7 @@ void text_symbolizer::set_fontset(font_set const& fontset)
     placement_options_->defaults.format.fontset = fontset;
 }
 
-font_set const& text_symbolizer::get_fontset() const
+boost::optional<font_set> const& text_symbolizer::get_fontset() const
 {
     return placement_options_->defaults.format.fontset;
 }

tests/python_tests/fontset_test.py

@@ -19,13 +19,17 @@ def test_loading_fontset_from_map():
 
 def test_loading_fontset_from_python():
     m = mapnik.Map(256,256)
-    fset = mapnik.FontSet('my-set')
+    fset = mapnik.FontSet('foo')
     fset.add_face_name('Comic Sans')
     fset.add_face_name('Papyrus')
+    eq_(fset.name,'foo')
+    fset.name = 'my-set'
+    eq_(fset.name,'my-set')
     m.append_fontset('my-set', fset)
     sty = mapnik.Style()
     rule = mapnik.Rule()
     tsym = mapnik.TextSymbolizer()
+    eq_(tsym.fontset,None)
     tsym.fontset = fset
     rule.symbols.append(tsym)
     sty.rules.append(rule)

tests/python_tests/object_test.py

@@ -155,7 +155,7 @@ def test_shield_symbolizer_init():
     # 11c34b1: default transform list is empty, not identity matrix
     eq_(s.transform, '')
 
-    eq_(len(s.fontset.names), 0)
+    eq_(s.fontset, None)
 
 # ShieldSymbolizer missing image file
 # images paths are now PathExpressions are evaluated at runtime