Review Request: Launchersupport in libtaskmanager - final implementation

[Aaron Seigo]

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



trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h
<http://svn.reviewboard.kde.org/r/5785/#comment8907>

    with these changes, GroupManager gets a lot of API for launchers. similar API does not exist for windows, startups or groups, though. this seems like the "wrong" place for such API. why is it added here?
    
    if anything, it may make more sense to move addLauncher from GroupManager to TaskGroup. this will allow GroupManager to remain a manager of groups and TaskGroup can continue to manage collections of items.



trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h
<http://svn.reviewboard.kde.org/r/5785/#comment8906>

    hasFittingLauncher -> hasMatchingLauncher? or better yet, perhaps:
    
    LauncherItem *findLauncher(const AbstractGroupableItem *item) const;



trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h
<http://svn.reviewboard.kde.org/r/5785/#comment8916>

    are these signals ever used at all? i don't see them used here or in the tasks plasmoid patch. if they aren't, they should be dropped until there is a use case for them.



trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp
<http://svn.reviewboard.kde.org/r/5785/#comment8909>

    odd that is returns NULL even though a launcher exists for it?
    
    it also "leaks" the launcher created above. instead, i might write it like this:
    
    LauncherItem *launcher = findLauncher(launcher);
    if (!launcher) {
        launcher = new LauncherItem ...
    }
    



trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp
<http://svn.reviewboard.kde.org/r/5785/#comment8915>

    as this is only used from the context menu action, this should be moved there. the actions are not public API while this is.



trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp
<http://svn.reviewboard.kde.org/r/5785/#comment8910>

    whitespace



trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp
<http://svn.reviewboard.kde.org/r/5785/#comment8913>

    why "Pin Task"? that doesn't really tell me what it does at all. what it really does is create a launcher for the item.
    
    this seems like a very odd place to put the feature. i'd actually expect the ability to add a launcher using drag and drop from, e.g. a launcher menu or even through a UI similar to the quicklaunch applet.



trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp
<http://svn.reviewboard.kde.org/r/5785/#comment8914>

    the fact that his can fail hints that this is a feature in the wrong place.
    
    if it is decided that this context menu should be kept (and i'm not yet convinced it should be), it should _only_ be shown for entries for which a launcher can be created in the first place.


- Aaron


On 2010-11-07 13:19:14, Anton Kreuzkamp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5785/
> -----------------------------------------------------------
> 
> (Updated 2010-11-07 13:19:14)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> This is the final implementation of the launchersupport for libtaskmanager. Many parts of the initial implementation has been changed but now everything works as it should.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.h 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupableitem.cpp 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/abstractgroupingstrategy.h 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/abstractsortingstrategy.cpp 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.h 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/launcheritem.cpp 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/alphasortingstrategy.cpp 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/strategies/desktopsortingstrategy.cpp 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.h 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions_p.h 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.h 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/taskgroup.cpp 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/taskitem.h 1193833 
>   trunk/KDE/kdebase/workspace/libs/taskmanager/taskmanager.h 1193833 
> 
> Diff: http://svn.reviewboard.kde.org/r/5785/diff
> 
> 
> Testing
> -------
> 
> Tested and everything worked fine.
> 
> 
> Thanks,
> 
> Anton
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20101107/081056a7/attachment-0001.htm