D27088: [applets/SystemTray] Implement sorting in the model
This revision was automatically updated to reflect the committed changes. Closed by commit R120:6d86c690d133: [applets/SystemTray] Implement sorting in the model (authored by kmaterka). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27088?vs=76026=76027 REVISION DETAIL https://phabricator.kde.org/D27088 AFFECTED FILES applets/systemtray/CMakeLists.txt applets/systemtray/package/contents/ui/ConfigEntries.qml applets/systemtray/sortedsystemtraymodel.cpp applets/systemtray/sortedsystemtraymodel.h applets/systemtray/systemtray.cpp applets/systemtray/systemtray.h applets/systemtray/systemtraymodel.cpp applets/systemtray/systemtraymodel.h To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik Cc: mart, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra
D27088: [applets/SystemTray] Implement sorting in the model
kmaterka updated this revision to Diff 76026. kmaterka added a comment. Review fixes REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27088?vs=75112=76026 BRANCH master REVISION DETAIL https://phabricator.kde.org/D27088 AFFECTED FILES applets/systemtray/CMakeLists.txt applets/systemtray/package/contents/ui/ConfigEntries.qml applets/systemtray/sortedsystemtraymodel.cpp applets/systemtray/sortedsystemtraymodel.h applets/systemtray/systemtray.cpp applets/systemtray/systemtray.h applets/systemtray/systemtraymodel.cpp applets/systemtray/systemtraymodel.h To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik Cc: mart, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra
D27088: [applets/SystemTray] Implement sorting in the model
kmaterka added inline comments. INLINE COMMENTS > davidedmundson wrote in sortedsystemtraymodel.cpp:47 > I think calling QSortFilterProxyModel::lessThan(left, right); would do the > same > > then you don't need compareDisplayAlphabetically > > your implementation looks fine though, so do whichever The question of being implicit or explicit. I like your idea more, will change that. > davidedmundson wrote in sortedsystemtraymodel.h:35 > We tend not to write virtual at the start now we have the clearer override > keyword Another embarrassing lack of C++ knowledge... Especially this "new" (hehe, C++11) feature :) > davidedmundson wrote in systemtraymodel.cpp:115 > The applet is still alive after removeApplet is called. "this" is still alive > > dataItem is not. > > If an applet is added, removed and then (potentially during applet > teardown) it changes its title, this would crash. > > I don't know if that's a realistic scenario or not, but I would still maybe > disconnect all signals from applet -> this on removeApplet? During lifetime of the SystemTray, dataItems are never removed, they are just marked as not "renderable". Item is still alive, it is needed in configuration page (you can enable applet again in the configuration). Anyway, it is a good idea to disconnect signals, for the sake of consisteny. In addition, dataItems are deleted eventually which may (?) lead to a crash on shutdown. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D27088 To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik Cc: mart, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra
D27088: [applets/SystemTray] Implement sorting in the model
davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.
The role names part is nice.
I have one major-ish comment, and 2 pendantic comments that I don't really
care about.
INLINE COMMENTS
> sortedsystemtraymodel.cpp:47
> +if (categoriesComparison == 0) {
> +return compareDisplayAlphabetically(left, right);
> +} else {
I think calling QSortFilterProxyModel::lessThan(left, right); would do the same
then you don't need compareDisplayAlphabetically
your implementation looks fine though, so do whichever
> sortedsystemtraymodel.h:35
> +protected:
> +virtual bool lessThan(const QModelIndex _left, const QModelIndex
> _right) const override;
> +
We tend not to write virtual at the start now we have the clearer override
keyword
> systemtraymodel.cpp:115
> +dataItem->setData(applet->title(), Qt::DisplayRole);
> +connect(applet, ::Applet::titleChanged, this, [dataItem] (const
> QString ) {
> +dataItem->setData(title, static_cast(Qt::DisplayRole));
The applet is still alive after removeApplet is called. "this" is still alive
dataItem is not.
If an applet is added, removed and then (potentially during applet teardown)
it changes its title, this would crash.
I don't know if that's a realistic scenario or not, but I would still maybe
disconnect all signals from applet -> this on removeApplet?
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D27088
To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik
Cc: mart, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh,
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf,
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra
D27088: [applets/SystemTray] Implement sorting in the model
kmaterka added a comment. OK, I will wait for second review from @davidedmundson, @broulik or @mart. REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D27088 To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik Cc: mart, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra
D27088: [applets/SystemTray] Implement sorting in the model
ngraham accepted this revision. ngraham added a subscriber: mart. ngraham added a comment. This revision is now accepted and ready to land. LGTM but let's make sure one of our model experts (@davidedmundson, @broulik, @mart) gets a chance to make sure I'm not smoking crack by approving this. :) REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D27088 To: kmaterka, #plasma_workspaces, #plasma, davidedmundson, ngraham, broulik Cc: mart, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra
