Re: Review Request 117813: Make the system tray faster
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/ --- (Updated May 6, 2014, 11:03 a.m.) Status -- This change has been marked as submitted. 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/CompactRepresentation.qml 01308e7 applets/systemtray/package/contents/ui/ExpandedRepresentation.qml 646b908 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/package/contents/ui/main.qml d1a6851 applets/systemtray/plugin/CMakeLists.txt f6e23b4 applets/systemtray/plugin/host.h 02c5bbe applets/systemtray/plugin/host.cpp eafd0b6 applets/systemtray/plugin/task.h 68dcd12 applets/systemtray/plugin/task.cpp 1f8e3ca 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
Re: Review Request 117813: Make the system tray faster
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/#review57400 --- This review has been submitted with commit 45eaffacab8f4badf27d82ec8134d51f748f375f by David Edmundson to branch master. - Commit Hook On May 2, 2014, 4:10 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/ --- (Updated May 2, 2014, 4:10 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/CompactRepresentation.qml 01308e7 applets/systemtray/package/contents/ui/ExpandedRepresentation.qml 646b908 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/package/contents/ui/main.qml d1a6851 applets/systemtray/plugin/CMakeLists.txt f6e23b4 applets/systemtray/plugin/host.h 02c5bbe applets/systemtray/plugin/host.cpp eafd0b6 applets/systemtray/plugin/task.h 68dcd12 applets/systemtray/plugin/task.cpp 1f8e3ca 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
Re: Review Request 117813: Make the system tray faster
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/#review57296 --- Ship it! applets/systemtray/package/contents/ui/StatusNotifierItem.qml https://git.reviewboard.kde.org/r/117813/#comment39914 This could be removed, sebas said applets/systemtray/package/contents/ui/TaskDelegate.qml https://git.reviewboard.kde.org/r/117813/#comment39915 This is used (remove the fixme) applets/systemtray/plugin/host.cpp https://git.reviewboard.kde.org/r/117813/#comment39916 Remove this please, it adds nothing useful to the output - Martin Klapetek On May 2, 2014, 6:10 p.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/ --- (Updated May 2, 2014, 6:10 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/CompactRepresentation.qml 01308e7 applets/systemtray/package/contents/ui/ExpandedRepresentation.qml 646b908 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/package/contents/ui/main.qml d1a6851 applets/systemtray/plugin/CMakeLists.txt f6e23b4 applets/systemtray/plugin/host.h 02c5bbe applets/systemtray/plugin/host.cpp eafd0b6 applets/systemtray/plugin/task.h 68dcd12 applets/systemtray/plugin/task.cpp 1f8e3ca 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
Re: Review Request 117813: Make the system tray faster
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/#review57097 --- what's the status of this? - Marco Martin 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
Re: Review Request 117813: Make the system tray faster
On May 2, 2014, 8:39 a.m., Marco Martin wrote: what's the status of this? I have a crash when combined with Martin's new notifications. Fixing first. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/#review57097 --- 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
Re: Review Request 117813: Make the system tray faster
On May 2, 2014, 10:39 a.m., Marco Martin wrote: what's the status of this? David Edmundson wrote: I have a crash when combined with Martin's new notifications. Fixing first. Speaking of which https://git.reviewboard.kde.org/r/117903/ ;) - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/#review57097 --- On April 28, 2014, 12:51 a.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/ --- (Updated April 28, 2014, 12:51 a.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
Re: Review Request 117813: Make the system tray faster
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/ --- (Updated May 2, 2014, 4:10 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 (updated) - applets/systemtray/package/contents/ui/CompactRepresentation.qml 01308e7 applets/systemtray/package/contents/ui/ExpandedRepresentation.qml 646b908 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/package/contents/ui/main.qml d1a6851 applets/systemtray/plugin/CMakeLists.txt f6e23b4 applets/systemtray/plugin/host.h 02c5bbe applets/systemtray/plugin/host.cpp eafd0b6 applets/systemtray/plugin/task.h 68dcd12 applets/systemtray/plugin/task.cpp 1f8e3ca 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
Re: Review Request 117813: Make the system tray faster
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/#review56718 --- All in all, looks like a good improvement to me. applets/systemtray/package/contents/ui/TaskDelegate.qml https://git.reviewboard.kde.org/r/117813/#comment39576 ! applets/systemtray/package/contents/ui/TaskDelegate.qml https://git.reviewboard.kde.org/r/117813/#comment39575 ! - Aleix Pol Gonzalez 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
Re: Review Request 117813: Make the system tray faster
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/#review56714 --- Overall +1 from me applets/systemtray/package/contents/ui/TaskDelegate.qml https://git.reviewboard.kde.org/r/117813/#comment39564 No. applets/systemtray/plugin/host.cpp https://git.reviewboard.kde.org/r/117813/#comment39565 This reminds me - didn't we decide at the Jan sprint that the popup (the hidden here?) should contain /all/ the systray plasmoids? Also sorry for hijacking the review, we could just fix it at once? applets/systemtray/plugin/protocols/plasmoid/plasmoidtask.cpp https://git.reviewboard.kde.org/r/117813/#comment39571 Whitespace leftover it seems applets/systemtray/plugin/tasklistmodel.h https://git.reviewboard.kde.org/r/117813/#comment39566 remove ;) applets/systemtray/plugin/tasklistmodel.cpp https://git.reviewboard.kde.org/r/117813/#comment39568 const QModelIndex index - const QModelIndex index applets/systemtray/plugin/tasklistmodel.cpp https://git.reviewboard.kde.org/r/117813/#comment39569 const QModelIndex parent - const QModelIndex parent also Q_UNUSED(parent) to spare compiler warning applets/systemtray/plugin/tasklistmodel.cpp https://git.reviewboard.kde.org/r/117813/#comment39577 Task* task - Task *task applets/systemtray/plugin/tasklistmodel.cpp https://git.reviewboard.kde.org/r/117813/#comment39578 Task* task - Task *task - Martin Klapetek On April 28, 2014, 12:51 a.m., David Edmundson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/ --- (Updated April 28, 2014, 12:51 a.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
Re: Review Request 117813: Make the system tray faster
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/#review56721 --- very good direction, just a couple of minor issues applets/systemtray/plugin/host.cpp https://git.reviewboard.kde.org/r/117813/#comment39583 did you try disabling and then reenabling a category? (didn't try the patch, just not sure looking at it how correctly reenables stuff that doesn't keep track of?) applets/systemtray/plugin/tasklistmodel.h https://git.reviewboard.kde.org/r/117813/#comment39581 as a convention, models exported in qml export their size as count (just because the primitive ListModel calls it that way) - Marco Martin 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
Re: Review Request 117813: Make the system tray faster
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/117813/#review56719 --- I like muchos. Haven't tested it yet, as I'm still resurrecting my builds, so I'm ok with shipping it, but don't feel comfortable ship iting it myself. :) Good work! applets/systemtray/package/contents/ui/StatusNotifierItem.qml https://git.reviewboard.kde.org/r/117813/#comment39579 The answer is easy: It also wastes memory! ;-) More seriously, I think it's a leftover from the old implementation and can be removed throughout. applets/systemtray/package/contents/ui/TaskDelegate.qml https://git.reviewboard.kde.org/r/117813/#comment39582 It is used to determine whether we're a delegate in the hidden section (so with text), or in the panel section (square). applets/systemtray/plugin/host.cpp https://git.reviewboard.kde.org/r/117813/#comment39584 no spaces around Task applets/systemtray/plugin/tasklistmodel.h https://git.reviewboard.kde.org/r/117813/#comment39585 a model representing items for the system tray? applets/systemtray/plugin/tasklistmodel.cpp https://git.reviewboard.kde.org/r/117813/#comment39586 see above - Sebastian Kügler 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
Re: Review Request 117813: Make the system tray faster
---
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 QHashint, 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
Re: Review Request 117813: Make the system tray faster
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. 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. 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); beginInsertRows(QModelIndex(), 0, 1) is saying you are inserting 2 rows, starting at 0 ending at 1. It's an annoying QAbstractItemModel API. - David --- 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 117813: Make the system tray faster
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. 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 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. 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) 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. the model does, but the exported count property needs to be notified by hand - Marco --- 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 ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request 117813: Make the system tray faster
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 -
