broulik added a comment.
Pretty cool! INLINE COMMENTS > kcm.cpp:48 > + qmlRegisterType<OutputModel>(); > + qmlRegisterType<KScreen::Output>("org.kde.kscreen", 1, 0, > "KScreenOutput"); > + Given this is for the KCM only, I would suggest to name the import `org.kde.private.kcm.kscreen` > output_model.cpp:45 > + const KScreen::OutputPtr &output = m_outputs[index.row()].ptr; > + if (role == Qt::DisplayRole) { > + return output->name(); `switch`? > output_model.cpp:157 > + QHash<int, QByteArray> roles; > + roles[Qt::DisplayRole] = "display"; > + roles[EnabledRole] = "enabled"; You could also initialize it with `QAbstractItemModel::roleNames()` (which has "display") and then add yours to it > OutputPanel.qml:41 > + Controls.CheckBox { > + Kirigami.FormData.label: "Enabled" > + checked: model.enabled `i18n` and everywhere else > OutputPanel.qml:41 > + Controls.CheckBox { > + Kirigami.FormData.label: "Enabled" > + checked: model.enabled Try `Kirigami.FormData.checkable: true` to make the checkbox to the left instead of having a lonely tiny checkbox floating there > OutputPanel.qml:49 > + checked: model.primary > + onCheckedChanged: model.primary = checked > + visible: kcm.primaryOutputSupported Use `onClicked` which only fires when the user explicitly clicks it, not in case a property/model role changes on its own, also everywhere else > OutputPanel.qml:56 > + Layout.fillWidth: true > + Controls.Slider { > + id: resSlider When you have loads of resolutions, we change the `Slider` for a `ComboBox`. I don't really like that but a slider could be fiddly to operate with many steps > OutputPanel.qml:111 > + > + RotationButton { > + value: 0 I think you might want to use a `ButtonGroup` here to make them semantically mutally exclusive (just a suggestion) > OutputPanel.qml:114 > + tooltip: i18n("No Rotation") > + onClicked: model.rotation = 1 > + checked: model.rotation === 1 Please use enum names here instead of magic numbers > OutputPanel.qml:138 > + Controls.ComboBox { > + Layout.fillWidth: true > + Kirigami.FormData.label: "Refresh rate" You sure this combobox should be full width? > OutputPanel.qml:140 > + Kirigami.FormData.label: "Refresh rate" > + model: outputPanel.refreshRates > + currentIndex: outputPanel.getRefreshRateIndex() They lack formatting (decimal separator and "Hz" suffix) > OutputPanel.qml:141 > + model: outputPanel.refreshRates > + currentIndex: outputPanel.getRefreshRateIndex() > + onCurrentIndexChanged: Why not just bind those properties directly? > OutputPanel.qml:142 > + currentIndex: outputPanel.getRefreshRateIndex() > + onCurrentIndexChanged: > + outputPanel.setRefreshRateIndex(currentIndex) Use `onActivated` which only fires when the user explicitly changes it > Panel.qml:34 > + } > + // TODO: this is a hack to align the label with other labels > + // what we probably really want is using a FormLayout from Have you tried `twinFormLayouts`? > Panel.qml:88 > + Layout.fillWidth: true > + Kirigami.FormData.label: "Scale" > + Why is "Scale" under arrangement? > Panel.qml:98 > + to: 3 > + stepSize: 0.1 > + live: true Can we make it not show all the tick marks but only the integer ones like in the current scale UI? > Panel.qml:101 > + value: kcm.globalScale > + onValueChanged: kcm.globalScale = value > + } Use `onMoved` which blabla, see `onClicked` and `onActivated` > Panel.qml:105 > + Layout.alignment: Qt.AlignHCenter > + text: Math.round(globalScaleSlider.value * 10) / 10 > + } You can pretty-format the number like this: globalScaleSlider.value.toLocaleString(Qt.locale(), "f", 1) (If that doesn't work wrap it in `Number(...).toLocaleString(...)`) > Panel.qml:122 > + > + onCheckedChanged: checked ? kcm.outputRetention = 0 : 0 > + } `onClicked` I also think the operator priorities are a bit mixed up here > RotationButton.qml:32 > + > + PlasmaComponents.ToolButton { > + id: button Don't use `PlasmaComponents` stuff in widget-like dialogs. Instead, you want to be using `QQC2.Button` and then overriding its `contentItem` (I think). It is a bit convoluted as qqc2-desktop-style paints the entire button as `background` ooooor you just turn the button on its side like you do below :D REPOSITORY R104 KScreen REVISION DETAIL https://phabricator.kde.org/D22468 To: romangg, #plasma, #kwin Cc: broulik, filipf, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart