More generic handling of child type in DrawingItem.

Fix clip object selection bug (LP #365458).

Fixed bugs:
  - https://launchpad.net/bugs/365458

(bzr r10347.1.31)

13 files changed, 106 insertions, 64 deletions

diff --git a/src/display/drawing-group.cpp b/src/display/drawing-group.cpp
index 38ab73ca2..002a5a2d4 100644
--- a/src/display/drawing-group.cpp
+++ b/src/display/drawing-group.cpp
@@ -112,10 +112,10 @@ DrawingGroup::_clipItem(DrawingContext &ct, Geom::IntRect const &area)
 }
 
 DrawingItem *
-DrawingGroup::_pickItem(Geom::Point const &p, double delta, bool sticky)
+DrawingGroup::_pickItem(Geom::Point const &p, double delta, unsigned flags)
 {
     for (ChildrenList::iterator i = _children.begin(); i != _children.end(); ++i) {
-        DrawingItem *picked = i->pick(p, delta, sticky);
+        DrawingItem *picked = i->pick(p, delta, flags);
         if (picked) {
             return _pick_children ? picked : this;
         }
diff --git a/src/display/drawing-group.h b/src/display/drawing-group.h
index 7b0645bf4..377c0be39 100644
--- a/src/display/drawing-group.h
+++ b/src/display/drawing-group.h
@@ -36,7 +36,7 @@ protected:
                                  unsigned flags, unsigned reset);
     virtual void _renderItem(DrawingContext &ct, Geom::IntRect const &area, unsigned flags);
     virtual void _clipItem(DrawingContext &ct, Geom::IntRect const &area);
-    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, bool sticky);
+    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, unsigned flags);
     virtual bool _canClip();
 
     SPStyle *_style;
diff --git a/src/display/drawing-image.cpp b/src/display/drawing-image.cpp
index 64601354d..074393ab5 100644
--- a/src/display/drawing-image.cpp
+++ b/src/display/drawing-image.cpp
@@ -196,7 +196,7 @@ distance_to_segment (Geom::Point const &p, Geom::Point const &a1, Geom::Point co
 }
 
 DrawingItem *
-DrawingImage::_pickItem(Geom::Point const &p, double delta, bool /*sticky*/)
+DrawingImage::_pickItem(Geom::Point const &p, double delta, unsigned /*sticky*/)
 {
     if (!_pixbuf) return NULL;
 
diff --git a/src/display/drawing-image.h b/src/display/drawing-image.h
index de8591221..9f758398b 100644
--- a/src/display/drawing-image.h
+++ b/src/display/drawing-image.h
@@ -38,7 +38,7 @@ protected:
     virtual unsigned _updateItem(Geom::IntRect const &area, UpdateContext const &ctx,
                                  unsigned flags, unsigned reset);
     virtual void _renderItem(DrawingContext &ct, Geom::IntRect const &area, unsigned flags);
-    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, bool sticky);
+    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, unsigned flags);
 
     GdkPixbuf *_pixbuf;
     cairo_surface_t *_surface;
diff --git a/src/display/drawing-item.cpp b/src/display/drawing-item.cpp
index ae3dd49ab..ac0d1be1e 100644
--- a/src/display/drawing-item.cpp
+++ b/src/display/drawing-item.cpp
@@ -55,6 +55,7 @@ DrawingItem::DrawingItem(Drawing &drawing)
     , _user_data(NULL)
     , _cache(NULL)
     , _state(0)
+    , _child_type(CHILD_ORPHAN)
     , _visible(true)
     , _sensitive(true)
     , _cached(0)
@@ -62,9 +63,6 @@ DrawingItem::DrawingItem(Drawing &drawing)
     , _has_cache_iterator(0)
     , _propagate(0)
 //    , _renders_opacity(0)
-    , _clip_child(0)
-    , _mask_child(0)
-    , _drawing_root(0)
     , _pick_children(0)
 {
 }
