-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117813/#review56724
-----------------------------------------------------------



applets/systemtray/plugin/host.cpp
<https://git.reviewboard.kde.org/r/117813/#comment39587>

    Looks like it can all be removed.



applets/systemtray/plugin/host.cpp
<https://git.reviewboard.kde.org/r/117813/#comment39588>

    just init(); ?
    I don't see a reason why you need a single shot timer here.



applets/systemtray/plugin/protocols/plasmoid/plasmoidtask.cpp
<https://git.reviewboard.kde.org/r/117813/#comment39599>

    Ahh, this is tricky to follow. You are invoking the init function here 
_and_ earlier on using a singleShot timer if this translates to the Host::init 
method. If that's the case then i think you might be safe in removing this line 
and the singleshot and just call init() from the constructor.



applets/systemtray/plugin/tasklistmodel.h
<https://git.reviewboard.kde.org/r/117813/#comment39596>

    Not needed, the model will notify QML when the row count changes.



applets/systemtray/plugin/tasklistmodel.cpp
<https://git.reviewboard.kde.org/r/117813/#comment39593>

    Perhaps try do these test before calling this function via qLowerBound.
    
    It would certainly be faster, but also more complicated so i don't know if 
it's worth it.



applets/systemtray/plugin/tasklistmodel.cpp
<https://git.reviewboard.kde.org/r/117813/#comment39594>

    I don't think you plan on changing this at runtime - ever. You might want 
to change this to a const and use initializer lists (a C++11 feature). It works 
like this:
    
    const QHash<int, QByteArray> roleNames { {Qt::UserRole, "name"}, ..., ...};



applets/systemtray/plugin/tasklistmodel.cpp
<https://git.reviewboard.kde.org/r/117813/#comment39595>

    Are you sure this is ok?
    For example, if you add one row (and have none) this should translate to:
    beginInsertRows(QModelIndex(), 0, 1);
    
    I think you need something like:
    int startCount = (rowCount() - 1 >= 0) ? rowCount() : 0;
    beginInsertRows(QModelIndex(), startCount, startCount + 1); 



applets/systemtray/plugin/tasklistmodel.cpp
<https://git.reviewboard.kde.org/r/117813/#comment39597>

    Remove, the model will notify QML. No need to do this yourself.



applets/systemtray/plugin/tasklistmodel.cpp
<https://git.reviewboard.kde.org/r/117813/#comment39598>

    Remove, the model will notify QML. No need to do this yourself.


- Mark Gaiser


On April 27, 2014, 10:51 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117813/
> -----------------------------------------------------------
> 
> (Updated April 27, 2014, 10:51 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> Port QQmlListProperty to QAbstractListModel.
> QQmlListProperty only has a signal that the list has changed.This means when 
> used in a ListView every delegate has to be redone whenever a single item is 
> inserted or removed rather than just moved.
> 
> Given TaskDelegate is not the simplest of things this has a performance gain, 
> most noticeably on startup. Also rather than sorting all items after an 
> insert items are inserted in the right place using qLowerBound. Now we have 
> the correct signals we can remove the compression, they won't add anything. 
> 
> 
> Other commits:
> 
> Avoid constructing a QString for comparing, use QLatin1String for == 
> operators.
> 
> Remove useless include
> 
> Do not construct a map inside a lessThan function
> 
> lessThan functions have to be fast.
> Also Map -> Hash as we're not using order here.
> 
> 
> Diffs
> -----
> 
>   applets/systemtray/package/contents/ui/ExpandedRepresentation.qml 2ef180b 
>   applets/systemtray/package/contents/ui/PlasmoidItem.qml 0eb1687 
>   applets/systemtray/package/contents/ui/StatusNotifierItem.qml fc889a8 
>   applets/systemtray/package/contents/ui/TaskDelegate.qml 913d8f1 
>   applets/systemtray/package/contents/ui/TaskListDelegate.qml 5501e02 
>   applets/systemtray/plugin/CMakeLists.txt f6e23b4 
>   applets/systemtray/plugin/host.h 02c5bbe 
>   applets/systemtray/plugin/host.cpp eafd0b6 
>   applets/systemtray/plugin/protocols/plasmoid/plasmoidtask.cpp 2b846f2 
>   applets/systemtray/plugin/tasklistmodel.h PRE-CREATION 
>   applets/systemtray/plugin/tasklistmodel.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/117813/diff/
> 
> 
> Testing
> -------
> 
> Seems to work :)
> 
> see branch davidedmundson/faster_systray to test
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel

Reply via email to