D22468: Rewrite KScreen KCM as ConfigModule with outputs model and Kirigami

[Kai Uwe Broulik]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190715/0260dc48/attachment-0001.html>