Review Request: TaskManager: TaskAction changes from IconTasks

Aaron J. Seigo aseigo at kde.org
Tue Nov 1 14:05:08 UTC 2011


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


a few issues with this patch, which can be divided into three types:

* over simplification of the menu; i am all for reducing the items in this menu, including moving more to the "Advanced" submenu (which should probably be renamed to "More Actions"), but this patch goes too far and renders the BasicMenu far, far les useful than it ought to be
* custom properties in the QActions are unneeded (see inline comments for how these can be handled without custom properties)
* whitespace correctness (the most trivial and least important of the issues)


libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6689>

    please watch whitespace usage. this should be:
    
    if (QEvent:: ..) {
    
    also, no extra space at the end of lines, please.
    
    this needs to be addressed throughout the patch. i know it's finicky work, but it pays off with code readability at the end.



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6688>

    why a property for this? why not simply:
    
    if (!act->toolTip().isEmpty()) {



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6690>

    we have a name, it should be used. it hardly matters what the eventual Launcher is called. shortening it with the removal of "It Is" is probably alright ("computer application english" :)



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6693>

    how well does this work with KUniqueApplications?



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6692>

    whitespace around ==



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6691>

    should be on same line



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6694>

    ws



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6695>

    ws



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6696>

    there's a line between "simplifiying" and "making it useless in too many cases" that this crosses imho. this would have me reaching for the window titlebar in too many cases.
    
    i would be in favour of renaming "Advanced" (a word we are trying to get away from) with something like "More actions"



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6698>

    with this change one has to open the goup, right click on the correct entry and then choose. i don't see the point of this simlification other than to make it harder to get at.



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6699>

    no, this is one of the PRIMARY uses of the context menu. what would be nice is to move "To Current Desktop" into the DesktopsMenu, however, right above "All Desktops"



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6700>

    could be moved into the "Advanced"(/"More Actions") menu?



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6701>

    again, this is just crippling the menu, not making it "simpler"



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6702>

    you should be able to just  menu->addAction(action) for the separator case?



libs/taskmanager/taskactions.cpp
<http://git.reviewboard.kde.org/r/103004/#comment6703>

    this is a misuse of QAction. you can add QActions to a QAction and that parent QAction becomes the submenu. no need for such custom property setting/reading and menu creation.


- Aaron J. Seigo


On Oct. 31, 2011, 8:40 p.m., Craig Drummond wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103004/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2011, 8:40 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> -------
> 
> 1. Add NewInstance action to launch a new instance of an application.
> 2. Move toggle launcher action out of advanced menu.
> 3. Don't show application name in 'Show A Launcher' action, as might not know the real name at this time (e.g. no desktop file read).
> 4. Add application actions, to be shown at the top of the right-click menu. IconTasks uses this to show recent docuents, dock manager menu items, and unity items.
> 5. A ToolTipMenu class is created, so that tooltips may be give for menu items. This is so that IconTasks can display the full path of a documents in the recent documents menu.
> 6. Add option to have only basic window controls in menu.
> 
> 
> Diffs
> -----
> 
>   libs/taskmanager/taskactions.h 2b5a641 
>   libs/taskmanager/taskactions.cpp 0e6ba8e 
>   libs/taskmanager/taskactions_p.h 913966c 
> 
> Diff: http://git.reviewboard.kde.org/r/103004/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Craig Drummond
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20111101/9f5bcdd9/attachment-0001.html>


More information about the Plasma-devel mailing list