D9414: Implement OSD to select action when unknown monitors is connected

[Kai Uwe Broulik]

broulik added a comment.


  Cool.

INLINE COMMENTS

> daemon.cpp:188
> +{
> +    self->deleteLater();
> +

This makes me uneasy.

> daemon.cpp:195
> +    case KScreen::OsdAction::SwitchToInternal:
> +        qCDebug(KSCREEN_KDED) << "OSD: swutch to internal";
> +        doApplyConfig(Generator::self()->displaySwitch(Generator::TurnOffExternal));

switch

> osd.cpp:44
>  
>      m_osdObject->setSource(QUrl::fromLocalFile(osdPath));
>  

Can we change it to create the `QmlObject` on demand the first time it's used?

> osd.cpp:94
>  
> +void Osd::showActionSelector() {
> +    auto *rootObject = m_osdObject->rootObject();

Coding style, brace on next line

> osd.cpp:114-115
> +{
> +    if (m_output) {
> +        if (!m_output->isConnected() || !m_output->isEnabled() || !m_output->currentMode()) {
> +            hideOsd();

Can be simplified to

  if (!m_output || !m_output->isConnected() || ...)

> osdmanager.cpp:51
> +                this, [this](Action action) {
> +                    Q_EMIT selected(this, action);
> +                });

Why do you pass `this` in the signal?

> osdmanager.cpp:60
>  {
> +    qmlRegisterUncreatableType<OsdAction>("org.kde.KScreen", 1, 0, "OsdAction", "You cannot create OsdAction");
> +

Shouldn't it rather do that in the plugin's `registerTypes`?

> osdmanager.cpp:134
>                  KScreen::Osd* osd = nullptr;
> -                if (m_osds.keys().contains(output->name())) {
> +                if (m_osds.contains(output->name())) {
>                      osd = m_osds.value(output->name());

Can be simplified even further

  KScreen::Osd *osd = m_osds.value(output->name());
  if (!osd) {
      osd = new KScreen::Osd ...
  ...

> osdmanager.cpp:202
> +            KScreen::Osd* osd = nullptr;
> +            if (m_osds.contains(osdOutput->name())) {
> +                osd = m_osds.value(osdOutput->name());

Simplify here too

> OsdSelector.qml:42
> +        height: parent.height - label.height - ((units.smallSpacing/2) * 3)
> +        width: (actionRepeater.model.length * height) + ((actionRepeater.model.length - 1) * buttonRow.spacing);
> +

`Repeater` has a `count` property, avoids copying that JS Array over just to get its length

(also, urgh, I hoped `ButtonRow` was smarter than that)

> OsdSelector.qml:79
> +            delegate: PlasmaComponents.Button {
> +                PlasmaCore.IconItem {
> +                    source: modelData.iconSource

Why do you need an `IconItem` inside? The `Button` can have an icon of its own

REPOSITORY
  R104 KScreen

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

To: dvratil, sebas, davidedmundson, #vdg
Cc: abetts, broulik, kamathraghavendra, graesslin, ngraham, plasma-devel, mlaurent, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180118/5fcfc58b/attachment-0001.html>