Over the past couple evenings after the DDC talks, mental and I have been hacking on one of the marker bugs, and last night managed to find the fix to it. It seems to work okay but I haven't had a chance to really exercise it so there may be some side effects... Patch is attached. Please test out gradients and other things that use defs, undo/redo, etc.
Here's the bug:
http://sourceforge.net/tracker/index.php?func=detail&aid=1298718&gro...
seems like a fairly obscure issue, that not many users would run into, but it is a key underlying bug for being able to alter markers (and anything else that uses defs).
The actually fix was a one-liner (as most things are), but in finding it we've also done:
* Added clearer, more complete comments/documentation for the modify/update paths in SPObject and Document.
* Factored out common code in SPObject subclasses for getting a child list; now this is provided by SPObject's new childList() member.
* Refactored updateDisplay() and requestDisplayUpdate() to highlight their similarities; maybe this could suggest a better refactoring in the future. Now they're consistent at least.
* Added new pseudo-private member function _updateDocument() to the Document class, which factors out the flag-based update code to a single place. sp_document_ensure_up_to_date and sp_document_idle_handler have been refactored to move their shared logic into _updateDocument().
* An Action enumeration thingee originally implemented in sp-item-group has been promoted up to SPObject. Honestly I don't quite understand the purpose of it, but it doesn't appear to hurt anything, so hopefully whomever added this knows what it's for and can put it to more use.
Anyway, now that this is fixed, the way is cleared for further enhancements to the marker code.
Bryce
P.S., Mental, can you commit this? I'm still having trouble committing to svn.
Index: src/sp-object.h =================================================================== --- src/sp-object.h (revision 12719) +++ src/sp-object.h (working copy) @@ -206,6 +206,10 @@ SPObject *lastChild() { return _last_child; } SPObject const *lastChild() const { return _last_child; }
+ enum Action { ActionGeneral, ActionBBox, ActionUpdate, ActionShow }; + /** @brief Retrieves children as a GSList */ + GSList *childList(bool add_ref, Action action = ActionGeneral); + SPObject *appendChildRepr(Inkscape::XML::Node *repr);
/** @brief Gets the author-visible label for this object. */ Index: src/document.h =================================================================== --- src/document.h (revision 12719) +++ src/document.h (working copy) @@ -115,6 +115,8 @@
Inkscape::EventLog& getEventLog() const;
+ bool _updateDocument(); + private: SPDocument(SPDocument const &); // no copy void operator=(SPDocument const &); // no assign Index: src/sp-item-group.cpp =================================================================== --- src/sp-item-group.cpp (revision 12719) +++ src/sp-item-group.cpp (working copy) @@ -592,13 +592,15 @@ }
void CGroup::onUpdate(SPCtx *ctx, unsigned int flags) { - SPObject *child; SPItemCtx *ictx, cctx;
ictx = (SPItemCtx *) ctx; cctx = *ictx;
- if (flags & SP_OBJECT_MODIFIED_FLAG) flags |= SP_OBJECT_PARENT_MODIFIED_FLAG; + if (flags & SP_OBJECT_MODIFIED_FLAG) { + flags |= SP_OBJECT_PARENT_MODIFIED_FLAG; + } + flags &= SP_OBJECT_MODIFIED_CASCADE;
if (flags & SP_OBJECT_STYLE_MODIFIED_FLAG) { @@ -608,9 +610,9 @@ } }
- GSList *l = g_slist_reverse(_childList(true, ActionUpdate)); + GSList *l = g_slist_reverse(_group->childList(true, SPObject::ActionUpdate)); while (l) { - child = SP_OBJECT (l->data); + SPObject *child = SP_OBJECT (l->data); l = g_slist_remove (l, child); if (flags || (child->uflags & (SP_OBJECT_MODIFIED_FLAG | SP_OBJECT_CHILD_MODIFIED_FLAG))) { if (SP_IS_ITEM (child)) { @@ -626,17 +628,6 @@ } }
-GSList *CGroup::_childList(bool add_ref, Action) { - GSList *l = NULL; - for (SPObject *child = sp_object_first_child(_group) ; child != NULL ; child = SP_OBJECT_NEXT(child) ) { - if (add_ref) - g_object_ref (G_OBJECT (child)); - - l = g_slist_prepend (l, child); - } - return l; -} - void CGroup::onModified(guint flags) { SPObject *child;
@@ -650,7 +641,7 @@ } }
- GSList *l = g_slist_reverse(_childList(true)); + GSList *l = g_slist_reverse(_group->childList(true)); while (l) { child = SP_OBJECT (l->data); l = g_slist_remove (l, child); @@ -662,7 +653,7 @@ }
void CGroup::calculateBBox(NRRect *bbox, NR::Matrix const &transform, unsigned const flags) { - GSList *l = _childList(false, ActionBBox); + GSList *l = _group->childList(false, SPObject::ActionBBox); while (l) { SPObject *o = SP_OBJECT (l->data); if (SP_IS_ITEM(o)) { @@ -675,7 +666,7 @@ }
void CGroup::onPrint(SPPrintContext *ctx) { - GSList *l = g_slist_reverse(_childList(false)); + GSList *l = g_slist_reverse(_group->childList(false)); while (l) { SPObject *o = SP_OBJECT (l->data); if (SP_IS_ITEM(o)) { @@ -723,7 +714,7 @@ NRArenaItem *ac = NULL; NRArenaItem *ar = NULL; SPItem * child = NULL; - GSList *l = g_slist_reverse(_childList(false, ActionShow)); + GSList *l = g_slist_reverse(_group->childList(false, SPObject::ActionShow)); while (l) { SPObject *o = SP_OBJECT (l->data); if (SP_IS_ITEM (o)) { @@ -742,7 +733,7 @@ void CGroup::hide (unsigned int key) { SPItem * child;
- GSList *l = g_slist_reverse(_childList(false, ActionShow)); + GSList *l = g_slist_reverse(_group->childList(false, SPObject::ActionShow)); while (l) { SPObject *o = SP_OBJECT (l->data); if (SP_IS_ITEM (o)) { Index: src/sp-switch.cpp =================================================================== --- src/sp-switch.cpp (revision 12719) +++ src/sp-switch.cpp (working copy) @@ -75,9 +75,9 @@ return NULL; }
-GSList *CSwitch::_childList(bool add_ref, Action action) { - if ( ActionGeneral != action ) { - return CGroup::_childList(add_ref, action); +GSList *CSwitch::_childList(bool add_ref, SPObject::Action action) { + if ( action != SPObject::ActionGeneral ) { + return _group->childList(add_ref, action); }
SPObject *child = _evaluateFirst(); @@ -120,7 +120,7 @@ _releaseLastItem(_cached_item);
SPItem * child; - for ( GSList *l = _childList(false, ActionShow); + for ( GSList *l = _childList(false, SPObject::ActionShow); NULL != l ; l = g_slist_remove (l, l->data)) { SPObject *o = SP_OBJECT (l->data); @@ -162,7 +162,7 @@ NRArenaItem *ac = NULL; NRArenaItem *ar = NULL; SPItem * child; - GSList *l = _childList(false, ActionShow); + GSList *l = _childList(false, SPObject::ActionShow); while (l) { SPObject *o = SP_OBJECT (l->data); if (SP_IS_ITEM (o)) { Index: src/sp-object.cpp =================================================================== --- src/sp-object.cpp (revision 12719) +++ src/sp-object.cpp (working copy) @@ -458,6 +458,22 @@ } }
+/** + * Retrieves the children as a GSList object, optionally ref'ing the children + * in the process, if add_ref is specified. + */ +GSList *SPObject::childList(bool add_ref, Action) { + GSList *l = NULL; + for (SPObject *child = sp_object_first_child(this) ; child != NULL; child = SP_OBJECT_NEXT(child) ) { + if (add_ref) + g_object_ref (G_OBJECT (child)); + + l = g_slist_prepend (l, child); + } + return l; + +} + /** Gets the label property for the object or a default if no label * is defined. */ @@ -1168,26 +1184,31 @@ void SPObject::requestDisplayUpdate(unsigned int flags) { + g_return_if_fail( this->document != NULL ); + if (update_in_progress) { g_print("WARNING: Requested update while update in progress, counter = %d\n", update_in_progress); }
+ /* requestModified must be used only to set one of SP_OBJECT_MODIFIED_FLAG or + * SP_OBJECT_CHILD_MODIFIED_FLAG */ g_return_if_fail(!(flags & SP_OBJECT_PARENT_MODIFIED_FLAG)); g_return_if_fail((flags & SP_OBJECT_MODIFIED_FLAG) || (flags & SP_OBJECT_CHILD_MODIFIED_FLAG)); g_return_if_fail(!((flags & SP_OBJECT_MODIFIED_FLAG) && (flags & SP_OBJECT_CHILD_MODIFIED_FLAG)));
- /* Check for propagate before we set any flags */ - /* Propagate means, that this is not passed through by modification request cascade yet */ - unsigned int propagate = (!(this->uflags & (SP_OBJECT_MODIFIED_FLAG | SP_OBJECT_CHILD_MODIFIED_FLAG))); + bool already_propagated = (!(this->uflags & (SP_OBJECT_MODIFIED_FLAG | SP_OBJECT_CHILD_MODIFIED_FLAG)));
- /* Just set this flags safe even if some have been set before */ this->uflags |= flags;
- if (propagate) { - if (this->parent) { - this->parent->requestDisplayUpdate(SP_OBJECT_CHILD_MODIFIED_FLAG); + /* If requestModified has already been called on this object or one of its children, then we + * don't need to set CHILD_MODIFIED on our ancestors because it's already been done. + */ + if (already_propagated) { + SPObject *parent = SP_OBJECT_PARENT(this); + if (parent) { + parent->requestDisplayUpdate(SP_OBJECT_CHILD_MODIFIED_FLAG); } else { - sp_document_request_modified(this->document); + sp_document_request_modified(SP_OBJECT_DOCUMENT(this)); } } } @@ -1228,29 +1249,30 @@ update_in_progress --; }
+/** + * Request modified always bubbles *up* the tree, as opposed to + * request display update, which trickles down and relies on the + * flags set during this pass... + */ void SPObject::requestModified(unsigned int flags) { g_return_if_fail( this->document != NULL );
- /* PARENT_MODIFIED is computed later on and is not intended to be - * "manually" queued */ + /* requestModified must be used only to set one of SP_OBJECT_MODIFIED_FLAG or + * SP_OBJECT_CHILD_MODIFIED_FLAG */ g_return_if_fail(!(flags & SP_OBJECT_PARENT_MODIFIED_FLAG)); - - /* we should be setting either MODIFIED or CHILD_MODIFIED... */ g_return_if_fail((flags & SP_OBJECT_MODIFIED_FLAG) || (flags & SP_OBJECT_CHILD_MODIFIED_FLAG)); - - /* ...but not both */ g_return_if_fail(!((flags & SP_OBJECT_MODIFIED_FLAG) && (flags & SP_OBJECT_CHILD_MODIFIED_FLAG)));
- unsigned int old_mflags=this->mflags; + bool already_propagated = (!(this->mflags & (SP_OBJECT_MODIFIED_FLAG | SP_OBJECT_CHILD_MODIFIED_FLAG))); + this->mflags |= flags;
- /* If we already had MODIFIED or CHILD_MODIFIED queued, we will - * have already queued CHILD_MODIFIED with our ancestors and - * need not disturb them again. + /* If requestModified has already been called on this object or one of its children, then we + * don't need to set CHILD_MODIFIED on our ancestors because it's already been done. */ - if (!( old_mflags & ( SP_OBJECT_MODIFIED_FLAG | SP_OBJECT_CHILD_MODIFIED_FLAG ) )) { + if (already_propagated) { SPObject *parent=SP_OBJECT_PARENT(this); if (parent) { parent->requestModified(SP_OBJECT_CHILD_MODIFIED_FLAG); @@ -1260,6 +1282,9 @@ } }
+/** + * This is what actually delivers the modified signals + */ void SPObject::emitModified(unsigned int flags) { Index: src/document.cpp =================================================================== --- src/document.cpp (revision 12719) +++ src/document.cpp (working copy) @@ -746,69 +746,75 @@ ctx->i2vp = NR::identity(); }
+/** + * Tries to update the document state based on the modified and + * "update required" flags, and return true if the document has + * been brought fully up to date. + */ +bool +SPDocument::_updateDocument() +{ + /* Process updates */ + if (this->root->uflags || this->root->mflags) { + if (this->root->uflags) { + SPItemCtx ctx; + sp_document_setup_viewport (this, &ctx); + + bool saved = sp_document_get_undo_sensitive(this); + sp_document_set_undo_sensitive(this, FALSE); + + this->root->updateDisplay((SPCtx *)&ctx, 0); + + sp_document_set_undo_sensitive(this, saved); + } + this->_emitModified(); + } + + return !(this->root->uflags || this->root->mflags); +} + + +/** + * Repeatedly works on getting the document updated, since sometimes + * it takes more than one pass to get the document updated. But it + * usually should not take more than a few loops, and certainly never + * more than 32 iterations. So we bail out if we hit 32 iterations, + * since this typically indicates we're stuck in an update loop. + */ gint sp_document_ensure_up_to_date(SPDocument *doc) { - int lc; - lc = 32; - while (doc->root->uflags || doc->root->mflags) { - lc -= 1; - if (lc < 0) { - g_warning("More than 32 iterations while updating document '%s'", doc->uri); - if (doc->modified_id) { - /* Remove handler */ - gtk_idle_remove(doc->modified_id); - doc->modified_id = 0; - } - return FALSE; + int counter = 32; + while (!doc->_updateDocument()) { + if (counter == 0) { + g_warning("More than 32 iteration while updating document '%s'", doc->uri); + break; } - /* Process updates */ - if (doc->root->uflags) { - SPItemCtx ctx; - sp_document_setup_viewport (doc, &ctx); - doc->root->updateDisplay((SPCtx *)&ctx, 0); - } - doc->_emitModified(); + counter--; } + if (doc->modified_id) { /* Remove handler */ gtk_idle_remove(doc->modified_id); doc->modified_id = 0; } - return TRUE; + return counter>0; }
+/** + * An idle handler to update the document. Returns true if + * the document needs further updates. + */ static gint sp_document_idle_handler(gpointer data) { - SPDocument *doc; - int repeat; - - doc = static_cast<SPDocument *>(data); - -#ifdef SP_DOCUMENT_DEBUG_IDLE - g_print("->\n"); -#endif - - /* Process updates */ - if (doc->root->uflags) { - SPItemCtx ctx; - sp_document_setup_viewport (doc, &ctx); - - gboolean saved = sp_document_get_undo_sensitive(doc); - sp_document_set_undo_sensitive(doc, FALSE); - - doc->root->updateDisplay((SPCtx *)&ctx, 0); - - sp_document_set_undo_sensitive(doc, saved); - /* if (doc->root->uflags & SP_OBJECT_MODIFIED_FLAG) return TRUE; */ + SPDocument *doc = static_cast<SPDocument *>(data); + if (doc->_updateDocument()) { + doc->modified_id = 0; + return false; + } else { + return true; } - - doc->_emitModified(); - - repeat = (doc->root->uflags || doc->root->mflags); - if (!repeat) doc->modified_id = 0; - return repeat; }
static bool is_within(NR::Rect const &area, NR::Rect const &box) Index: src/sp-switch.h =================================================================== --- src/sp-switch.h (revision 12719) +++ src/sp-switch.h (working copy) @@ -38,7 +38,7 @@ virtual gchar *getDescription();
protected: - virtual GSList *_childList(bool add_ref, Action action); + virtual GSList *_childList(bool add_ref, SPObject::Action action); virtual void _showChildren (NRArena *arena, NRArenaItem *ai, unsigned int key, unsigned int flags);
SPObject *_evaluateFirst(); Index: src/sp-defs.cpp =================================================================== --- src/sp-defs.cpp (revision 12719) +++ src/sp-defs.cpp (working copy) @@ -83,21 +83,14 @@
flags &= SP_OBJECT_MODIFIED_CASCADE;
- GSList *l = NULL; - for ( SPObject *child = sp_object_first_child(object) ; child != NULL; child = SP_OBJECT_NEXT(child) ) { - g_object_ref(G_OBJECT(child)); - l = g_slist_prepend(l, child); - } - - l = g_slist_reverse(l); - + GSList *l = g_slist_reverse(object->childList(true)); while (l) { SPObject *child = SP_OBJECT(l->data); l = g_slist_remove(l, child); - if (flags || (child->uflags & SP_OBJECT_MODIFIED_FLAG)) { + if (flags || (child->uflags & (SP_OBJECT_MODIFIED_FLAG | SP_OBJECT_CHILD_MODIFIED_FLAG))) { child->updateDisplay(ctx, flags); } - g_object_unref(G_OBJECT(child)); + g_object_unref (G_OBJECT (child)); } }
Index: src/sp-item-group.h =================================================================== --- src/sp-item-group.h (revision 12719) +++ src/sp-item-group.h (working copy) @@ -79,8 +79,6 @@ gint getItemCount();
protected: - enum Action { ActionGeneral, ActionBBox, ActionUpdate, ActionShow }; - virtual GSList *_childList(bool add_ref, Action action = ActionGeneral); virtual void _showChildren (NRArena *arena, NRArenaItem *ai, unsigned int key, unsigned int flags);
SPGroup *_group; Index: ChangeLog =================================================================== --- ChangeLog (revision 12719) +++ ChangeLog (working copy) @@ -1,3 +1,10 @@ +2006-07-18 Bryce Harrington <bryce@...961...> + * src/document.h, src/document.cpp: Refactoring from mental & + bryce to consolidate document update functionality from + sp_document_ensure_is_up_to_date() and + sp_document_idle_handler(). This is the first step in getting + updates of defs fixed. + 2006-07-18 Tim Dwyer <Tim.Dwyer@...38...>
* src/graphlayout/graphlayout.cpp,