Replace two wrapping PlasmaCore.SortFilterModel with custom sorting on model side. Extract common code to base class.
This fixes small issue in sorting and allows to implement other improvements (later on).
Details
Details
- Reviewers
davidedmundson ngraham broulik - Group Reviewers
Plasma: Workspaces Plasma - Commits
- R120:6d86c690d133: [applets/SystemTray] Implement sorting in the model
Everything should work as before.
Diff Detail
Diff Detail
- Repository
- R120 Plasma Workspace
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 22698 Build 22716: arc lint + arc unit
Comment Actions
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. :)
Comment Actions
The role names part is nice.
I have one major-ish comment, and 2 pendantic comments that I don't really care about.
| applets/systemtray/sortedsystemtraymodel.cpp | ||
|---|---|---|
| 47 ↗ | (On Diff #75112) | 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 |
| applets/systemtray/sortedsystemtraymodel.h | ||
| 35 ↗ | (On Diff #75112) | We tend not to write virtual at the start now we have the clearer override keyword |
| applets/systemtray/systemtraymodel.cpp | ||
| 115 ↗ | (On Diff #75112) | 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? |
| applets/systemtray/sortedsystemtraymodel.cpp | ||
|---|---|---|
| 47 ↗ | (On Diff #75112) | The question of being implicit or explicit. I like your idea more, will change that. |
| applets/systemtray/sortedsystemtraymodel.h | ||
| 35 ↗ | (On Diff #75112) | Another embarrassing lack of C++ knowledge... Especially this "new" (hehe, C++11) feature :) |
| applets/systemtray/systemtraymodel.cpp | ||
| 115 ↗ | (On Diff #75112) | 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. |