@@ -85,19 +83,29 @@ DrawingItem::~DrawingItem()
     // due to the effect of clearChildren(), this only happens for the top-level deleted item
     if (_parent) {
         _markForRendering();
+    }
+
+    switch (_child_type) {
+    case CHILD_NORMAL: {
+        ChildrenList::iterator ithis = _parent->_children.iterator_to(*this);
+        _parent->_children.erase(ithis);
+        } break;
+    case CHILD_CLIP:
         // we cannot call setClip(NULL) or setMask(NULL),
         // because that would be an endless loop
-        if (_clip_child) {
-            _parent->_clip = NULL;
-        } else if (_mask_child) {
-            _parent->_mask = NULL;
-        } else {
-            ChildrenList::iterator ithis = _parent->_children.iterator_to(*this);
-            _parent->_children.erase(ithis);
-        }
-        _parent->_markForUpdate(STATE_ALL, false);
-    } else if (_drawing_root) {
+        _parent->_clip = NULL;
+        break;
+    case CHILD_MASK:
+        _parent->_mask = NULL;
+        break;
+    case CHILD_ROOT:
         _drawing._root = NULL;
+        break;
+    default: ;
+    }
+
+    if (_parent) {
+        _parent->_markForUpdate(STATE_ALL, false);
     }
     clearChildren();
     delete _transform;
@@ -118,6 +126,8 @@ void
 DrawingItem::appendChild(DrawingItem *item)
 {
     item->_parent = this;
+    assert(item->_child_type == CHILD_ORPHAN);
+    item->_child_type = CHILD_NORMAL;
     _children.push_back(*item);
     _markForUpdate(STATE_ALL, false);
 }
@@ -126,6 +136,8 @@ void
 DrawingItem::prependChild(DrawingItem *item)
 {
     item->_parent = this;
+    assert(item->_child_type == CHILD_ORPHAN);
+    item->_child_type = CHILD_NORMAL;
     _children.push_front(*item);
     _markForUpdate(STATE_ALL, false);
 }
@@ -139,6 +151,7 @@ DrawingItem::clearChildren()
     // from which they have already been removed by clear_and_dispose
     for (ChildrenList::iterator i = _children.begin(); i != _children.end(); ++i) {
         i->_parent = NULL;
+        i->_child_type = CHILD_ORPHAN;
     }
     _children.clear_and_dispose(DeleteDisposer());
 }
@@ -217,7 +230,8 @@ DrawingItem::setClip(DrawingItem *item)
     _clip = item;
     if (item) {
         item->_parent = this;
-        item->_clip_child = true;
+        assert(item->_child_type == CHILD_ORPHAN);
+        item->_child_type = CHILD_CLIP;
     }
     _markForUpdate(STATE_ALL, true);
 }
@@ -230,7 +244,8 @@ DrawingItem::setMask(DrawingItem *item)
     _mask = item;
         if (item) {
         item->_parent = this;
-        item->_mask_child = true;
+        assert(item->_child_type == CHILD_ORPHAN);
+        item->_child_type = CHILD_MASK;
     }
     _markForUpdate(STATE_ALL, true);
 }
@@ -294,7 +309,7 @@ DrawingItem::update(Geom::IntRect const &area, UpdateContext const &ctx, unsigne
     if ((~_state & flags) == 0) return;  // nothing to do
 
     // TODO this might be wrong
-    if (_state & (outline ? STATE_BBOX : STATE_DRAWBOX)) {
+    if (_state & STATE_BBOX) {
         // we have up-to-date bbox
         if (!area.intersects(outline ? _bbox : _drawbox)) return;
     }
@@ -655,25 +670,39 @@ DrawingItem::clip(Inkscape::DrawingContext &ct, Geom::IntRect const &area)
  *               When true, invisible and insensitive objects can also be picked.
  */
 DrawingItem *
-DrawingItem::pick(Geom::Point const &p, double delta, bool sticky)
+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))
         return NULL;
-
-    if (!sticky && !(_visible && _sensitive))
+    // ignore invisible and insensitive items unless sticky
+    if (!(flags & PICK_STICKY) && !(_visible && _sensitive))
         return NULL;
 
-    // some part of the shape might be hidden by clipping
-    // TODO add Geom::OptRect(Geom::OptIntRect const &) constructor
-    Geom::OptIntRect expanded_i = _bbox & _drawbox;
-    Geom::OptRect expanded = expanded_i ? Geom::Rect(*expanded_i) : Geom::OptRect();
-    if (!expanded) return NULL;
-    expanded->expandBy(delta);
+    bool outline = _drawing.outline();
+
+    if (!_drawing.outline()) {
+        // pick inside clipping path; if NULL, it means the object is clipped away there
+        if (_clip) {
+            DrawingItem *cpick = _clip->pick(p, delta, flags | PICK_AS_CLIP);
+            if (!cpick) return NULL;
+        }
+        // same for mask
+        if (_mask) {
+            DrawingItem *mpick = _mask->pick(p, delta, flags);
+            if (!mpick) return NULL;
+        }
+    }
+
+    Geom::OptIntRect box = (outline || (flags & PICK_AS_CLIP)) ? _bbox : _drawbox;
+    if (!box) return NULL;
+
+    Geom::Rect expanded = *box;
+    expanded.expandBy(delta);
 
