Port System Settings sidebar to QQC2
ClosedPublic

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

Details

Summary


Also makes QML imports consistent.

Test Plan

Open the System Settings. Everything should still work.

Diff Detail

Repository
R124 System Settings
Branch
port-system-settings-sidebar-to-qqc2-and-make-qml-imports-consistent (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14867
Build 14885: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
ngraham added a subscriber: ngraham.Aug 2 2019, 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)Aug 2 2019, 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.Aug 2 2019, 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.Aug 2 2019, 6:27 PM

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

ngraham accepted this revision.Aug 2 2019, 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.Aug 2 2019, 6:30 PM
GB_2 edited the summary of this revision. (Show Details)Aug 3 2019, 5:56 AM
GB_2 updated this revision to Diff 63180.Aug 6 2019, 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)Aug 6 2019, 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.Aug 8 2019, 8:44 AM

Rebase on master

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

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.Aug 8 2019, 9:59 AM
sidebar/SidebarMode.cpp
570

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.Aug 9 2019, 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.Aug 9 2019, 1:15 PM

Rebase

GB_2 edited the summary of this revision. (Show Details)Aug 9 2019, 1:16 PM
GB_2 updated this revision to Diff 63458.Aug 10 2019, 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.Aug 10 2019, 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.Aug 10 2019, 8:07 AM
GB_2 updated this revision to Diff 63460.Aug 10 2019, 8:16 AM

"Configure" -> "Configure..."

GB_2 edited the summary of this revision. (Show Details)Aug 10 2019, 8:18 AM
mart accepted this revision.Aug 10 2019, 8:04 PM
mart accepted this revision.Aug 10 2019, 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.EditedAug 10 2019, 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.Aug 10 2019, 8:21 PM
This revision now requires changes to proceed.Aug 10 2019, 8:21 PM
GB_2 updated this revision to Diff 63611.Aug 12 2019, 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.Aug 12 2019, 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.Aug 12 2019, 4:10 PM
sidebar/SidebarMode.cpp
401

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

402

const

403

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);
}
409

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.Aug 12 2019, 7:42 PM
GB_2 marked 7 inline comments as done.

Address comments

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

Bind checked property directly

ngraham added inline comments.Aug 15 2019, 4:42 PM
app/SettingsBase.cpp
143 ↗(On Diff #63799)

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.Aug 15 2019, 5:08 PM

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

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

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

sidebar/SidebarMode.h
54 ↗(On Diff #63827)

Doesn't need a WRITE as it is internal

77 ↗(On Diff #63827)

Make the setter private

107 ↗(On Diff #63827)

Move this to the Private class

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

Address comments

GB_2 marked 3 inline comments as done.Aug 16 2019, 4:27 PM
GB_2 updated this revision to Diff 63968.Aug 18 2019, 8:07 AM

Add tooltip to menu button

broulik accepted this revision.Aug 19 2019, 4:32 PM

Fix it, then shipit!

sidebar/SidebarMode.h
107 ↗(On Diff #63827)

This isnt addressed

GB_2 updated this revision to Diff 64057.Aug 19 2019, 5:06 PM

Move setActionMenuVisible to the Private class

GB_2 marked an inline comment as done.Aug 19 2019, 5:06 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 19 2019, 6:15 PM
This revision was automatically updated to reflect the committed changes.