D27088: [applets/SystemTray] Implement sorting in the model

[David Edmundson]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200218/cc34afae/attachment-0001.html>