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

List:       koffice-devel
Subject:    Re: koffice/libs/flake/tests
From:       Jan Hambrecht <jaham () gmx ! net>
Date:       2008-08-18 11:00:50
Message-ID: 48A95662.8000603 () gmx ! net
[Download RAW message or body]

Thomas Zander wrote:
> On Friday 15. August 2008 00:52:58 Jan Hambrecht wrote:
>> SVN commit 847200 by jaham:
>>
>> fix unit test after fixing selection bug
> 
> I'm reading your patch and I see you changed the behavior for 
> the "FullSelection" to return a different set as I designed it to return. 
> This change in behavior most probably breaks code that depended on the lack 
> of group objects in the selection.
> 
> I can understand that if you read the enum name "FullSelection" you expect 
> also the groups to be returned, so I understand your change. :)
> 
> To be clear we have two strategies;
> a) return all shapes, all grouping shapes and all containers.
> b) return all shapes, but not the grouping meta-objects. (KoShapeGroup)
> 
> This is next to ;
> StrippedSelection,  ///< Create a stripped list, without children if the 
> container is also in the list.
> TopLevelSelection   ///< Create a list, much like the StrippedSelection, but 
> have the KoShapeGroup instead of all of its children if one is selected.
> 
> Behavior (a) is what you changed it into.
> 
> Behavior (b) makes most sense to me because if I click on a group I want all 
> its members selected, but not the group object itself. Thats not really 
> anything the user ever should select. It breaks code that assumes the group 
> is not selected.
> 
> So my question to you is
> * why did you need the selection to return the group object AND the child 
> objects?  Can't you use the other selection strategies for your usecase?
> If not; I suggest to create a new enum to restore behavior (b) as it was 
> designed.
> If you can make due with the previously known strategies, can we instead of 
> your change in behavior rename the enum value to something more clear?
> 
> What do you think?

The reason i changed the behaviour is because i wanted to fix selection 
related crashes. Those happened because KoSelection::count() always 
returns the number of all shapes (including group shapes) of the 
selection. If there is only a group shape left in the selection and i 
call KoSelection::firstSelectedShape( Koflake::FullSelection 
)->doSomething() i get a crash because in that case a null pointer was 
returned. First i thought just fix these places where something like 
that appeared in the code:
if( selection->count() )
     selection->firstSelectedShape()->doSomething();
But then i wondered that it might not be the right place to fix this and 
there will probably more places where code like this will appear. So i 
read the description of KoFlake::FullSelection which says: "Create a 
list of all objects in the selection". This was the hint which let me 
change the behaviour to match the description and the enum identifier.
So yes FullSelection and its docs are misleading which led me to change 
that. Imho atm KoSelection::count() in its current form is not usable at 
all because it probably returns a number of shapes which i will not get 
when calling KoSelection::selectedShapes(...) and thus will break code.

Ciao Jan

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