review (flake)

could someone please review r1042478 for backporting?

On add/remove shape make sure we do collision detection
    BUG:185342

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

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.
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.

>      shape->update();
>      shape->removeShapeManager(this);
>      d->selection->deselect(shape);
>

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

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.

> 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.

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

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.
--
Thomas Zander
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

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]

---

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@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Tuesday 3. November 2009 05.48.26 Thorsten Zachmann wrote:
> I have created a patch against head that does what I described above.
> What do you think?

Thanks for that!
I committed and backported the patch now.
--
Thomas Zander
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel