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

List:       koffice-devel
Subject:    Re: koffice/plugins/defaultTools/defaulttool
From:       Thomas Zander <zander () kde ! org>
Date:       2009-11-17 23:36:57
Message-ID: 200911180036.58432.zander () kde ! org
[Download RAW message or body]

On Tuesday 17. November 2009 14.17.36 Jan Hambrecht wrote:
> Thomas Zander wrote:
> > On Tuesday 17. November 2009 12.59.58 Jan Hambrecht wrote:
> >> Probably, but then I would have to duplicate all the group related
> >> code also. More work now but it might be easier in the future for me.
> >
> > I'm a bit surprised by this, the KoShapeGroup was finalized in its
> > design years ago and I never noticed Karbon doing something a bit
> > different there. Can you really select a group shape in karbon
> > currently?
> >
> > Maybe you could please explain what usecase the old group concepts /
> > design fails for so we can put our minds together and come up with the
> > best solution for everyone.
>
> Some problems I encountered while working on svg filter effects:
> 1. If a group has a filter effect applied, it needs to be added to the
> KoRTree of the shape manager so it is returned by
> KoRTree::intersects(QRectF) when painting shapes.
> 2. Size and position of groups need to be correct as svg filter effect
> regions depend on the shapes bounding box they are applied to.

These two things are completely at odds with the design and implementation 
of groups as they are now.  Groups should not have a position or a size, 
or at minimum its irrelevant.  Again, according to how they were designed 
and are now (mostly) implemented.

If this is something that doesn't work we should have a long look at groups 
and see if we need to redesign things and check all the implications it has 
on things.

If we want to avoid bugfixing little details for ages we need to properly 
look at the design of this. Maybe we need to introduce a new class, maybe 
we can have a mode. I don't know right now.
All I'm saying is that subtle behavior changes like the one this shows tear 
the design in the middle and will likely break things in unexpected ways.

Looking at the actual bug that Thorsten tried to fix, this is indeed 
exactly what happened. The initial design ages ago was to never ever make 
a KoShapeGroup become part of the selection. So this was never an issue.
If I look at the code I see that the group is now added, which looks wrong.

So instead of the fix that Thorsten made, may I suggest this?
Is there any reason for a group to be in the selection?

--- libs/flake/KoSelection.cpp
+++ libs/flake/KoSelection.cpp
@@ -152,7 +152,6 @@ void KoSelection::select(KoShape *shape, bool recursive)
             KoShapeGroup *parentGroup = dynamic_cast<KoShapeGroup*>(parent);
             if (! parentGroup) break;
             if (! d->selectedShapes.contains(parentGroup)) {
-                d->selectedShapes << parentGroup;
                 d->selectGroupChildren(parentGroup);
             }
             parent = parentGroup->parent();

-- 
Thomas Zander
_______________________________________________
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]