Port System Settings sidebar to QQC2
Needs ReviewPublic

Authored by GB_2 on Fri, Aug 2, 2:34 PM.

Details

Reviewers
ngraham
mart
Group Reviewers
Plasma
VDG
Maniphest Tasks
T10862: Make QML imports consistent
Summary


Also makes QML imports consistent.

Test Plan

Open the System Settings. Everything should still work.

Diff Detail

Repository
R124 System Settings
Branch
arcpatch-D22896
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15186
Build 15204: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Plasma. · View Herald TranscriptFri, Aug 2, 2:34 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
GB_2 requested review of this revision.Fri, Aug 2, 2:34 PM
GB_2 edited the summary of this revision. (Show Details)Fri, Aug 2, 2:35 PM
GB_2 edited the summary of this revision. (Show Details)
broulik added a subscriber: broulik.Fri, Aug 2, 2:51 PM

"Only issue", I think that's quite a severe regression?

ngraham added a subscriber: ngraham.Fri, Aug 2, 3:15 PM

Yeah that's pretty bad. I think we need to fix that first--or at the very least, make it close when clicking on the button.

GB_2 edited the summary of this revision. (Show Details)Fri, Aug 2, 3:19 PM

Yeah that's pretty bad. I think we need to fix that first--or at the very least, make it close when clicking on the button.

It does that. Making it close when clicking anywhere would need some C++ code though.

filipf added a subscriber: filipf.Fri, Aug 2, 3:22 PM

If it would be draining to do now, we can leave that menu at QQC1 and add a note pointing to a bug report?

Ah so it does. I think that's the most important case to cover, so tentative +1 from me.

GB_2 added a comment.Fri, Aug 2, 6:27 PM

That is good enough IMO, but if anyone knows how to fix this, then please do :-)

ngraham accepted this revision.Fri, Aug 2, 6:30 PM

Me too, but please wait for a Plasma person to approve or reject. :)

This revision is now accepted and ready to land.Fri, Aug 2, 6:30 PM
GB_2 edited the summary of this revision. (Show Details)Sat, Aug 3, 5:56 AM
GB_2 updated this revision to Diff 63180.Tue, Aug 6, 12:47 PM

Close menu when clicking anywhere in the window. Most likely not the best solution.

GB_2 edited the summary of this revision. (Show Details)Tue, Aug 6, 12:48 PM
GB_2 edited the summary of this revision. (Show Details)

Hey, that works great!

GB_2 updated this revision to Diff 63338.Thu, Aug 8, 8:44 AM

Rebase on master

broulik added inline comments.Thu, Aug 8, 9:04 AM
sidebar/SidebarMode.cpp
605

Not a huge fan of this.
Can this be done in qqc2-desktop-style to be generic and fix every Menu and ComboBox popup?

GB_2 added inline comments.Thu, Aug 8, 9:59 AM
sidebar/SidebarMode.cpp
605

Not really, seems like the KQuickStyleItem only has access to the QML window of the menu control and also can't detect events on the rest of the QWidget window :(

GB_2 updated this revision to Diff 63403.Fri, Aug 9, 1:14 PM

