[SystemTray] Use unified data model everywhere
ClosedPublic

Authored by kmaterka on Jan 29 2020, 3:33 PM.

Details

Summary

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.

Test Plan

Affects almost all areas of system tray.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kmaterka requested review of this revision.Jan 29 2020, 3:33 PM
kmaterka created this revision.
kmaterka added a comment.EditedJan 29 2020, 3:36 PM

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?

kmaterka updated this revision to Diff 74805.Jan 31 2020, 3:39 PM

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.

First revision: D27085. In contains some random improvements

... and second one: D27088. It contains model refactoring and sorting.

After that two are accepped I will rebase this one.

kmaterka updated this revision to Diff 75134.Feb 6 2020, 7:44 PM

Rebase, only D27088 left

kmaterka updated this revision to Diff 76028.Feb 19 2020, 9:36 PM

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).

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

ngraham accepted this revision.Mar 4 2020, 5:32 PM

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.

This revision is now accepted and ready to land.Mar 4 2020, 5:32 PM
davidedmundson accepted this revision.Mar 6 2020, 2:32 PM
davidedmundson added inline comments.
applets/systemtray/package/contents/ui/items/AbstractItem.qml
34–35

Lets look at fixing in a follow up patch

QML can do enums now

kmaterka marked an inline comment as done.Mar 7 2020, 10:19 AM
kmaterka added inline comments.
applets/systemtray/package/contents/ui/items/AbstractItem.qml
34–35

OK, I will change that in a separate patch

This revision was automatically updated to reflect the committed changes.
kmaterka marked an inline comment as done.
kossebau added inline comments.
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?

kmaterka marked an inline comment as done.Jul 2 2020, 6:57 PM
kmaterka added inline comments.
applets/systemtray/systemtray.h
100

I don't know, this code was there before my changes, I've just done some refactoring here.

kossebau added inline comments.Jul 2 2020, 7:19 PM
applets/systemtray/systemtray.h
100

Thanks for your reply. Okay, so seems you did not hit anything related.

Thing is, you added
``
Q_INVOKABLE Plasma::Service *serviceForSource(const QString &source);

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 :)
kmaterka marked an inline comment as done.Jul 2 2020, 8:52 PM
kmaterka added inline comments.
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?

kossebau added inline comments.Jul 2 2020, 10:17 PM
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 :)