-    if (expanded->contains(p)) {
-        return _pickItem(p, delta, sticky);
+    if (expanded.contains(p)) {
+        return _pickItem(p, delta, flags);
     }
     return NULL;
 }
diff --git a/src/display/drawing-item.h b/src/display/drawing-item.h
index b934570f2..a50e3ef03 100644
--- a/src/display/drawing-item.h
+++ b/src/display/drawing-item.h
@@ -58,12 +58,16 @@ public:
     };
     enum StateFlags {
         STATE_NONE = 0,
-        STATE_BBOX = (1<<0),    // geometric bounding box is up-to-date
-        STATE_DRAWBOX = (1<<1), // visual bounding box is up-to-date
-        STATE_CACHE = (1<<2),   // cache extents and clean area are up-to-date
-        STATE_PICK = (1<<3),    // can process pick requests
-        STATE_RENDER = (1<<4),  // can be rendered
-        STATE_ALL = (1<<5)-1
+        STATE_BBOX = (1<<0),    // bounding boxes are up-to-date
+        STATE_CACHE = (1<<1),   // cache extents and clean area are up-to-date
+        STATE_PICK = (1<<2),    // can process pick requests
+        STATE_RENDER = (1<<3),  // can be rendered
+        STATE_ALL = (1<<4)-1
+    };
+    enum PickFlags {
+        PICK_NORMAL = 0, // normal pick
+        PICK_STICKY = (1<<0), // sticky pick - ignore visibility and sensitivity
+        PICK_AS_CLIP = (1<<2) // pick with no stroke and opaque fill regardless of item style
     };
 
     DrawingItem(Drawing &drawing);
@@ -103,9 +107,19 @@ public:
     void update(Geom::IntRect const &area = Geom::IntRect::infinite(), UpdateContext const &ctx = UpdateContext(), unsigned flags = STATE_ALL, unsigned reset = 0);
     void render(DrawingContext &ct, Geom::IntRect const &area, unsigned flags = 0);
     void clip(DrawingContext &ct, Geom::IntRect const &area);
-    DrawingItem *pick(Geom::Point const &p, double delta, bool sticky);
+    DrawingItem *pick(Geom::Point const &p, double delta, unsigned flags = 0);
 
 protected:
+    enum ChildType {
+        CHILD_ORPHAN = 0, // no parent
+        CHILD_NORMAL = 1, // contained in _children of parent
+        CHILD_CLIP = 2, // referenced by _clip member of parent
+        CHILD_MASK = 3, // referenced by _mask member of parent
+        CHILD_ROOT = 4, // root item of _drawing
+        CHILD_FILL_PATTERN = 5, // not yet implemented: referenced by fill pattern of parent
+        CHILD_STROKE_PATTERN = 6 // not yet implemented: referenced by stroke pattern of parent
+    };
+
     void _renderOutline(DrawingContext &ct, Geom::IntRect const &area, unsigned flags);
     void _markForUpdate(unsigned state, bool propagate);
     void _markForRendering();
@@ -116,7 +130,7 @@ protected:
                                  unsigned flags, unsigned reset) { return 0; }
     virtual void _renderItem(DrawingContext &ct, Geom::IntRect const &area, unsigned flags) {}
     virtual void _clipItem(DrawingContext &ct, Geom::IntRect const &area) {}
-    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, bool sticky = false) { return NULL; }
+    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, unsigned flags) { return NULL; }
     virtual bool _canClip() { return false; }
 
     // member variables start here
@@ -152,23 +166,18 @@ protected:
     CacheList::iterator _cache_iterator;
 
     unsigned _state : 8;
+    unsigned _child_type : 3; // see ChildType enum
     unsigned _visible : 1;
     unsigned _sensitive : 1; ///< Whether this item responds to events
     unsigned _cached : 1; ///< Whether the rendering is stored for reuse
     unsigned _cached_persistent : 1; ///< If set, will always be cached regardless of score
