[Differential] [Commented On] D3598: rework kscreen's OSD logic

[broulik (Kai Uwe Broulik)]

broulik added a comment.


  A bunch of nitpicks below. Haven't tried it yet, though :)
  
  Is there a chance we could serve the novice user usecase of Meta+P and then *click* / choose the desired setup? As far as I can tell we only show "You now use $setup" but not "You can choose: [Clone, Left of, Internal only, External only, Right of]" (doesn't neccessarily have to be part of this patchset, obviously, just food for thought, and you know how obsessed I am with the Meta+P menu ;)

INLINE COMMENTS

> daemon.cpp:242-243
>      QDBusConnection::sessionBus().asyncCall(msg);
> +
> +
> +}

Superfluous lines

> daemon.cpp:283
>  
> +    QHash<Generator::DisplaySwitchAction, QString> actionMessages({
> +        {Generator::DisplaySwitchAction::None, i18nc("osd when displaybutton is pressed", "No Action")},

static?

> daemon.cpp:287
> +        {Generator::DisplaySwitchAction::ExtendToLeft, i18nc("osd when displaybutton is pressed", "Extend Left")},
> +        {Generator::DisplaySwitchAction::TurnOffEmbedded, i18nc("osd when displaybutton is pressed", "Embedded Off")},
> +        {Generator::DisplaySwitchAction::TurnOffExternal, i18nc("osd when displaybutton is pressed", "External Off")},

Not sure about "Embedded", perhaps "Internal"? Dunno.

Also I'd rather turn the wording around, rather than "internal off" and "external off", say "external only" and "internal only"?

> daemon.cpp:291
> +    });
> +    QString message = actionMessages.value(m_iteration);
> +

const &

> osd.cpp:32
> +
> +namespace KScreen {
> +

using namespace

> osd.cpp:37
> +    , m_output(output)
> +    , m_osdPath(QStandardPaths::locate(QStandardPaths::QStandardPaths::GenericDataLocation, QStringLiteral("kded_kscreen/qml/Osd.qml")))
> +    , m_osdObject(new KDeclarative::QmlObject(this))

You're only using it in the constructor, no need for it to be a member variable that stays around

> osd.cpp:38
> +    , m_osdPath(QStandardPaths::locate(QStandardPaths::QStandardPaths::GenericDataLocation, QStringLiteral("kded_kscreen/qml/Osd.qml")))
> +    , m_osdObject(new KDeclarative::QmlObject(this))
> +{

Can we lazy-load this first time it is used? It's not like you often change screen setup

Edit: nvm, saw later that you actually create the entire thing on demand

> osd.cpp:53
> +
> +    if (!m_osdTimer) {
> +        m_osdTimer = new QTimer(this);

Is always null here

> osd.cpp:85
> +    } else {
> +        realSize = QSize(mode->size().height(), mode->size().width());
> +    }

QSize has a transpose() method "Swaps the width and height values."

> osd.cpp:120
> +    // pukes loads of warnings into our logs
> +    if (qGuiApp->platformName() == QStringLiteral("xcb")) {
> +        rootObject->setProperty("animateOpacity", false);

Compare with QL1S - in case you have KWindowSystem you can ask it

> osd.h:47
> +    void showOutputIdentifier(const KScreen::OutputPtr output);
> +
> +

Superfluous line

> osd.h:49
> +
> +private Q_SLOTS:
> +    void hideOsd();

Doesn't have to be a slot with new connect syntax

> osdmanager.cpp:21
> +#include "osd.h"
> +#include "debug.h"
> +

Unused?

> osdmanager.cpp:31
> +
> +OsdManager* OsdManager::m_instance = 0;
> +

nullptr, also s_ prefix since it's static

> osdmanager.cpp:41
> +    connect(m_cleanupTimer, &QTimer::timeout, this, [this]() {
> +        qDeleteAll(m_osds.begin(), m_osds.end());
> +        m_osds.clear();

There's a a qDeleteAll(m_osds) that does begin() and end() for you

> osdmanager.cpp:44
> +    });
> +    QDBusConnection::sessionBus().registerObject(QStringLiteral("/org/kde/kscreen/osdService"), this, QDBusConnection::ExportAllSlots | QDBusConnection::ExportAllSignals);
> +}

There are no signals in this class

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

QMap::contains(key), no need for keys()

Also, better use (const)Find to avoid double loop-up (once for contains and once for value)

> osdmanager.cpp:99
> +
> +            const KScreen::ConfigPtr config = qobject_cast<KScreen::GetConfigOperation*>(op)->config();
> +

Can you perhaps put that stuff into a separate function to avoid code duplication - the loops are almost identical

> Osd.qml:57
> +            if (item != undefined) {
> +                item.rootItem = root;
> +            }

Not really happy with this "rootItem" property but then David told us that randomly accessing properties outside a file is bad, too ;)

Perhaps try

  Loader setSource(source, {rootItem: root})

so the item is already created with that property set, avoids warnings and/or `foo ? foo.bar : null` dance and re-evaluations

> OsdItem.qml:55
> +    }
> +    Component.onCompleted: print("osditem loaded..." + root.infoText);
> +}

;)

REPOSITORY
  R104 KScreen

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: sebas, #plasma
Cc: broulik, plasma-devel, davidedmundson, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20161205/d5eb96d8/attachment-0001.html>