broulik added inline comments. INLINE COMMENTS
> kcm.cpp:88 > + m_newStuffDialog->winId(); // so it creates the windowHandle(); > connect(m_newStuffDialog.data(), &KNS3::DownloadDialog::accepted, > this, &KCMPlymouth::load); > connect(m_newStuffDialog.data(), &KNS3::DownloadDialog::finished, > m_newStuffDialog.data(), &KNS3::DownloadDialog::deleteLater); Ideally here we would reload the model and then set the newly installed theme as current > kcm.cpp:157 > + const auto list = dir.entryInfoList(); > + for (const QFileInfo &fileInfo : list) { > + const QString pluginName = fileInfo.fileName(); I think the model population must be split from the KCM `load()` so you can reload the model without marking it as non-dirty. Right now when installing a new theme, the Apply button doesn't enable and no theme is selected. Ideally it would select the newly installed theme. Also needs a `emit selectedPluginIndexChanged();` after reloading the model as a model reset will cause `ListView` to forget its `currentIndex` > kcm.cpp:210 > if (!rc) { > - KMessageBox::error(nullptr, i18n("Unable to authenticate/execute the > action: %1, %2", job->error(), job->errorString())); > + emit showErrorMessage(i18n("Unable to authenticate/execute the > action: %1 (%2)", job->error(), job->errorString())); > + emit load(); In error handling we should probably check for `error() == KAuth::ActionReply::UserCancelledError` to not show a pointless error message when user canceled > kcm.cpp:211 > + emit showErrorMessage(i18n("Unable to authenticate/execute the > action: %1 (%2)", job->error(), job->errorString())); > + emit load(); > } This isn't a signal, also why do you need to reload when saving? It's not like any themes could magically appear here? > kcm.h:72 > + > + void beginSave(); > + void finishSave(); I think this should be a property `Q_PROPERTY(bool saving...)` or more generic `busy` like in the other KCMs and then proper bindings in the UI > main.qml:68 > > - Rectangle { > - anchors { > - left: parent.left > - right: parent.right > - bottom: parent.bottom > - } > - height: childrenRect.height > - gradient: Gradient { > - GradientStop { > - position: 0.0 > - color: "transparent" > - } > - GradientStop { > - position: 1.0 > - color: Qt.rgba(0, 0, 0, 0.5) > - } > - } > - QtControls.Label { > - anchors { > - horizontalCenter: > parent.horizontalCenter > - } > - color: "white" > - text: model.display > - } > - } > - } > - Rectangle { > - opacity: grid.currentIndex == index ? 1.0 : 0 > - anchors.fill: parent > - border.width: units.smallSpacing * 2 > - border.color: syspal.highlight > - color: "transparent" > - Behavior on opacity { > - PropertyAnimation { > - duration: units.longDuration > - easing.type: Easing.OutQuad > - } > - } > - } > - MouseArea { > - anchors.fill: parent > - hoverEnabled: true > - onClicked: { > - grid.currentIndex = index > - kcm.selectedPlugin = model.pluginName > - } > - Timer { > - interval: 1000 // FIXME TODO: Use platform > value for tooltip activation delay. > + Kirigami.InlineMessage { > + id: infoLabel In all the other KCMs the `InlineMessage` is above the controls to not push them around as it comes and goes > main.qml:74 > > - running: parent.containsMouse && > !parent.pressedButtons > + QtControls.ProgressBar { > + id: progressBar I don't think this "progress" bar adds much value REPOSITORY R258 Plymouth KCM BRANCH improve-the-look-of-the-plymouth-boot-splash-screen-kcm-ui (branched from master) REVISION DETAIL https://phabricator.kde.org/D20549 To: GB_2, #plasma, #vdg, ngraham, broulik Cc: abetts, ngraham, broulik, #vdg, plasma-devel, #plasma, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart