Taskmanager with grouping support preview version

[Aaron J. Seigo]

On Thursday 03 July 2008, christian mollekopf wrote:
> I am back with some code...
.. snip ...
> I didn't create diffs because i changed more or less everything.... Just

unfortunately, this makes it pretty much impossible to track changes, provide 
timely feedback and hopefully give the thumbs up / down on anything making it 
into trunk/.

i really don't want to see your efforts wasted.

as it is the code is already fairly divergent from some basic style and coding 
patterns (such as binary compat, apidox and singletons); retrofitting your code 
after the fact versus being able to shape the code as you go is going to be a 
serious PITA, so i'd suggest that the first we do is adjust the workflow here.

so could you please commit your code into a branch in KDE's svn? that way 
there's a hope of tracking what's going on and getting involved. 
/branches/work/plasma-groupingtaskbar or something like that would be great.

> Because we have a Task class in workspace/libs/taskmanager/ i thought it
> would be nice to have also a non GUI  group there. So the TaskGroupItems
> follow the groups they are associated to. This has the advantage that we

ok, let's just start with libtaskmanager and take those items one at a time. 
in particular, we have AbstractGroupableItem, TaskGroup and 
BasicGroupingStrategy. 

i see changes needed elsewhere as well (and already see 
TaskManager::getRootGroup that should be just rootGroup and is going to cause 
problems since TaskManager is a singleton, and the root group will not be), 
but let's work out these couple of classes first =)

some notes:

 * none of the classes have a d pointer. this makes it impossible to maintain 
binary compatibility in the library. all member variables need to be moved 
into a private class

* these classes are already taking quite a bit of shape but there are zero 
apidox comments in the headers.

* the singleton for the BasicGroupingStrategy should be using K_GLOBAL_STATIC. 
see kdebase/workspace/libs/plasma/theme.cpp for an example of how to use that.

* methods like nameRemoved(QString name) should actually be 
nameRemoved(const QString &name). anything non-POD should be like this (with 
some exceptions, of course)

* do not prefix getters with "get". AbstractGroupableItem::getParentGroup is 
just AbstractGroupableItem::parentGroup

* there are some stray ';'s in the headers (e.g. virtual bool isGroupItem() 
const{return true;};) once in svn, those can be cleaned up pretty easily by 
people doing reviews such as myself.


and onto the design questions:

AbstractGroupableItem

* can you explain the strategy behind the slots? it seems a bit of a jumble

* hasParentGroup seems gratuitous if one can just do parentGroup() != 0?

* what is a "groupingLevel"?

* what is the preferred insert index based on?

* typedef QSet<AbstractGroupableItem*> ItemList; is going to give you 
problems. Task is a shared class, and this is purposeful. it also why we have 
TaskPtr. using this strategy of a naked AbstractGroupableItem* you will end up 
running into crashes and various race conditions. these points *must* be 
shared and guarded.

* joinParent is misnamed. it's actually joinGrandparent, or some such. i'd 
actually remove this method entirely.


BasicGroupingStrategy:

* since it's meant to be subclasses, do we need the word "Basic" in it? it 
actually doesn't seem to do any grouping, actually, but i assume that's due to 
the stage of devel it's in... if it's meant to be a shell of a class that is 
meant to be subclassed (i see virtuals =) then it could be 
AbstractGroupingStrategy and the virtuals made pure virtual. personally, i'd 
just call it GroupingStrategy

* in the taskbar we're usually getting one task at a time from the window 
manager (as windows are added/removed). i don't see any API that easily maps 
an item to an existing group. that seems odd. where does the "which group 
should this task be put in?" code currently exist?

* what is the purpose of having separate suggesting methods for names, colours 
and icons, other than to go through the list 3 times? i can't think of any 
situation where you'd only want the names or only the colours, or in which 
having the other information would be detrimental. on the flip side, going 
through the list N times is a bit wasteful. not to mention it complicates the 
API =)

* why is it a singleton when there can be N users of a strategy? why is it a 
singleton if it is meant to be subclassed? this really doesn't look like the 
place for a singleton, tbh.

* where will ordering of items be handled? you seem to be assuming a manual 
ordering of things always and that's not what most people want: they want an 
automated ordering (alpha, by desktop, etc). i'm not sure it belongs in 
TaskGroup because all task groups in a given collection should behave 
similarly and alternative strategies would require either putting all possible 
strategies into TaskGroup or making it subclassable. (neither are pretty)

* there's no API even stubbed in for configuration management.


TaskGroup:

* i don't understand the relationship between the grouping strategy and the 
group class. if you have N tasks divided into M groups, what decides what task 
belongs in each group? right now it seems the strategy suggests where they 
should go, but the group decides. what happens in the case of multiple groups 
when a new task comes around?

the way i might go about it is have the strategy manage a collection of groups 
that it then exports. this would be a tree of objects representing the groups 
and their nesting. groups would contain items and be completely stupid about 
the grouping mechanics. that way the relationship between group and strategy 
is obvious, it's clear how multiple strategies would work in parallel, and 
give a sane point for configuration management

* there are gui concepts mixed into this data class. collapsed/not collapsed 
is not a data concept, it's a visualization concept. all of that logic belongs 
in the UI side.


a few implementation comments:

TaskGroup:

* don't use KIconLoader directly and then create a QIcon out of it. use KIcon 
just as you would QIcon and let it do the KIconLoader stuff internally

* add(dynamic_cast<AbstractGroupableItem*>(task.data())) -> *never* take the 
data of a TaskPtr and store it somewhere. that defeats the purpose of TaskPtr 
altogether. moreover, you don't need to cast a subclass to a superclass. 
dynamic_cast is for downcasting. in fact, i don't actually see the point of 
the various add() method overrides (for TaskPtr, TaskGroup, etc). they don't 
actually do anything useful.

* ditto for the remove() methods

* clearGroup could be much more easily written:

TaskGroup *p = parentGroup();
foreach (AbstractGroupableItem *item, m_members) {
    item->setParent(p);
}
m_members.clear();

> To Group Items just drop them on each other with the groupModifierKey (Alt
> is hardcoded right now) to group them. The group gets a name and color
> created by the BasicGroupingStrategy class which just creates numbered
> names and random colors.

is there a non-manual grouping system in place? it would probably make sense 
to implement an automated grouping system (can be really basic, based on the 
application name of the window) to stress the API in a different and useful 
way.

> -Save custom order on collapse group

custom order? you mean the order someone has dragged the item into? this is 
not going to work with automated systems, or at leat make them very 
complicated, or require being able to switch 

> -It should be possible to have multiple rows and force a certain number of
> rows or let the LayoutWidget automatically appen and remove rows as needed.
> -Add Group editing window where name and color can be edited (with support
> of automatic guesses). It could look like the window that pops up when
> clock or the new device notifier is clicked.

beware of "a million dialogs" syndrome

> -Find a way to recognize a window over different sessions

window class and window role properties, most likely.

> -Implement Dragging of expanded groups by dragging groupmember outside of
> group

unless the implementation is more straightforward than the description, this 
could be a problematic feature.

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/panel-devel/attachments/20080703/62f041e3/attachment-0001.pgp