refactor: Eliminate SPIString::value_default

- eliminate value_default
- make value private (-> _value)
- add value() method

20 files changed, 191 insertions, 173 deletions

diff --git a/src/desktop-style.cpp b/src/desktop-style.cpp
index 260b442ef..56d95d210 100644
--- a/src/desktop-style.cpp
+++ b/src/desktop-style.cpp
@@ -1447,11 +1447,7 @@ objects_query_fontfeaturesettings (const std::vector<SPItem*> &objects, SPStyle
     bool different = false;
     int texts = 0;
 
-    if (style_res->font_feature_settings.value) {
-        g_free(style_res->font_feature_settings.value);
-        style_res->font_feature_settings.value = nullptr;
-    }
-    style_res->font_feature_settings.set = FALSE;
+    style_res->font_feature_settings.clear();
 
     for (auto obj : objects) {
         // std::cout << "  " << reinterpret_cast<SPObject*>(i->data)->getId() << std::endl;
@@ -1466,18 +1462,14 @@ objects_query_fontfeaturesettings (const std::vector<SPItem*> &objects, SPStyle
 
         texts ++;
 
-        if (style_res->font_feature_settings.value && style->font_feature_settings.value &&
-            strcmp (style_res->font_feature_settings.value, style->font_feature_settings.value)) {
+        if (style_res->font_feature_settings.set && //
+            strcmp(style_res->font_feature_settings.value(),
+                   style->font_feature_settings.value())) {
             different = true;  // different fonts
         }
 
-        if (style_res->font_feature_settings.value) {
-            g_free(style_res->font_feature_settings.value);
-            style_res->font_feature_settings.value = nullptr;
-        }
-
+        style_res->font_feature_settings = style->font_feature_settings;
         style_res->font_feature_settings.set = true;
-        style_res->font_feature_settings.value = g_strdup(style->font_feature_settings.value);
     }
 
     if (texts == 0 || !style_res->font_feature_settings.set) {
@@ -1599,11 +1591,7 @@ objects_query_fontfamily (const std::vector<SPItem*> &objects, SPStyle *style_re
     bool different = false;
     int texts = 0;
 
-    if (style_res->font_family.value) {
-        g_free(style_res->font_family.value);
-        style_res->font_family.value = nullptr;
-    }
-    style_res->font_family.set = FALSE;
+    style_res->font_family.clear();
 
     for (auto obj : objects) {
         // std::cout << "  " << reinterpret_cast<SPObject*>(i->data)->getId() << std::endl;
@@ -1618,18 +1606,13 @@ objects_query_fontfamily (const std::vector<SPItem*> &objects, SPStyle *style_re
 
         texts ++;
 
-        if (style_res->font_family.value && style->font_family.value &&
-            strcmp (style_res->font_family.value, style->font_family.value)) {
+        if (style_res->font_family.set && //
+            strcmp(style_res->font_family.value(), style->font_family.value())) {
             different = true;  // different fonts
         }
 
-        if (style_res->font_family.value) {
-            g_free(style_res->font_family.value);
-            style_res->font_family.value = nullptr;
-        }
-
+        style_res->font_family = style->font_family;
         style_res->font_family.set = true;
-        style_res->font_family.value = g_strdup(style->font_family.value);
     }
 
     if (texts == 0 || !style_res->font_family.set) {
@@ -1653,11 +1636,7 @@ objects_query_fontspecification (const std::vector<SPItem*> &objects, SPStyle *s
     bool different = false;
     int texts = 0;
 
-    if (style_res->font_specification.value) {
-        g_free(style_res->font_specification.value);
-        style_res->font_specification.value = nullptr;
-    }
-    style_res->font_specification.set = FALSE;
+    style_res->font_specification.clear();
 
     for (auto obj : objects) {
         // std::cout << "  " << reinterpret_cast<SPObject*>(i->data)->getId() << std::endl;
@@ -1672,21 +1651,14 @@ objects_query_fontspecification (const std::vector<SPItem*> &objects, SPStyle *s
 
         texts ++;
 
-        if (style_res->font_specification.value && style_res->font_specification.set &&
-            style->font_specification.value && style->font_specification.set &&
-            strcmp (style_res->font_specification.value, style->font_specification.value)) {
+        if (style_res->font_specification.set &&
+            g_strcmp0(style_res->font_specification.value(), style->font_specification.value())) {
             different = true;  // different fonts
         }
 
         if (style->font_specification.set) {
-
-            if (style_res->font_specification.value) {
-                g_free(style_res->font_specification.value);
-                style_res->font_specification.value = nullptr;
-            }
-
+            style_res->font_specification = style->font_specification;
             style_res->font_specification.set = true;
-            style_res->font_specification.value = g_strdup(style->font_specification.value);
         }
     }
 
diff --git a/src/extension/internal/emf-print.cpp b/src/extension/internal/emf-print.cpp
index ec85b9281..dd48d37ef 100644
--- a/src/extension/internal/emf-print.cpp
+++ b/src/extension/internal/emf-print.cpp
@@ -2027,7 +2027,7 @@ unsigned int PrintEmf::text(Inkscape::Extension::Print * /*mod*/, char const *te
             _lookup_ppt_fontfix("Convert To Wingdings", params);
             break;
         default:  //also CVTNON
-            _lookup_ppt_fontfix(style->font_family.value, params);
+            _lookup_ppt_fontfix(style->font_family.value(), params);
             break;
         }
         if (params.f2 != 0 || params.f3 != 0) {
@@ -2055,7 +2055,7 @@ unsigned int PrintEmf::text(Inkscape::Extension::Print * /*mod*/, char const *te
         // of the special fonts.
         uint16_t *wfacename;
         if (!newfont) {
-            wfacename = U_Utf8ToUtf16le(style->font_family.value, 0, nullptr);
+            wfacename = U_Utf8ToUtf16le(style->font_family.value(), 0, nullptr);
         } else {
             wfacename = U_Utf8ToUtf16le(FontName(newfont), 0, nullptr);
         }
diff --git a/src/extension/internal/wmf-print.cpp b/src/extension/internal/wmf-print.cpp
index 147a6369f..ade732964 100644
--- a/src/extension/internal/wmf-print.cpp
+++ b/src/extension/internal/wmf-print.cpp
@@ -1423,7 +1423,7 @@ unsigned int PrintWmf::text(Inkscape::Extension::Print * /*mod*/, char const *te
             _lookup_ppt_fontfix("Convert To Wingdings", params);
             break;
         default:  //also CVTNON
-            _lookup_ppt_fontfix(style->font_family.value, params);
+            _lookup_ppt_fontfix(style->font_family.value(), params);
             break;
         }
         if (params.f2 != 0 || params.f3 != 0) {
@@ -1452,7 +1452,7 @@ unsigned int PrintWmf::text(Inkscape::Extension::Print * /*mod*/, char const *te
         // of the special fonts.
         char *facename;
         if (!newfont) {
-            facename = U_Utf8ToLatin1(style->font_family.value, 0, nullptr);
+            facename = U_Utf8ToLatin1(style->font_family.value(), 0, nullptr);
         } else {
             facename = U_Utf8ToLatin1(FontName(newfont), 0, nullptr);
         }
diff --git a/src/file-update.cpp b/src/file-update.cpp
index 4fae51cf0..a49ddaa99 100644
--- a/src/file-update.cpp
+++ b/src/file-update.cpp
@@ -141,7 +141,7 @@ void fix_font_name(SPObject *o)
     std::vector<SPObject *> cl = o->childList(false);
     for (std::vector<SPObject *>::const_iterator ci = cl.begin(); ci != cl.end(); ++ci)
         fix_font_name(*ci);
-    std::string prev = o->style->font_family.value ? o->style->font_family.value : o->style->font_family.value_default;
+    std::string prev = o->style->font_family.value();
     if (prev == "Sans")
         o->style->font_family.read("sans-serif");
     else if (prev == "Serif")
diff --git a/src/graphlayout.cpp b/src/graphlayout.cpp
index b04e5f703..003767861 100644
--- a/src/graphlayout.cpp
+++ b/src/graphlayout.cpp
@@ -180,7 +180,7 @@ void graphlayout(std::vector<SPItem*> const & items) {
                 //cout << "Edge: (" << u <<","<<v<<")"<<endl;
                 es.emplace_back(u, v);
                 if (conn->style->marker_end.set) {
-                    if (directed && strcmp(conn->style->marker_end.value, "none")) {
+                    if (directed && strcmp(conn->style->marker_end.value(), "none")) {
                         constraints.push_back(new SeparationConstraint(YDIM, v, u,
                                 ideal_connector_length * directed_edge_height_modifier));
                     }
diff --git a/src/id-clash.cpp b/src/id-clash.cpp
index 7ba021985..8ae82ad1a 100644
--- a/src/id-clash.cpp
+++ b/src/id-clash.cpp
@@ -158,7 +158,7 @@ find_references(SPObject *elem, refmap_type &refmap)
     /* check for url(#...) references in markers */
     const gchar *markers[4] = { "", "marker-start", "marker-mid", "marker-end" };
     for (unsigned i = SP_MARKER_LOC_START; i < SP_MARKER_LOC_QTY; i++) {
-        const gchar *value = style->marker_ptrs[i]->value;
+        const gchar *value = style->marker_ptrs[i]->value();
         if (value) {
             auto uri = extract_uri(value);
             if (uri[0] == '#') {
diff --git a/src/libnrtype/FontFactory.cpp b/src/libnrtype/FontFactory.cpp
index 0260a9c4b..7844925b7 100644
--- a/src/libnrtype/FontFactory.cpp
+++ b/src/libnrtype/FontFactory.cpp
@@ -79,7 +79,7 @@ PangoFontDescription* ink_font_description_from_style(SPStyle const *style)
 {
     PangoFontDescription *descr = pango_font_description_new();
 
-    pango_font_description_set_family(descr, style->font_family.value);
+    pango_font_description_set_family(descr, style->font_family.value());
 
     // This duplicates Layout::EnumConversionItem... perhaps we can share code?
     switch ( style->font_style.computed ) {
@@ -572,11 +572,12 @@ font_instance* font_factory::FaceFromStyle(SPStyle const *style)
     if (style) {
 
         //  First try to use the font specification if it is set
+        char const *val;
         if (style->font_specification.set
-            && style->font_specification.value
-            && *style->font_specification.value) {
+            && (val = style->font_specification.value())
+            && val[0]) {
 
-            font = FaceFromFontSpecification(style->font_specification.value);
+            font = FaceFromFontSpecification(val);
         }
 
         // If that failed, try using the CSS information in the style
diff --git a/src/libnrtype/font-lister.cpp b/src/libnrtype/font-lister.cpp
index 30283e624..dcd94d972 100644
--- a/src/libnrtype/font-lister.cpp
+++ b/src/libnrtype/font-lister.cpp
@@ -542,7 +542,7 @@ std::pair<Glib::ustring, Glib::ustring> FontLister::selection_update()
 
     //std::cout << "  Attempting selected style" << std::endl;
     if (result != QUERY_STYLE_NOTHING && query.font_specification.set) {
-        fontspec = query.font_specification.value;
+        fontspec = query.font_specification.value();
         //std::cout << "   fontspec from query   :" << fontspec << ":" << std::endl;
     }
 
diff --git a/src/object/sp-path.cpp b/src/object/sp-path.cpp
index 9d7ef1fe0..58a823543 100644
--- a/src/object/sp-path.cpp
+++ b/src/object/sp-path.cpp
@@ -133,14 +133,14 @@ void SPPath::build(SPDocument *document, Inkscape::XML::Node *repr) {
 
         (d_source == SP_STYLE_SRC_STYLE_PROP || d_source == SP_STYLE_SRC_STYLE_SHEET) ) {
 
-        if (style->d.value) {
+        if (char const *d_val = style->d.value()) {
             // Chrome shipped with a different syntax for property vs attribute.
             // The SVG Working group decided to follow the Chrome syntax (which may
             // allow future extensions of the 'd' property). The property syntax
             // wraps the path data with "path(...)". We must strip that!
 
             // Must be Glib::ustring or we get conversion errors!
-            Glib::ustring input = style->d.value;
+            Glib::ustring input = d_val;
             Glib::ustring expression = R"A(path\("(.*)"\))A";
             Glib::RefPtr<Glib::Regex> regex = Glib::Regex::create(expression);
             Glib::MatchInfo matchInfo;
diff --git a/src/object/sp-shape.cpp b/src/object/sp-shape.cpp
index be68aae1f..c342fba85 100644
--- a/src/object/sp-shape.cpp
+++ b/src/object/sp-shape.cpp
@@ -67,7 +67,7 @@ void SPShape::build(SPDocument *document, Inkscape::XML::Node *repr) {
     SPLPEItem::build(document, repr);
 
     for (int i = 0 ; i < SP_MARKER_LOC_QTY ; i++) {
-        sp_shape_set_marker (this, i, this->style->marker_ptrs[i]->value);
+        sp_shape_set_marker (this, i, this->style->marker_ptrs[i]->value());
     }
 }
 
@@ -133,7 +133,7 @@ void SPShape::update(SPCtx* ctx, guint flags) {
      * match the style.
      */
     for (int i = 0 ; i < SP_MARKER_LOC_QTY ; i++) {
-        sp_shape_set_marker (this, i, this->style->marker_ptrs[i]->value);
+        sp_shape_set_marker (this, i, this->style->marker_ptrs[i]->value());
     }
 
     if (flags & (SP_OBJECT_STYLE_MODIFIED_FLAG | SP_OBJECT_VIEWPORT_MODIFIED_FLAG)) {
@@ -856,7 +856,7 @@ Inkscape::DrawingItem* SPShape::show(Inkscape::Drawing &drawing, unsigned int /*
      * match the style.
      */
     for (int i = 0 ; i < SP_MARKER_LOC_QTY ; i++) {
-        sp_shape_set_marker (this, i, this->style->marker_ptrs[i]->value);
+        sp_shape_set_marker (this, i, this->style->marker_ptrs[i]->value());
     }
 
     if (has_markers) {
diff --git a/src/object/sp-text.cpp b/src/object/sp-text.cpp
index 3cfae54cb..714ebe1d2 100644
--- a/src/object/sp-text.cpp
+++ b/src/object/sp-text.cpp
@@ -352,7 +352,7 @@ gchar* SPText::description() const {
 
     SPStyle *style = this->style;
 
-    char *n = xml_quote_strdup( style->font_family.value );
+    char *n = xml_quote_strdup(style->font_family.value());
 
     Inkscape::Preferences *prefs = Inkscape::Preferences::get();
     int unit = prefs->getInt("/options/font/unitType", SP_CSS_UNIT_PT);
@@ -1024,7 +1024,7 @@ Inkscape::XML::Node* SPText::get_first_rectangle()
 
     Inkscape::XML::Node *our_ref = getRepr();
 
-    if (style->shape_inside.set && style->shape_inside.value) {
+    if (style->shape_inside.set) {
 
         std::vector<Glib::ustring> shapes = get_shapes();
 
@@ -1047,11 +1047,11 @@ Inkscape::XML::Node* SPText::get_first_rectangle()
 std::vector<Glib::ustring> SPText::get_shapes() const
 {
     std::vector<Glib::ustring> shapes;
-    if (style->shape_inside.set && style->shape_inside.value) {
-
+    char const *val;
+    if (style->shape_inside.set && (val = style->shape_inside.value())) {
         static Glib::RefPtr<Glib::Regex> regex = Glib::Regex::create("url\\(#([A-z0-9#]*)\\)");
         Glib::MatchInfo matchInfo;
-        regex->match(style->shape_inside.value, matchInfo);
+        regex->match(val, matchInfo);
 
         while (matchInfo.matches()) {
             shapes.push_back(matchInfo.fetch(1));
diff --git a/src/selection-chemistry.cpp b/src/selection-chemistry.cpp
index 26b406dbe..b8e14238d 100755
--- a/src/selection-chemistry.cpp
+++ b/src/selection-chemistry.cpp
@@ -2175,9 +2175,8 @@ std::vector<SPItem*> sp_get_same_style(SPItem *sel, std::vector<SPItem*> &src, S
                 match = true;
                 int len = sizeof(sel_style->marker)/sizeof(SPIString);
                 for (int i = 0; i < len; i++) {
-                    match = (sel_style->marker_ptrs[i]->set == iter_style->marker_ptrs[i]->set);
-                    if (sel_style->marker_ptrs[i]->set && iter_style->marker_ptrs[i]->set &&
-                        (strcmp(sel_style->marker_ptrs[i]->value, iter_style->marker_ptrs[i]->value))) {
+                    if (g_strcmp0(sel_style->marker_ptrs[i]->value(),
+                                  iter_style->marker_ptrs[i]->value())) {
                         match = false;
                         break;
                     }
diff --git a/src/style-internal.cpp b/src/style-internal.cpp
index bce687d7f..2bd291fa8 100644
--- a/src/style-internal.cpp
+++ b/src/style-internal.cpp
@@ -94,6 +94,60 @@ const Glib::ustring SPIBase::write(guint const flags, SPStyleSrc const &style_sr
 }
 
 
+/**
+ * If str.endswith("!important") then assign stripped = str[:-10].rstrip() and return true.
+ * Otherwise, leave stripped unmodified and return false.
+ */
+static bool strip_important(gchar const *str, std::string &stripped)
+{
+    assert(str != nullptr);
+
+    constexpr size_t N = 10; // strlen("!important")
+    auto pos = strlen(str);
+
+    if (pos >= N && strncmp(str + pos - N, "!important", N) == 0) {
+        pos -= N;
+
+        // strip whitespace from the right
+        while (pos > 0 && g_ascii_isspace(str[pos - 1])) {
+            --pos;
+        }
+
+        stripped.assign(str, pos);
+        return true;
+    }
+
+    return false;
+}
+
+void SPIBase::readIfUnset(gchar const *str, SPStyleSrc source)
+{
+    if (!str)
+        return;
+
+    bool has_important = false;
+    std::string stripped;
+
+    // '!important' is invalid on attributes
+    if (source != SP_STYLE_SRC_ATTRIBUTE) {
+        has_important = strip_important(str, stripped);
+        if (has_important) {
+            str = stripped.c_str();
+        }
+    }
+
+    if (!set || (has_important && !important)) {
+        style_src = source;
+        read(str);
+        if (set) {
+            if (has_important) {
+                important = true;
+            }
+        }
+    }
+}
+
+
 // SPIFloat -------------------------------------------------------------
 
 void
@@ -1086,63 +1140,95 @@ const Glib::ustring SPIEastAsian::get_value() const
 
 void
 SPIString::read( gchar const *str ) {
+    g_assert(!set);
+    g_assert(!inherit);
+    g_assert(!_value);
+
     if( !str ) return;
 
+    if (style_src == SP_STYLE_SRC_ATTRIBUTE && id() == SP_ATTR_D) {
+        return;
+    }
+
     if (!strcmp(str, "inherit")) {
         set = true;
         inherit = true;
-        value = nullptr;
-    } else {
+    } else if (!g_strcmp0(str, get_default_value())) {
+        // no need to copy string
         set = true;
-        inherit = false;
+    } else {
+        Glib::ustring str_temp;
 
-        Glib::ustring str_temp(str);
-        if (id() == SP_ATTR_D && style_src == SP_STYLE_SRC_ATTRIBUTE) {
-            set = false;
-            return;
-        }
         if (id() == SP_PROP_FONT_FAMILY) {
             // Family names may be quoted in CSS, internally we use unquoted names.
+            str_temp = str;
             css_font_family_unquote( str_temp );
+            str = str_temp.c_str();
         } else if (id() == SP_PROP_INKSCAPE_FONT_SPEC) {
+            str_temp = str;
             css_unquote( str_temp );
+            str = str_temp.c_str();
         }
 
-        value = g_strdup(str_temp.c_str());
+        set = true;
+        _value = g_strdup(str);
     }
 }
 
 
+/**
+ * Value as it should be written to CSS representation, including quotes if needed.
+ */
 const Glib::ustring SPIString::get_value() const
 {
-    if (this->inherit) return Glib::ustring("inherit");
-    if (!this->value) return Glib::ustring("");
-    if (id() == SP_PROP_FONT_FAMILY) {
-        Glib::ustring font_family( this->value );
-        css_font_family_quote( font_family );
-        return font_family;
-    } else if (id() == SP_PROP_INKSCAPE_FONT_SPEC) {
-        Glib::ustring font_spec( this->value );
-        css_quote( font_spec );
-        return font_spec;
+    Glib::ustring val;
+
+    if (set && inherit) {
+        val = "inherit";
+    } else if (auto *v = value()) {
+        val = v;
+
+        if (id() == SP_PROP_FONT_FAMILY) {
+            css_font_family_quote(val);
+        } else if (id() == SP_PROP_INKSCAPE_FONT_SPEC) {
+            css_quote(val);
+        }
+    }
+
+    return val;
+}
+
+char const *SPIString::value() const
+{
+    return _value ? _value : get_default_value();
+}
+
+char const *SPIString::get_default_value() const
+{
+    switch (id()) {
+        case SP_PROP_FONT_FAMILY:
+            return "sans-serif";
+        case SP_PROP_FONT_FEATURE_SETTINGS:
+            return "normal";
+        default:
+            return nullptr;
     }
-    return Glib::ustring(this->value);
 }
 
+
 void
 SPIString::clear() {
     SPIBase::clear();
-    g_free( value );
-    value = nullptr;
-    if( value_default ) value = g_strdup( value_default );
+    g_free(_value);
+    _value = nullptr;
 }
 
 void
 SPIString::cascade( const SPIBase* const parent ) {
     if( const SPIString* p = dynamic_cast<const SPIString*>(parent) ) {
         if( inherits && (!set || inherit) ) {
-            g_free(value);
-            value = g_strdup(p->value);
+            g_free(_value);
+            _value = g_strdup(p->_value);
         }
     } else {
         std::cerr << "SPIString::cascade(): Incorrect parent type" << std::endl;
@@ -1156,8 +1242,8 @@ SPIString::merge( const SPIBase* const parent ) {
             if( (!set || inherit) && p->set && !(p->inherit) ) {
                 set     = p->set;
                 inherit = p->inherit;
-                g_free(value);
-                value = g_strdup(p->value);
+                g_free(_value);
+                _value = g_strdup(p->_value);
             }
         }
     }
@@ -1166,10 +1252,7 @@ SPIString::merge( const SPIBase* const parent ) {
 bool
 SPIString::operator==(const SPIBase& rhs) {
     if( const SPIString* r = dynamic_cast<const SPIString*>(&rhs) ) {
-        if( value == nullptr && r->value == nullptr ) return (SPIBase::operator==(rhs));
-        if( value == nullptr || r->value == nullptr ) return false;
-
-        return (strcmp(value, r->value) == 0 && SPIBase::operator==(rhs));
+        return g_strcmp0(_value, r->_value) == 0 && SPIBase::operator==(rhs);
     } else {
         return false;
     }
@@ -1191,9 +1274,7 @@ SPIShapes::read( gchar const *str) {
 
     if( !str ) return;
 
-    set = true;
-    inherit = false;
-    value = g_strdup(str);
+    SPIString::read(str);
 
     // The object/repr this property is connected to..
     SPObject* object = style->object;
diff --git a/src/style-internal.h b/src/style-internal.h
index fb24b9631..f395e23d3 100644
--- a/src/style-internal.h
+++ b/src/style-internal.h
@@ -139,43 +139,12 @@ public:
     = default;
 
     virtual void read( gchar const *str ) = 0;
-    virtual void readIfUnset( gchar const *str, SPStyleSrc const &source = SP_STYLE_SRC_STYLE_PROP ) {
-        if (!str) return;
-
-        bool has_important = false;
-        Glib::ustring stripped = strip_important(str, has_important); // Sets 'has_important'
-        // '!important' is invalid on attributes, don't read.
-        if (source == SP_STYLE_SRC_ATTRIBUTE && has_important){
-            return;
-        }
-
-        if ( !set || (has_important && !important) ) {
-            style_src = source;
-            read( stripped.c_str() );
-            if ( set ) {
-                if (has_important) {
-                    important = true;
-                }
-            }
-        }
-    }
+    virtual void readIfUnset(gchar const *str, SPStyleSrc source = SP_STYLE_SRC_STYLE_PROP);
 
     Glib::ustring important_str() const {
         return Glib::ustring(important ? " !important" : "");
     }
 
-    Glib::ustring strip_important( gchar const *str, bool &important ) {
-        assert (str != NULL);
-        Glib::ustring string = Glib::ustring(str);
-        auto pos = string.rfind( " !important" );
-        important = false;
-        if (pos != std::string::npos) {
-            important = true;
-            string.erase(pos);
-        }
-        return string;
-    }
-
     virtual void readAttribute( Inkscape::XML::Node *repr ) {
         readIfUnset(repr->attribute(name().c_str()), SP_STYLE_SRC_ATTRIBUTE);
     }
@@ -641,15 +610,12 @@ class SPIString : public SPIBase
 {
 
 public:
-    // TODO probably want to avoid gchar* and c-style strings.
-    SPIString(gchar const *value_default_in = nullptr, bool inherits = true)
+    SPIString(bool inherits = true)
         : SPIBase(inherits)
-        , value_default(g_strdup(value_default_in))
     {}
 
     ~SPIString() override {
-        g_free(value);
-        g_free(value_default);
+        g_free(_value);
     }
 
     void read( gchar const *str ) override;
@@ -660,10 +626,8 @@ public:
 
     SPIString& operator=(const SPIString& rhs) {
         SPIBase::operator=(rhs);
-        g_free(value);
-        g_free(value_default);
-        value            = rhs.value ? g_strdup(rhs.value) : nullptr;
-        value_default    = rhs.value_default ? g_strdup(rhs.value_default) : nullptr;
+        g_free(_value);
+        _value = g_strdup(rhs._value);
         return *this;
     }
 
@@ -672,10 +636,13 @@ public:
         return !(*this == rhs);
     }
 
-  // To do: make private, convert value to Glib::ustring
-public:
-    gchar *value = nullptr;
-    gchar *value_default = nullptr;
+    //! Get value if set, or inherited value, or default value (may be NULL)
+    char const *value() const;
+
+  private:
+    char const *get_default_value() const;
+
+    gchar *_value = nullptr;
 };
 
 /// Shapes type internal to SPStyle.
@@ -685,10 +652,7 @@ class SPIShapes : public SPIString
 {
 
 public:
-    // TODO probably want to avoid gchar* and c-style strings.
-    SPIShapes(gchar const *value_default_in = nullptr)
-        : SPIString(value_default_in)
-    {}
+    SPIShapes() = default;
 
     void read( gchar const *str ) override;
 
@@ -1258,9 +1222,10 @@ class SPIVectorEffect : public SPIBase
 {
 
 public:
-    SPIVectorEffect() {
+    SPIVectorEffect()
+        : SPIBase(false)
+    {
         this->clear();
-        inherits = false;
     }
 
     ~SPIVectorEffect() override
diff --git a/src/style.cpp b/src/style.cpp
index bef6718da..1e0fdaf78 100644
--- a/src/style.cpp
+++ b/src/style.cpp
@@ -271,7 +271,7 @@ SPStyle::SPStyle(SPDocument *document_in, SPObject *object_in) :
 
 
     // SVG 2 attributes promoted to properties. (When geometry properties are added, move after font.)
-    d(                      nullptr,    false),  // SPIString Not inherited!
+    d(                      false),  // SPIString Not inherited!
 
     // Font related properties and 'font' shorthand
     font_style(             SP_CSS_FONT_STYLE_NORMAL),
@@ -280,7 +280,7 @@ SPStyle::SPStyle(SPDocument *document_in, SPObject *object_in) :
     font_stretch(           SP_CSS_FONT_STRETCH_NORMAL),
     font_size(),
     line_height(            1.25 ),         // SPILengthOrNormal
-    font_family(            "sans-serif"),  // SPIString w/default
+    font_family(            ),  // SPIString w/default
     font(),                                                      // SPIFont
     font_specification(     ),              // SPIString
 
@@ -291,7 +291,7 @@ SPStyle::SPStyle(SPDocument *document_in, SPObject *object_in) :
     font_variant_numeric(   ),
     font_variant_alternates(SP_CSS_FONT_VARIANT_ALTERNATES_NORMAL),
     font_variant_east_asian(),
-    font_feature_settings(  "normal"),
+    font_feature_settings(  ),
 
     // Variable Fonts
     font_variation_settings(),  // SPIFontVariationSettings
@@ -594,7 +594,6 @@ SPStyle::read( SPObject *object, Inkscape::XML::Node *repr ) {
         // Shorthands are not allowed as presentation properites. Note: text-decoration and
         // font-variant are converted to shorthands in CSS 3 but can still be read as a
         // non-shorthand for compatibility with older renders, so they should not be in this list.
-        // We could add a flag to SPIBase to avoid string comparison.
         if (p->id() != SP_PROP_FONT && p->id() != SP_PROP_MARKER) {
             p->readAttribute( repr );
         }
@@ -1041,9 +1040,10 @@ SPStyle::getFontFeatureString() {
     if( font_variant_east_asian.value & SP_CSS_FONT_VARIANT_EAST_ASIAN_RUBY )
         feature_string += "ruby, ";
 
-    if ( font_feature_settings.value && strcmp( font_feature_settings.value, "normal") ) {
+    char const *val = font_feature_settings.value();
+    if (val[0] && strcmp(val, "normal")) {
         // We do no sanity checking...
-        feature_string += font_feature_settings.value; 
+        feature_string += val;
         feature_string += ", ";
     }
 
diff --git a/src/ui/dialog/font-substitution.cpp b/src/ui/dialog/font-substitution.cpp
index bf371588a..5616b4bfa 100644
--- a/src/ui/dialog/font-substitution.cpp
+++ b/src/ui/dialog/font-substitution.cpp
@@ -175,13 +175,11 @@ std::vector<SPItem*> FontSubstitution::getFontReplacedItems(SPDocument* doc, Gli
         if (style) {
             gchar const *style_font = nullptr;
             if (style->font_family.set)
-                style_font = style->font_family.value;
+                style_font = style->font_family.value();
             else if (style->font_specification.set)
-                style_font = style->font_specification.value;
-            else if (style->font_family.value)
-                style_font = style->font_family.value;
-            else if (style->font_specification.value)
-                style_font = style->font_specification.value;
+                style_font = style->font_specification.value();
+            else
+                style_font = style->font_family.value();
 
             if (style_font) {
                 if (has_visible_text(item)) {
diff --git a/src/ui/toolbar/text-toolbar.cpp b/src/ui/toolbar/text-toolbar.cpp
index 9743ce524..0dd94cadf 100644
--- a/src/ui/toolbar/text-toolbar.cpp
+++ b/src/ui/toolbar/text-toolbar.cpp
@@ -181,11 +181,11 @@ static void sp_text_toolbox_select_cb( GtkEntry* entry, GtkEntryIconPosition /*p
 
       Glib::ustring family_style;
       if (style->font_family.set) {
-	family_style = style->font_family.value;
+	family_style = style->font_family.value();
 	//std::cout << " family style from font_family: " << family_style << std::endl;
       }
       else if (style->font_specification.set) {
-	family_style = style->font_specification.value;
+	family_style = style->font_specification.value();
 	//std::cout << " family style from font_spec: " << family_style << std::endl;
       }
 
diff --git a/src/ui/tools/text-tool.cpp b/src/ui/tools/text-tool.cpp
index 203ce027e..6eb02b7b5 100644
--- a/src/ui/tools/text-tool.cpp
+++ b/src/ui/tools/text-tool.cpp
@@ -657,7 +657,7 @@ bool TextTool::root_handler(GdkEvent* event) {
                             SPItem *text = create_text_with_rectangle (desktop, this->p0, p1);
 
                             /* Get "shape-inside" */
-                            gchar* shape_inside = g_strdup(text->style->shape_inside.value);
+                            auto shape_inside = text->style->shape_inside;
 
                             /* Set style */
                             sp_desktop_apply_style_tool(desktop, text->getRepr(), "/tools/text", true);
@@ -670,8 +670,7 @@ bool TextTool::root_handler(GdkEvent* event) {
                             text->setCSS(css,"style");
                             sp_repr_css_attr_unref(css);
                             /* Restore "shape-inside" */
-                            text->style->shape_inside.read( shape_inside );
-                            g_free( shape_inside );
+                            text->style->shape_inside = shape_inside;
                             text->updateRepr();
 
                             desktop->getSelection()->set(text);
diff --git a/src/ui/widget/font-variants.cpp b/src/ui/widget/font-variants.cpp
index 28ba7dc14..10874318b 100644
--- a/src/ui/widget/font-variants.cpp
+++ b/src/ui/widget/font-variants.cpp
@@ -716,10 +716,11 @@ namespace Widget {
       std::string setting;
 
       // Set feature radiobutton (if it exists) or add to _feature_entry string.
-      if (query->font_feature_settings.value) {
+      char const *val = query->font_feature_settings.value();
+      if (val) {
 
           std::vector<Glib::ustring> tokens =
-              Glib::Regex::split_simple("\\s*,\\s*", query->font_feature_settings.value);
+              Glib::Regex::split_simple("\\s*,\\s*", val);
 
           for (auto token: tokens) {
               regex->match(token, matchInfo);
diff --git a/src/widgets/stroke-style.cpp b/src/widgets/stroke-style.cpp
index 44f9ac714..815111dd9 100644
--- a/src/widgets/stroke-style.cpp
+++ b/src/widgets/stroke-style.cpp
@@ -591,7 +591,7 @@ StrokeStyle::forkMarker(SPObject *marker, int loc, SPItem *item)
     unsigned int refs = 0;
     for (int i = SP_MARKER_LOC_START; i < SP_MARKER_LOC_QTY; i++) {
         if (item->style->marker_ptrs[i]->set &&
-            !strcmp(urlId.c_str(), item->style->marker_ptrs[i]->value)) {
+            !strcmp(urlId.c_str(), item->style->marker_ptrs[i]->value())) {
             refs++;
         }
     }
@@ -1277,12 +1277,14 @@ StrokeStyle::updateAllMarkers(std::vector<SPItem*> const &objects, bool skip_und
 
         if (!all_texts) {
             for (SPObject *object : simplified_list) {
+                char const *value = object->style->marker_ptrs[markertype.loc]->value();
+
                 // If the object has this type of markers,
-                if (object->style->marker_ptrs[markertype.loc]->value == nullptr)
+                if (value == nullptr)
                     continue;
 
                 // Extract the name of the marker that the object uses
-                marker = getMarkerObj(object->style->marker_ptrs[markertype.loc]->value, object->document);
+                marker = getMarkerObj(value, object->document);
 
                 // Set the marker color
                 if (update < 0) {