Review Request: Launchersupport in libtaskmanager - final implementation

Anton Kreuzkamp akreuzkamp at web.de
Mon Nov 8 16:34:13 CET 2010 


> On 2010-11-07 18:07:13, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h, lines 129-147
> > <http://svn.reviewboard.kde.org/r/5785/diff/1/?file=40714#file40714line129>
> >
> >     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.

It is possible that you have more than one root group (for every desktop there is one root group), but launchers need to be global, so you can't put in in TaskGroup. (I just see that the launcher gets added to just one rootgroup although, I'll change that)


> On 2010-11-07 18:07:13, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h, line 133
> > <http://svn.reviewboard.kde.org/r/5785/diff/1/?file=40714#file40714line133>
> >
> >     hasFittingLauncher -> hasMatchingLauncher? or better yet, perhaps:
> >     
> >     LauncherItem *findLauncher(const AbstractGroupableItem *item) const;

Matching was the word I searched for....
but the second possibility is really better together with the changes below


> On 2010-11-07 18:07:13, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp, line 526
> > <http://svn.reviewboard.kde.org/r/5785/diff/1/?file=40715#file40715line526>
> >
> >     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 ...
> >     }
> >

Why didn't I think of this myself? Of course, I'll change that.


> On 2010-11-07 18:07:13, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.h, lines 138-141
> > <http://svn.reviewboard.kde.org/r/5785/diff/1/?file=40714#file40714line138>
> >
> >     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.

They're used in the taskapplet to add/delete the launchers in the config.
(tasks.cpp l.118-119)


> On 2010-11-07 18:07:13, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/libs/taskmanager/groupmanager.cpp, line 607
> > <http://svn.reviewboard.kde.org/r/5785/diff/1/?file=40715#file40715line607>
> >
> >     as this is only used from the context menu action, this should be moved there. the actions are not public API while this is.

together with findLauncher() this is possible now, I'll change it.


> On 2010-11-07 18:07:13, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp, lines 305-312
> > <http://svn.reviewboard.kde.org/r/5785/diff/1/?file=40721#file40721line305>
> >
> >     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.

I couldn't find a good term for it, so I just used the term windows uses. (Yeah, I know shame on me for that)
I'm thankful for every idea for a good term.

I've asked wether to make the configuration per contextmenu or per configuration UI in #plasma some time ago, and Marco told me to make it as contextmenu.
(Beside the fact that windows does it like this, but that's not a reason)


> On 2010-11-07 18:07:13, Aaron Seigo wrote:
> > trunk/KDE/kdebase/workspace/libs/taskmanager/taskactions.cpp, line 316
> > <http://svn.reviewboard.kde.org/r/5785/diff/1/?file=40721#file40721line316>
> >
> >     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.

I thought of it but I didn't know how to check wether it is possible.
If you tell me how to do it, I will change it, of course.


- Anton


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


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/20101108/987246df/attachment-0001.htm

More information about the Plasma-devel mailing list