1 files changed, 52 insertions, 22 deletions

diff --git a/src/display/drawing-item.cpp b/src/display/drawing-item.cpp
index 1af07cb44..71a7f8906 100644
--- a/src/display/drawing-item.cpp
+++ b/src/display/drawing-item.cpp
@@ -141,7 +141,14 @@ DrawingItem::appendChild(DrawingItem *item)
     assert(item->_child_type == CHILD_ORPHAN);
     item->_child_type = CHILD_NORMAL;
     _children.push_back(*item);
-    _markForUpdate(STATE_ALL, false);
+
+    // This ensures that _markForUpdate() called on the child will recurse to this item
+    item->_state = STATE_ALL;
+    // Because _markForUpdate recurses through ancestors, we can simply call it
+    // on the just-added child. This has the additional benefit that we do not
+    // rely on the appended child being in the default non-updated state.
+    // We set propagate to true, because the child might have descendants of its own.
+    item->_markForUpdate(STATE_ALL, true);
 }
 
 void
@@ -151,7 +158,9 @@ DrawingItem::prependChild(DrawingItem *item)
     assert(item->_child_type == CHILD_ORPHAN);
     item->_child_type = CHILD_NORMAL;
     _children.push_front(*item);
-    _markForUpdate(STATE_ALL, false);
+    // See appendChild for explanation
+    item->_state = STATE_ALL;
+    item->_markForUpdate(STATE_ALL, true);
 }
 
 /// Delete all regular children of this item (not mask or clip).
@@ -281,17 +290,10 @@ DrawingItem::setZOrder(unsigned z)
     _markForRendering();
 }
 
-void DrawingItem::setItemBounds(Geom::OptRect const &bounds)
-{
-    if (!bounds) return;
-    Geom::IntRect copy = bounds->roundOutwards();
-    if (_filter) _filter->area_enlarge(copy, this);
-    this->setFilterBounds(copy);
-}
-
-void DrawingItem::setFilterBounds(Geom::OptRect const &bounds)
+void
+DrawingItem::setItemBounds(Geom::OptRect const &bounds)
 {
-    if (bounds) _filter_bbox = bounds;
+    _item_bbox = bounds;
 }
 
 /**
@@ -359,10 +361,14 @@ DrawingItem::update(Geom::IntRect const &area, UpdateContext const &ctx, unsigne
 
     if (to_update & STATE_BBOX) {
         // compute drawbox
-        if (_filter && render_filters && _filter_bbox) {
-            Geom::OptRect enlarged = _filter_bbox;
-            *enlarged *= ctm();
-            _drawbox = enlarged->roundOutwards();
+        if (_filter && render_filters) {
+            Geom::OptRect enlarged = _filter->filter_effect_area(_item_bbox);
+            if (enlarged) {
+                *enlarged *= ctm();
+                _drawbox = enlarged->roundOutwards();
+            } else {
+                _drawbox = Geom::OptIntRect();
+            }
         } else {
             _drawbox = _bbox;
         }
@@ -714,8 +720,11 @@ DrawingItem::pick(Geom::Point const &p, double delta, unsigned flags)
 {
     // Sometimes there's no BBOX in state, reason unknown (bug 992817)
     // I made this not an assert to remove the warning
-    if (!(_state & STATE_BBOX) || !(_state & STATE_PICK))
+    if (!(_state & STATE_BBOX) || !(_state & STATE_PICK)) {
+        g_warning("Invalid state when picking: STATE_BBOX = %d, STATE_PICK = %d",
+                  _state & STATE_BBOX, _state & STATE_PICK);
         return NULL;
+    }
     // ignore invisible and insensitive items unless sticky
     if (!(flags & PICK_STICKY) && !(_visible && _sensitive))
         return NULL;
@@ -736,7 +745,10 @@ DrawingItem::pick(Geom::Point const &p, double delta, unsigned flags)
     }
 
     Geom::OptIntRect box = (outline || (flags & PICK_AS_CLIP)) ? _bbox : _drawbox;
-    if (!box) return NULL;
+    if (!box) {
+        g_warning("bbox unset when picking");
+        return NULL;
+    }
 
     Geom::Rect expanded = *box;
     expanded.expandBy(delta);
@@ -756,6 +768,9 @@ DrawingItem::pick(Geom::Point const &p, double delta, unsigned flags)
 void
 DrawingItem::_markForRendering()
 {
+    // TODO: this function does too much work when a large subtree
+    // is invalidated - fix
+    
     bool outline = _drawing.outline();
     Geom::OptIntRect dirty = outline ? _bbox : _drawbox;
     if (!dirty) return;
@@ -807,8 +822,8 @@ DrawingItem::_invalidateFilterBackground(Geom::IntRect const &area)
  * all children should also have the corresponding flags unset before checking
  * whether they need to be traversed. This way there is one less traversal
  * of the tree. Without this we would need to unset state bits in all children.
- * With _propagate we do this during the update call, when we have to traverse
- * the tree anyway.
+ * With _propagate we do this during the update call, when we have to recurse
+ * into children anyway.
  */
 void
 DrawingItem::_markForUpdate(unsigned flags, bool propagate)
@@ -818,15 +833,31 @@ DrawingItem::_markForUpdate(unsigned flags, bool propagate)
     }
 
     if (_state & flags) {
+        unsigned oldstate = _state;
         _state &= ~flags;
-        if (_parent) {
+        if (oldstate != _state && _parent) {
+            // If we actually reset anything in state, recurse on the parent.
             _parent->_markForUpdate(flags, false);
         } else {
+            // If nothing changed, it means our ancestors are already invalidated
+            // up to the root. Do not bother recursing, because it won't change anything.
+            // Also do this if we are the root item, because we have no more ancestors
+            // to invalidate.
             _drawing.signal_request_update.emit(this);
         }
     }
 }
 
+/**
+ * Process information related to the new style.
+ *
+ * This function is something of a hack to avoid creating an extra class in the hierarchy
+ * which would differ from DrawingItem only by having a _style member.
+ * This is mainly to the benefit of DrawingGlyphs, which use the style of their parent.
+ * This should probably be refactored some day, possibly by creating the relevant class
+ * or creating a more complex data model in DrawingText and removing DrawingGlyphs,
+ * which would cause every item to have a style.
+ */
 void
 DrawingItem::_setStyleCommon(SPStyle *&_style, SPStyle *style)
 {
@@ -834,7 +865,6 @@ DrawingItem::_setStyleCommon(SPStyle *&_style, SPStyle *style)
     if (_style) sp_style_unref(_style);
     _style = style;
 
-    // if group has a filter
     if (style->filter.set && style->getFilter()) {
         if (!_filter) {
             int primitives = sp_filter_primitive_count(SP_FILTER(style->getFilter()));