Use the unified data model everywhere, not just in configuration. This simplifies UI code, separetes presentation from data.
This requires the use of ListView and GridView.
This change enables the implementation of more advanced sorting algorithms for systemtray items.
Details
- Reviewers
davidedmundson ngraham broulik - Group Reviewers
Plasma: Workspaces Plasma - Commits
- R120:39975673cb6a: [SystemTray] Use unified data model everywhere
Affects almost all areas of system tray.
Diff Detail
- Repository
- R120 Plasma Workspace
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 22699 Build 22717: arc lint + arc unit
This change is very big. I can try to split in into two (maybe more) if that's required. If yes, should I connect revisions somehow? Create task, add parent/child revision?
I will split it into smaller changes, this one is too big for a review. I'll keep it here as a backup.
The patch is big, but it's manageable.
Splitting is up to you, don't feel you have to.
... and second one: D27088. It contains model refactoring and sorting.
After that two are accepped I will rebase this one.
Second (and last) change extracted from this one is in master. Rebased, ready for review.
UI looks good except for one thing: the changes to show a highlight effect on hover in the popup are unrelated and should be done in a separate patch (preferably in coordination with VDG folks since we're moving towards the idea of using this in other places too).
This change should not introduce any visual change but one: add transition (main.qml, line 170).
I guess you are referring to HiddenItemsView.qml, are you? I had to change Column to ListView to support model so almost everything changed or moved. CurrentItemHighLight and PlasmaComponents.HighLight was there from the very beginning.
My bad, your patch actually fixes a pre-existing visual bug. :) That accounts for the change. +1 visually
UI and interaction are good, and a basic code review reveals no issues to me. Still, you may want to wait for a more thorough code review from a Plasma person.
applets/systemtray/package/contents/ui/items/AbstractItem.qml | ||
---|---|---|
34 | Lets look at fixing in a follow up patch QML can do enums now |
applets/systemtray/package/contents/ui/items/AbstractItem.qml | ||
---|---|---|
34 | OK, I will change that in a separate patch |
applets/systemtray/systemtray.h | ||
---|---|---|
100 | Hi. Seeing this code, I have a question: from what I understood so far, for this method to be useable from the JavaScript inside QML, the type "Plasma::Service *" needs to be registered with at least qRegisterMetaType<Plasma::Service *>("Plasma::Service*"); or qRegisterMetaType<Plasma::Service *>(); Yet I have not found any code in Plasma related projects which calls this. The module org.kde.plasma.core only has code to register the unnamespaced type name ""Service*" (indirectly via qmlRegisterInterface<Plasma::Service>("Service"); which internally calls the equivalent of qRegisterMetaType<Plasma::Service *>("Service*");. And the namespace in the registered type name string seems important, things broke elsewhere if the type signature used in the Q_INVOKABLE was different WRT namespace compared to the registered type name string. Do you remember anything related or would have some insights? |
applets/systemtray/systemtray.h | ||
---|---|---|
100 | I don't know, this code was there before my changes, I've just done some refactoring here. |
applets/systemtray/systemtray.h | ||
---|---|---|
100 | Thanks for your reply. Okay, so seems you did not hit anything related. Thing is, you added here, whereas the JavaScript code before was calling serviceForSource() on a Plasma::DataSource class, which does not return the type "Plasma::Service *", but "QObject *", cmp. its method definition Q_INVOKABLE QObject *serviceForSource(const QString &source); and the JavaScript engine for a plain QObject simply exposes any Q_INVOKABLEs and slot methods, that's why no extra registration would be needed (AFAIK). So still a mystery to me why this here seems to work, too bad :) |
applets/systemtray/systemtray.h | ||
---|---|---|
100 | Oh, OK, now I remember, sorry for misinforming you... I haven't had any problems with this, it "Just Worked" :) Maybe it should return just plain QObject*, I'm not experienced in this area, what do you think? |
applets/systemtray/systemtray.h | ||
---|---|---|
100 | I guess if this works and no-one reported errors, it should be fine. Also not fully experienced, but hit some issues elsewhere and now trying to understand the magic. Though right now confused by what I read in the docs, what I see in existing code and what my tweak & see experiments deliver (with different result each time, meh). So ignore for now. I have this on my list, and if I ever understood things fully and then would see some issue here, I will come back to this and ping you :) |