-    unsigned _has_cache_iterator : 1; ///< If set, _cache_list_pos is valid
+    unsigned _has_cache_iterator : 1; ///< If set, _cache_iterator is valid
     unsigned _propagate : 1; ///< Whether to call update for all children on next update
     //unsigned _renders_opacity : 1; ///< Whether object needs temporary surface for opacity
-    unsigned _clip_child : 1; ///< If set, this is not a child of _parent, but a clipping path
-    unsigned _mask_child : 1; ///< If set, this is not a child of _parent, but a mask
-    unsigned _drawing_root : 1; ///< If set, this is the root item of Drawing
     unsigned _pick_children : 1; ///< For groups: if true, children are returned from pick(),
                                  ///  otherwise the group is returned
 
     friend class Drawing;
-
-private:
-    DrawingItem(DrawingItem const &);
 };
 
 struct DeleteDisposer {
diff --git a/src/display/drawing-shape.cpp b/src/display/drawing-shape.cpp
index 1e41bf5dd..b333b50f8 100644
--- a/src/display/drawing-shape.cpp
+++ b/src/display/drawing-shape.cpp
@@ -232,7 +232,7 @@ DrawingShape::_clipItem(DrawingContext &ct, Geom::IntRect const &area)
 }
 
 DrawingItem *
