Review Request: TaskManager: Store launcher order

[Aaron J. Seigo]

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


usual comment about whitespace correctness; otherwise a few comments inline ... some really nice fixes and improvements in this one. a little bit of cleanup and it should be ready to go.


libs/taskmanager/groupmanager.cpp
<http://git.reviewboard.kde.org/r/103006/#comment6715>

    instead of having the launchers in two lists, if ordering matters (and i agree it should), then perhaps turn the QHash of launchers into a QList<LauncherItem *> and keep the ordering there.
    
    finding by URL (which happens in two places) then becomes:
    
    foreach (const LauncherItem *item, launchers) {
        if (item->url() == url) {
             ... found ...
        }
    }
    
    the # of launchers will remain small so the overhead of this will be negligable and it will save both on allocations and having to keep two collections in sync with each other.
    
    this could even be turned into a small helper method in the private class: int indexOfLauncher(const KUrl &url) const;



libs/taskmanager/strategies/alphasortingstrategy.cpp
<http://git.reviewboard.kde.org/r/103006/#comment6722>

    might be more readable as an if/else given the length of the items in the ternary...



libs/taskmanager/strategies/alphasortingstrategy.cpp
<http://git.reviewboard.kde.org/r/103006/#comment6723>

    this makes the speed of taskName() even more critical as this will get called often



libs/taskmanager/strategies/desktopsortingstrategy.cpp
<http://git.reviewboard.kde.org/r/103006/#comment6726>

    this should be a private member of the class ... as this is a non-exported class, binary compatibility doesn't matter.



libs/taskmanager/strategies/programgroupingstrategy.cpp
<http://git.reviewboard.kde.org/r/103006/#comment6729>

    this check looks inccorect: if launchers are integrated or separate, it should still be possible to say that a given application should not be groupable with other windows from that same application.



libs/taskmanager/taskitem.cpp
<http://git.reviewboard.kde.org/r/103006/#comment6720>

    have you done any time measurements on how long this method takes? it will be called potentially quite a few times when a launcher is started ...


- Aaron J. Seigo


On Oct. 31, 2011, 8:42 p.m., Craig Drummond wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103006/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2011, 8:42 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Description
> -------
> 
> 1. Store the order in which launchers are created
> 2. Add a new config option, separateLaunchers - so that applet can decide if launchers and tasks should be kept separate.
> 
> (Part of IconTasks' taskmanager changes)
> 
> 
> Diffs
> -----
> 
>   libs/taskmanager/abstractgroupingstrategy.cpp 4ed424a 
>   libs/taskmanager/abstractsortingstrategy.cpp 390f6f0 
>   libs/taskmanager/groupmanager.h acaa142 
>   libs/taskmanager/groupmanager.cpp 6e7ffa7 
>   libs/taskmanager/launcheritem.cpp 20f0e7c 
>   libs/taskmanager/strategies/alphasortingstrategy.cpp 9ec1aca 
>   libs/taskmanager/strategies/desktopsortingstrategy.cpp 520fead 
>   libs/taskmanager/strategies/manualsortingstrategy.h 113faab 
>   libs/taskmanager/strategies/manualsortingstrategy.cpp 4409a6b 
>   libs/taskmanager/strategies/programgroupingstrategy.cpp 5c43d03 
>   libs/taskmanager/taskgroup.h 53c2871 
>   libs/taskmanager/taskgroup.cpp 49140ae 
>   libs/taskmanager/taskitem.h 5de8478 
>   libs/taskmanager/taskitem.cpp 0a768e5 
> 
> Diff: http://git.reviewboard.kde.org/r/103006/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Craig Drummond
> 
>

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