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