-DrawingShape::_pickItem(Geom::Point const &p, double delta, bool /*sticky*/)
+DrawingShape::_pickItem(Geom::Point const &p, double delta, unsigned flags)
 {
     if (_repick_after > 0)
         --_repick_after;
@@ -264,8 +264,9 @@ DrawingShape::_pickItem(Geom::Point const &p, double delta, bool /*sticky*/)
 
     double dist = Geom::infinity();
     int wind = 0;
-    bool needfill = (_nrstyle.fill.type != NRStyle::PAINT_NONE 
+    bool needfill = (flags & PICK_AS_CLIP) || (_nrstyle.fill.type != NRStyle::PAINT_NONE 
              && _nrstyle.fill.opacity > 1e-3 && !outline);
+    bool wind_evenodd = (flags & PICK_AS_CLIP) ? (_style->clip_rule.computed == SP_WIND_RULE_EVENODD) : (_style->fill_rule.computed == SP_WIND_RULE_EVENODD);
 
     if (_drawing.arena()) {
         Geom::Rect viewbox = _drawing.arena()->item.canvas->getViewbox();
@@ -285,13 +286,13 @@ DrawingShape::_pickItem(Geom::Point const &p, double delta, bool /*sticky*/)
 
     // covered by fill?
     if (needfill) {
-        if (!_style->fill_rule.computed) {
-            if (wind != 0) {
+        if (wind_evenodd) {
+            if (wind & 0x1) {
                 _last_pick = this;
                 return this;
             }
         } else {
-            if (wind & 0x1) {
+            if (wind != 0) {
                 _last_pick = this;
                 return this;
             }
@@ -309,7 +310,7 @@ DrawingShape::_pickItem(Geom::Point const &p, double delta, bool /*sticky*/)
 
     // if not picked on the shape itself, try its markers
     for (ChildrenList::iterator i = _children.begin(); i != _children.end(); ++i) {
-        DrawingItem *ret = i->pick(p, delta, false);
+        DrawingItem *ret = i->pick(p, delta, flags & ~PICK_STICKY);
         if (ret) {
             _last_pick = this;
             return this;
diff --git a/src/display/drawing-shape.h b/src/display/drawing-shape.h
index 153dcd54e..2938d6397 100644
--- a/src/display/drawing-shape.h
+++ b/src/display/drawing-shape.h
@@ -36,7 +36,7 @@ protected:
                                  unsigned flags, unsigned reset);
     virtual void _renderItem(DrawingContext &ct, Geom::IntRect const &area, unsigned flags);
     virtual void _clipItem(DrawingContext &ct, Geom::IntRect const &area);
-    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, bool sticky);
+    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, unsigned flags);
     virtual bool _canClip();
 
     SPCurve *_curve;
diff --git a/src/display/drawing-text.cpp b/src/display/drawing-text.cpp
index 2f0881c49..21588cc4f 100644
--- a/src/display/drawing-text.cpp
+++ b/src/display/drawing-text.cpp
@@ -102,7 +102,7 @@ DrawingGlyphs::_updateItem(Geom::IntRect const &area, UpdateContext const &ctx,
 }
 
 DrawingItem *
-DrawingGlyphs::_pickItem(Geom::Point const &p, double delta, bool /*sticky*/)
+DrawingGlyphs::_pickItem(Geom::Point const &p, double delta, unsigned /*flags*/)
 {
     if (!_font || !_bbox) return NULL;
 
@@ -248,9 +248,9 @@ DrawingText::_clipItem(DrawingContext &ct, Geom::IntRect const &area)
 }
 
 DrawingItem *
-DrawingText::_pickItem(Geom::Point const &p, double delta, bool sticky)
+DrawingText::_pickItem(Geom::Point const &p, double delta, unsigned flags)
 {
-    DrawingItem *picked = DrawingGroup::_pickItem(p, delta, sticky);
+    DrawingItem *picked = DrawingGroup::_pickItem(p, delta, flags);
     if (picked) return this;
     return NULL;
 }
diff --git a/src/display/drawing-text.h b/src/display/drawing-text.h
index 671f8f64e..faa33057c 100644
--- a/src/display/drawing-text.h
+++ b/src/display/drawing-text.h
@@ -32,7 +32,7 @@ public:
 protected:
     unsigned _updateItem(Geom::IntRect const &area, UpdateContext const &ctx,
                                  unsigned flags, unsigned reset);
-    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, bool sticky);
+    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, unsigned flags);
 
     Geom::Affine *_glyph_transform;
     font_instance *_font;
@@ -59,7 +59,7 @@ protected:
                                  unsigned flags, unsigned reset);
     virtual void _renderItem(DrawingContext &ct, Geom::IntRect const &area, unsigned flags);
     virtual void _clipItem(DrawingContext &ct, Geom::IntRect const &area);
-    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, bool sticky);
+    virtual DrawingItem *_pickItem(Geom::Point const &p, double delta, unsigned flags);
     virtual bool _canClip();
 
     Geom::OptRect _paintbox;
diff --git a/src/display/drawing.cpp b/src/display/drawing.cpp
index 5881c84ed..e1a17edf1 100644
--- a/src/display/drawing.cpp
+++ b/src/display/drawing.cpp
@@ -42,7 +42,10 @@ Drawing::setRoot(DrawingItem *item)
 {
     delete _root;
     _root = item;
-    _root->_drawing_root = true;
+    if (item) {
+        assert(item->_child_type == DrawingItem::CHILD_ORPHAN);
+        item->_child_type = DrawingItem::CHILD_ROOT;
+    }
 }
 
 RenderMode
@@ -168,10 +171,10 @@ Drawing::render(DrawingContext &ct, Geom::IntRect const &area, unsigned flags)
 }
 
 DrawingItem *
-Drawing::pick(Geom::Point const &p, double delta, bool sticky)
+Drawing::pick(Geom::Point const &p, double delta, unsigned flags)
 {
     if (_root) {
-        return _root->pick(p, delta, sticky);
+        return _root->pick(p, delta, flags);
     }
     return NULL;
 }
diff --git a/src/display/drawing.h b/src/display/drawing.h
index a8e70bbe6..011bf35a6 100644
--- a/src/display/drawing.h
+++ b/src/display/drawing.h
@@ -60,7 +60,7 @@ public:
 
     void update(Geom::IntRect const &area = Geom::IntRect::infinite(), UpdateContext const &ctx = UpdateContext(), unsigned flags = DrawingItem::STATE_ALL, unsigned reset = 0);
     void render(DrawingContext &ct, Geom::IntRect const &area, unsigned flags = 0);
-    DrawingItem *pick(Geom::Point const &p, double delta, bool sticky);
+    DrawingItem *pick(Geom::Point const &p, double delta, unsigned flags);
 
     sigc::signal<void, DrawingItem *> signal_request_update;
     sigc::signal<void, Geom::IntRect const &> signal_request_render;
diff --git a/src/display/nr-filter.cpp b/src/display/nr-filter.cpp
index df6b6222b..ef9ac5be6 100644
--- a/src/display/nr-filter.cpp
+++ b/src/display/nr-filter.cpp
@@ -284,7 +284,7 @@ Geom::Rect Filter::filter_effect_area(Geom::Rect const &bbox)
 
 double Filter::complexity(Geom::Affine const &ctm)
 {
-    double factor;
+    double factor = 1.0;
     for (unsigned i = 0 ; i < _primitive.size() ; i++) {
         if (_primitive[i]) {
             double f = _primitive[i]->complexity(ctm);