[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: review (flake)
From:       Thorsten Zachmann <t.zachmann () zagge ! de>
Date:       2009-11-03 4:48:26
Message-ID: 200911030548.26808.t.zachmann () zagge ! de
[Download RAW message or body]

On Mon November 2 2009, Thomas Zander wrote:
> On Friday 30. October 2009 06.30.37 Thorsten Zachmann wrote:
> > On Thu October 29 2009, Thomas Zander wrote:
> > > could someone please review r1042478 for backporting?
> > >
> > > On add/remove shape make sure we do collision detection
> > >     BUG:185342
> > >
> > >
> > >  void KoShapeManager::addAdditional(KoShape *shape)
> > > @@ -218,6 +219,8 @@
> > >
> > >  void KoShapeManager::remove(KoShape *shape)
> > >  {
> > > +    notifyShapeChanged(shape);
> > > +    d->updateTree();
> >
> > This does do some unnecessary stuff like adding removing the the shape
> > from the tree, and does the shape collision detection twice which is not
> > needed when removing a shape.
> 
> I am not sure what you are referring to here, the
> KoShapeManager::Private::updateTree method is only called once inside the
> remove() method.
> I can't see anything being done twice or unnecessary.

The stuff done twice and unnecessary is inside the updateTree method. There 2 
times the collision detection is done and also an update of the tree. That is 
not necessary when a shape is removed. One collision detection is enough

> > How about not calling updateTree but instead a function that does one
> > shape collision detection when a shape is removed? This will also avoid
> > calling update tree for every shape removed which is wrong as updateTree
> > is specially there to handle aggregated updates of shapes which is
> > circumvented by the patch.
> 
> I don't see what difference that would make.
> Calling updateTree 10 times in a row doesn't do more work, the list of
> aggregated shapes doesn't get bigger.
> We need to do the calculation before the remove() returns as the shape will
> likely be deleted later.

Shapes can get notified multiple times when it is done this way which does not 
happen when it is called only ones.

> 
> I don't see how we could make the patch better.
> If you see it, please let me know.

If you make the DetectCollision accessible inside the KoShapeManager you can 
use that directly in remove.

> If you don't have time, would you see any problem with me backporting the
> fix? It solving an often reported user problem that I'd like to see solved
>  in 2.1-final.

I have created a patch against head that does what I described above. What do 
you think?

Thorsten

["shape_manager.diff" (text/x-patch)]

Index: KoShapeManager.cpp
===================================================================
--- KoShapeManager.cpp	(revision 1043217)
+++ KoShapeManager.cpp	(working copy)
@@ -65,20 +65,6 @@ public:
      */
     void updateTree();
 
-    QList<KoShape *> shapes;
-    QList<KoShape *> additionalShapes; // these are shapes that are only handled for updates
-    KoSelection * selection;
-    KoCanvasBase * canvas;
-    KoRTree<KoShape *> tree;
-    QSet<KoShape *> aggregate4update;
-    QHash<KoShape*, int> shapeIndexesBeforeUpdate;
-    KoShapeManagerPaintingStrategy * strategy;
-    KoShapeManager *q;
-};
-
-void KoShapeManager::Private::updateTree()
-{
-    // for detecting collisions between shapes.
     class DetectCollision
     {
     public:
@@ -88,8 +74,10 @@ void KoShapeManager::Private::updateTree
                 bool isChild = false;
                 KoShapeContainer *parent = s->parent();
                 while (parent && !isChild) {
-                    if (parent == shape)
+                    if (parent == shape) {
                         isChild = true;
+                        break;
+                    }
                     parent = parent->parent();
                 }
                 if (isChild)
@@ -110,6 +98,21 @@ void KoShapeManager::Private::updateTree
     private:
         QList<KoShape*> shapesWithCollisionDetection;
     };
+
+    QList<KoShape *> shapes;
+    QList<KoShape *> additionalShapes; // these are shapes that are only handled for updates
+    KoSelection * selection;
+    KoCanvasBase * canvas;
+    KoRTree<KoShape *> tree;
+    QSet<KoShape *> aggregate4update;
+    QHash<KoShape*, int> shapeIndexesBeforeUpdate;
+    KoShapeManagerPaintingStrategy * strategy;
+    KoShapeManager *q;
+};
+
+void KoShapeManager::Private::updateTree()
+{
+    // for detecting collisions between shapes.
     DetectCollision detector;
     bool selectionModified = false;
     foreach(KoShape *shape, aggregate4update) {
@@ -138,7 +141,6 @@ void KoShapeManager::Private::updateTree
     }
 }
 
-
 KoShapeManager::KoShapeManager(KoCanvasBase *canvas, const QList<KoShape *> &shapes)
         : d(new Private(this, canvas))
 {
@@ -219,8 +221,10 @@ void KoShapeManager::addAdditional(KoSha
 
 void KoShapeManager::remove(KoShape *shape)
 {
-    notifyShapeChanged(shape);
-    d->updateTree();
+    Private::DetectCollision detector;
+    detector.detect(d->tree, shape, shape->zIndex());
+    detector.fireSignals();
+
     shape->update();
     shape->removeShapeManager(this);
     d->selection->deselect(shape);


_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel


[prev in list] [next in list] [prev in thread] [next in thread]