Review Request: Launcher support for libtaskmanager

[Aaron Seigo]

> On 2010-07-13 20:53:40, Aaron Seigo wrote:
> > /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp, lines 264-269
> > <http://reviewboard.kde.org/r/4585/diff/3/?file=30674#file30674line264>
> >
> >     why are launchers with no window instances removed from the member list?
> 
> Anton Kreuzkamp wrote:
>     isVisible() returns true if there's no window instance, so it get removed if there is one.
> 
> Aaron Seigo wrote:
>     ah! ok, that makes sense ... very unfortunate that this needs to be done on each request for members(), however, as that gets called fairly often. it would be a lot nicer if the launcher was only added (pre-pended?) to the list if there were no windows (and so it should be shown). this could be achieved by checking isVisible on the launcher item after calls to addWindowInstance and removeWindowInstance; a separate list/set of all launchers would need to be kept in this case (for the ones that are !isVisibe and therefore no longer in the main list), but this should prevent a lot of iteration, temporary list creation, casting and comparisons whenever members() is called.
>     
>     (another more complex approach might be to make LauncherItem change its type: when there are no associated windows it is a launcher item; when there is one window, it is a window item; where there is >1 window, it is a group item. this, however, is signficantly more complex and not at all worth it as far as i can see.) 
>     
>     (this also implies that once the app is launched, it doesn't make sense to launch it again (which isn't always the case), but that's probably ignorable.)
> 
> Anton Kreuzkamp wrote:
>     If I didn't missunderstood the code (please correct me if I did) it this only gets called if the item wasn't there before (in addTask(); removeTask() only gets called if the item really should be removed ) because of this is checked before. ("if (!item) {",l.269)

yes, that's correct. however, that is enough to know when a LauncherItem should or should not be shown. some possible aproaches:

* LauncherItem could emit a signal when the count drops to zero or becomes non-zero, allowing the TaskGroup to connect to this signal and add/remove it from the list of items only when it changes

* in LauncherItem::addWindowInstance and LauncherItem::removeWindowInstance, something like this:

if (parentGroup() {
    parentGroup()->launcherStatusChanged(this);
}

a matter of taste which is more pleasant. personally, i'd go for the latter; the signal would be "cleaner" if it weren't for the fact that managing the connections and disconnections would be required and the current code probably doesn't lend itself to that.


- Aaron


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4585/#review6545
-----------------------------------------------------------


On 2010-08-22 10:39:29, Anton Kreuzkamp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4585/
> -----------------------------------------------------------
> 
> (Updated 2010-08-22 10:39:29)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Adds support for Windows 7 like launchers in libtaskmanager.
> (I'm on holliday from 12th July until 1st August so I will not be able to reply during this time.)
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/CMakeLists.txt 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.h 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/abstractsortingstrategy.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.h PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.cpp PRE-CREATION 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/alphasortingstrategy.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/kustodiangroupingstrategy.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/manualgroupingstrategy.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/programgroupingstrategy.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp 1148442 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.h 1166310 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp 1166310 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.h 1166310 
>   /trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.cpp 1148442 
> 
> Diff: http://reviewboard.kde.org/r/4585/diff
> 
> 
> Testing
> -------
> 
> Tested with a small test-applett and everything works.
> 
> 
> Thanks,
> 
> Anton
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20100823/7201168c/attachment.htm