D22896: Port System Settings sidebar to QQC2

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> SidebarMode.cpp:402
> +{
> +    QMenu *menu = new QMenu();
> +    QStringList actionList { QStringLiteral("configure"), QStringLiteral("help_contents"), QStringLiteral("help_about_app"), QStringLiteral("help_about_kde") };

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

> SidebarMode.cpp:403
> +    QMenu *menu = new QMenu();
> +    QStringList actionList { QStringLiteral("configure"), QStringLiteral("help_contents"), QStringLiteral("help_about_app"), QStringLiteral("help_about_kde") };
> +    for (QAction *a : d->collection->actions()) {

`const`

> SidebarMode.cpp:404
> +    QStringList actionList { QStringLiteral("configure"), QStringLiteral("help_contents"), QStringLiteral("help_about_app"), QStringLiteral("help_about_kde") };
> +    for (QAction *a : d->collection->actions()) {
> +        if (actionList.contains(a->objectName())) {

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

> SidebarMode.cpp:410
> +    connect(menu, &QMenu::aboutToHide, this, [this] () { QMetaObject::invokeMethod(d->quickWidget->rootObject(), "closeMenu"); } );
> +    menu->exec(position);
> +}

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

> main.qml:44
> +    function closeMenu() {
> +        mainColumn.actionMenuButton.checked = false;
> +    }

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.

REPOSITORY
  R124 System Settings

REVISION DETAIL
  https://phabricator.kde.org/D22896

To: GB_2, #plasma, #vdg, ngraham, mart
Cc: mart, filipf, ngraham, broulik, #vdg, plasma-devel, #plasma, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190812/8076db86/attachment.html>