ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.
In general the UI here is pretty good. I think we can polish it a bit. In
particular, instead of having a header for a section and then a checkbox below
it to enable that section with the same text as the header, here's an idea for
an alternative: just use the checkbox itself as the header. For example:
[] Audible bell
Custom Bell: [] Sound: [ ]
[] Visual bell
Duration: [99]
Visual effect: (o) Flash color: [ color ]
( ) Invert screen
Another thing: you can save a lot of space by taking advantage of the
properties of a FormLayout by splitting strings up a bit. For example, for Slow
Keys, you can do the following:
Show System Bell: [x] When a key is pressed
[ ] When a key is accepted
[ ] When a key is rejected
Finally, I notice that the Apply button never gets enabled when a control is
changed.
INLINE COMMENTS
> Bell.qml:35
> + id: systemBell
> + text: i18n("Use system bell")
> + checked: kcm.bellSettings.systemBell
Instead of having a header and then a checkbox to enable it, with the same text
as the header, let's just use the checkbox itself as the header. i.e.:
[] Audible bell
Custom Bell: [] Sound: [ ]
[] Visual bell
Duration: [99]
Visual effect: (o) Flash color: [ color ]
( ) Invert screen
> Bell.qml:47
> + }
> + QQC2.TextField {
> + id: textEdit
Indent this by one GridUnit to show that its dependency on the checkbox above
it, or place it on the same line
> Bell.qml:70
> + text: i18n("Invert Screen")
> + enabled: !kcm.bellSettings.isImmutable("InvertScreen") &&
> kcm.bellSettings.visibleBell
> + checked: kcm.bellSettings.invertScreen
This radio button never gets enabled
> Bell.qml:81
> + onCheckedChanged: kcm.bellSettings.invertScreen = !checked
> + enabled: !kcm.bellSettings.isImmutable("InvertScreen")
> + }
This radio button never gets enabled
> Bell.qml:94
> + onValueChanged: kcm.bellSettings.visibleBellPause = value
> + Kirigami.FormData.label: i18n("Duration")
> + }
"Duration:"
> MouseNavigation.qml:38
> + onCheckStateChanged: kcm.mouseSettings.mouseKeys = checked
> + enabled: !kcm.keyboardSettings.isImmutable("MouseKeys")
> + }
This checkbox never gets enabled
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D25375
To: tcanabrava, ngraham
Cc: ognarb, mart, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh,
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai,
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra
