Architectural fix for defs (for marker bug 1298718)
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,
Could you identify the one-line fix please? I'm having trouble isolating the refactoring work from the fix itself.
For the most part this appears to be sound, but, I'm a bit worried about _childList no longer being virtual.
I realize you can't currently put virtual methods on an SPObject, but non-virtual polymorphic methods are seldom desirable... I think the approach here is probably to create a CObject analagous to CGroup ( CObject:SPObject::CGroup::SPItemGroup ), have CGroup derive from it, and put the method there.
On Wed, 2006-07-19 at 15:30 -0700, Bryce Harrington wrote:
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 */
/* 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 fromsp_document_ensure_is_up_to_date() andsp_document_idle_handler(). This is the first step in gettingupdates of defs fixed.2006-07-18 Tim Dwyer <Tim.Dwyer@...38...>
- src/graphlayout/graphlayout.cpp,
Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=D... _______________________________________________ Inkscape-devel mailing list Inkscape-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/inkscape-devel
On Wed, Jul 19, 2006 at 08:07:27PM -0400, MenTaLguY wrote:
Could you identify the one-line fix please? I'm having trouble isolating the refactoring work from the fix itself.
Sure, indicated below. On reflection, I am not certain it was this one-line fix alone; as I recall, there were one or two refactorings that you identified which may have contributed.
For the most part this appears to be sound, but, I'm a bit worried about _childList no longer being virtual.
I realize you can't currently put virtual methods on an SPObject, but non-virtual polymorphic methods are seldom desirable... I think the approach here is probably to create a CObject analagous to CGroup ( CObject:SPObject::CGroup::SPItemGroup ), have CGroup derive from it, and put the method there.
Yeah, that's exactly the issue I ran into. I figure additional work may be needed in creating CObject, etc. but decided that was too tangential to the marker work I want to stay focused on.
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))) {
This is what I originally believed fixed the issue.
However, I think I must have attempted this fix one or two times early, which is why I think one of the other refactorings enabled this fix to actually work. Might be worth testing this in isolation.
child->updateDisplay(ctx, flags); }
g_object_unref(G_OBJECT(child));
g_object_unref (G_OBJECT (child));}
Bryce
I've gone ahead and committed this. If the virtual thing is the only issue, and it otherwise looks okay, it's probably better to have it in the codebase where people can start testing it. Looks like Aaron is creating a script to take advantage of this fix to address the marker coloring bug, so this'll get him able to test that.
Please keep an eye out for anything odd showing up with things related to defs like markers, gradients, etc. and report asap.
Bryce
On Wed, Jul 19, 2006 at 07:23:38PM -0700, Bryce Harrington wrote:
On Wed, Jul 19, 2006 at 08:07:27PM -0400, MenTaLguY wrote:
Could you identify the one-line fix please? I'm having trouble isolating the refactoring work from the fix itself.
Sure, indicated below. On reflection, I am not certain it was this one-line fix alone; as I recall, there were one or two refactorings that you identified which may have contributed.
For the most part this appears to be sound, but, I'm a bit worried about _childList no longer being virtual.
I realize you can't currently put virtual methods on an SPObject, but non-virtual polymorphic methods are seldom desirable... I think the approach here is probably to create a CObject analagous to CGroup ( CObject:SPObject::CGroup::SPItemGroup ), have CGroup derive from it, and put the method there.
Yeah, that's exactly the issue I ran into. I figure additional work may be needed in creating CObject, etc. but decided that was too tangential to the marker work I want to stay focused on.
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))) {This is what I originally believed fixed the issue.
However, I think I must have attempted this fix one or two times early, which is why I think one of the other refactorings enabled this fix to actually work. Might be worth testing this in isolation.
child->updateDisplay(ctx, flags); }
g_object_unref(G_OBJECT(child));
g_object_unref (G_OBJECT (child));}
Bryce
Bryce Harrington wrote:
I've gone ahead and committed this. If the virtual thing is the only issue, and it otherwise looks okay, it's probably better to have it in the codebase where people can start testing it. Looks like Aaron is creating a script to take advantage of this fix to address the marker coloring bug, so this'll get him able to test that.
Well, my script runs well enough from the command line, but fails miserably when called from the Effects menu. I actually looks like there is code written specificly to prevent editing the defs via effects. See http://inkscape.modevia.com/doxygen/html/classInkscape_1_1Extension_1_1Imple...
Ted, could you please comment?
Aaron Spike
Aaron Spike wrote:
Bryce Harrington wrote:
I've gone ahead and committed this. If the virtual thing is the only issue, and it otherwise looks okay, it's probably better to have it in the codebase where people can start testing it. Looks like Aaron is creating a script to take advantage of this fix to address the marker coloring bug, so this'll get him able to test that.
Well, my script runs well enough from the command line, but fails miserably when called from the Effects menu. I actually looks like there is code written specificly to prevent editing the defs via effects. See http://inkscape.modevia.com/doxygen/html/classInkscape_1_1Extension_1_1Imple...
BTW, when I comment out those 4 lines the script works to change a markers to match a stroke with a solid color. I'm not sure what happens with gradients.
Aaron Spike
On Sun, Jul 23, 2006 at 08:43:24AM -0500, Aaron Spike wrote:
Aaron Spike wrote:
Bryce Harrington wrote:
I've gone ahead and committed this. If the virtual thing is the only issue, and it otherwise looks okay, it's probably better to have it in the codebase where people can start testing it. Looks like Aaron is creating a script to take advantage of this fix to address the marker coloring bug, so this'll get him able to test that.
Well, my script runs well enough from the command line, but fails miserably when called from the Effects menu. I actually looks like there is code written specificly to prevent editing the defs via effects. See http://inkscape.modevia.com/doxygen/html/classInkscape_1_1Extension_1_1Imple...
BTW, when I comment out those 4 lines the script works to change a markers to match a stroke with a solid color. I'm not sure what happens with gradients.
I'm sure ted had a rationale for why to hide defs, but if your tests with this patch don't display any untoward behavior among the various scripts, then maybe we should commit it, and see what misbehaviors actually turn up in practice. Perhaps there is a less brute force way to handle those issues. I think being able to use scripts to vary defs would enable a lot of handy new extensions.
Bryce
On Sun, 23 Jul 2006, Aaron Spike wrote:
Well, my script runs well enough from the command line, but fails miserably when called from the Effects menu. I actually looks like there is code written specificly to prevent editing the defs via effects. See http://inkscape.modevia.com/doxygen/html/classInkscape_1_1Extension_1_1Imple...
BTW, when I comment out those 4 lines the script works to change a markers to match a stroke with a solid color. I'm not sure what happens with gradients.
Hmm, I can't remember why specifically, but for some reason I thought copying defs caused problems... But, that might have been before we added the "destroying the document" signal. Specifically "ReconstructionStart" and "ResconstructionFinish". But, according to Doxygen that signal isn't referenced anywhere (I'm pretty sure it is though, I can't remember where). I think that may have 'fixed' the problem, although the protection wasn't backed out.
My feelings are, if it works, test it some more, and then check it in :)
--Ted
On Mon, Jul 24, 2006 at 12:22:00PM -0500, Ted Gould wrote:
On Sun, 23 Jul 2006, Aaron Spike wrote:
Well, my script runs well enough from the command line, but fails miserably when called from the Effects menu. I actually looks like there is code written specificly to prevent editing the defs via effects. See http://inkscape.modevia.com/doxygen/html/classInkscape_1_1Extension_1_1Imple...
BTW, when I comment out those 4 lines the script works to change a markers to match a stroke with a solid color. I'm not sure what happens with gradients.
Hmm, I can't remember why specifically, but for some reason I thought copying defs caused problems... But, that might have been before we added the "destroying the document" signal. Specifically "ReconstructionStart" and "ResconstructionFinish". But, according to Doxygen that signal isn't referenced anywhere (I'm pretty sure it is though, I can't remember where). I think that may have 'fixed' the problem, although the protection wasn't backed out.
My feelings are, if it works, test it some more, and then check it in :)
Sounds good. This'll be a nifty thing to be able to do with extensions. :-)
Bryce
On Wed, 2006-07-19 at 15:30 -0700, Bryce Harrington wrote:
- 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.
FWIW, reading the code it appears to be for the benefit of SPSwitch. With SPSwitch, whether or not certain children should be included in an enumeration of children depends upon the purpose for which they are being enumerated.
I think there are probably better ways to accomplish that, but that bit is a concern we can deal with some other time.
-mental
participants (4)
-
Aaron Spike -
Bryce Harrington -
MenTaLguY -
Ted Gould