Use Qt Labs Platform menu (need to wait until https://bugreports.qt.io/browse/QTBUG-77404 is fixed)

GB_2 updated this revision to Diff 63404.Fri, Aug 9, 1:15 PM

Rebase

GB_2 edited the summary of this revision. (Show Details)Fri, Aug 9, 1:16 PM
GB_2 updated this revision to Diff 63458.Sat, Aug 10, 8:02 AM

Add a "Show Most Used Page" button

GB_2 retitled this revision from Port System Settings sidebar to QQC2 and make QML imports consistent to [WIP] Port System Settings sidebar to QQC2 and add "Show Most Used Page" button.Sat, Aug 10, 8:06 AM
GB_2 edited the summary of this revision. (Show Details)
GB_2 edited the test plan for this revision. (Show Details)
GB_2 added a subscriber: mart.

Ping @mart, can you please help here?

GB_2 added a reviewer: mart.Sat, Aug 10, 8:07 AM
GB_2 updated this revision to Diff 63460.Sat, Aug 10, 8:16 AM

"Configure" -> "Configure..."

GB_2 edited the summary of this revision. (Show Details)Sat, Aug 10, 8:18 AM
mart accepted this revision.Sat, Aug 10, 8:04 PM
mart accepted this revision.Sat, Aug 10, 8:11 PM

Cool, tough I'm a bit hesitant on using qt.labs.platform as it may break any moment in any future release

Maybe the show most used action should go in a separate patch. This one feels like it's getting overloaded with too many different changes.

GB_2 added a comment.EditedSat, Aug 10, 8:15 PM

Maybe the show most used action should go in a separate patch. This one feels like it's getting overloaded with too many different changes.

Yeah, that feature is also not ready yet, because it currently doesn't reset the sidebar selection.

mart requested changes to this revision.Sat, Aug 10, 8:21 PM
This revision now requires changes to proceed.Sat, Aug 10, 8:21 PM
GB_2 updated this revision to Diff 63611.Mon, Aug 12, 3:31 PM

Use a QMenu

GB_2 retitled this revision from [WIP] Port System Settings sidebar to QQC2 and add "Show Most Used Page" button to Port System Settings sidebar to QQC2 and add "Show Most Used Page" button.Mon, Aug 12, 3:33 PM
GB_2 edited the summary of this revision. (Show Details)
GB_2 retitled this revision from Port System Settings sidebar to QQC2 and add "Show Most Used Page" button to Port System Settings sidebar to QQC2.
broulik added inline comments.Mon, Aug 12, 4:10 PM
sidebar/SidebarMode.cpp
403

This leaks.
menu->setAttribute(Qt::WA_DeleteOnClose);

404

const

405

I think it's better to iterate the list of actions and then get them from the collection. This way the order is also preserved correctly:

for (const QString &actionName : actionList) {
    menu->addAction(d->collection->action(actionName);
}
411

Don't exec() in conjunction with QML, this is just asking for trouble. Use popup() instead

sidebar/package/contents/ui/main.qml
44

Can you instead do a Q_PROPERTY(bool actionMenuVisible ...) in systemsettings which you set true before the menu opens and set false in aboutToHide. Then bind the checked of the button to it.
This way everything is in a predictable space and not called all over the place.

GB_2 updated this revision to Diff 63614.Mon, Aug 12, 7:42 PM
GB_2 marked 7 inline comments as done.

Address comments

GB_2 marked 7 inline comments as not done.Mon, Aug 12, 7:43 PM
GB_2 marked 7 inline comments as done.
GB_2 updated this revision to Diff 63799.Thu, Aug 15, 2:14 PM

Bind checked property directly

ngraham added inline comments.Thu, Aug 15, 4:42 PM
app/SettingsBase.cpp
142–143

lolwut

The default shortcut for "Configure" is Ctrl++comma these days!

I never even new this had a shortcut because only with your patch does it become visible. :)

GB_2 updated this revision to Diff 63827.Thu, Aug 15, 5:08 PM

Use standard shortcut for "Configure System Settings..." action

GB_2 marked an inline comment as done.Thu, Aug 15, 5:08 PM
GB_2 edited the test plan for this revision. (Show Details)
ngraham accepted this revision.Thu, Aug 15, 5:11 PM

Working very nice now! Just some minor nitpicks then this can go in

sidebar/SidebarMode.h
54

Doesn't need a WRITE as it is internal

77

Make the setter private

106

Move this to the Private class

GB_2 updated this revision to Diff 63872.Fri, Aug 16, 4:26 PM

Address comments

GB_2 marked 3 inline comments as done.Fri, Aug 16, 4:27 PM