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
