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 &source_left, const QModelIndex
> &source_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, &Plasma::Applet::titleChanged, this, [dataItem] (const
> QString &title) {
> + dataItem->setData(title, static_cast<int>(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
