Review Request 117813: Make the system tray faster
Mark Gaiser
markg85 at gmail.com
Mon Apr 28 13:06:48 UTC 2014
> On April 28, 2014, 11:06 a.m., Mark Gaiser wrote:
> > applets/systemtray/plugin/protocols/plasmoid/plasmoidtask.cpp, lines 61-63
> > <https://git.reviewboard.kde.org/r/117813/diff/1/?file=268852#file268852line61>
> >
> > 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.
>
> Marco Martin wrote:
> no, it isn't init of Host.
> this is the init() of m_taskGraphicsObject, ie the init() of the AppletInterface of the loaded plasmoid (is something that shouldn't even be neded in theory, but preloading the applets seems to avoid some crash scenarios)
Right, now it makes sense. Thank you for the clarification.
> On April 28, 2014, 11:06 a.m., Mark Gaiser wrote:
> > applets/systemtray/plugin/host.cpp, line 120
> > <https://git.reviewboard.kde.org/r/117813/diff/1/?file=268851#file268851line120>
> >
> > just init(); ?
> > I don't see a reason why you need a single shot timer here.
>
> Marco Martin wrote:
> this causes the tasks to be loaded after the applet is done, in turn, making the whole workspace ui to load faster (and task icons appearing after when the rest is done) is a little hack that does huge difference in perceived startup speed
Sounds like a good (undocumented) reason :)
> On April 28, 2014, 11:06 a.m., Mark Gaiser wrote:
> > applets/systemtray/plugin/tasklistmodel.cpp, line 99
> > <https://git.reviewboard.kde.org/r/117813/diff/1/?file=268854#file268854line99>
> >
> > 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);
>
> David Edmundson wrote:
> beginInsertRows(QModelIndex(), 0, 1)
>
> is saying you are inserting 2 rows, starting at 0 ending at 1. It's an annoying QAbstractItemModel API.
Ok, makes sense now. I'm always confused by the start and end numbers.
> On April 28, 2014, 11:06 a.m., Mark Gaiser wrote:
> > applets/systemtray/plugin/tasklistmodel.cpp, line 103
> > <https://git.reviewboard.kde.org/r/117813/diff/1/?file=268854#file268854line103>
> >
> > Remove, the model will notify QML. No need to do this yourself.
>
> David Edmundson wrote:
> I have it as a property, so I need to tell Qt the property has changed.
>
> I could do connect(this, SIGNAL("rowsInserted(...), this, SIGNAL(rowCountChanged());
> I could do connect(this, SIGNAL("rowsRemoved(...), this, SIGNAL(rowCountChanged());
> + rowsMoved + layoutChanged + modelReset..
> so I deemed this easier.
As explained in person. The view will know this so the line can be removed.
> On April 28, 2014, 11:06 a.m., Mark Gaiser wrote:
> > applets/systemtray/plugin/tasklistmodel.cpp, line 115
> > <https://git.reviewboard.kde.org/r/117813/diff/1/?file=268854#file268854line115>
> >
> > Remove, the model will notify QML. No need to do this yourself.
>
> Marco Martin wrote:
> the model does, but the exported count property needs to be notified by hand
If you keep the property, yes. But there is no need for the property to exist at all because the view knows the model count.
- Mark
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117813/#review56724
-----------------------------------------------------------
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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140428/0fcf6d16/attachment-0001.html>
More information about the Plasma-devel
mailing list