ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.
All the left labels in the formLayouts need to end with a colon, e.g.:
Delay: [ 0]
Don't use `onCheckStateChanged` or `onCheckedChanged` for checkboxes when you
want to handle user interaction;i t causes binding loops. Use `onToggled`
instead.
Put whitespace between logical sections in the formlayout. For example
between the audible bell and visual bell sections. You can do this like so:
Item {
Kirigami.FormData.isSection: true
}
Change `QtQuick.Controls 2.12` to `QtQuick.Controls 2.11` everywhere; we
don't formally depends on Qt 5.12 yet.
INLINE COMMENTS
> ActivationGestures.qml:29
> + QQC2.CheckBox {
> + Kirigami.FormData.label: i18n("Activation Gestures")
> + text: i18n("Use gestures for activating sticky keys and slow keys")
This section can be expressed more clearly and compactly as follows:
Sticky and slow keys: [ ] Use gestures to activate
[ ] Disable after: [99 seconds ^v]
In fact, this section maybe should be moved to the Modifier Keys page, and then
we can remove the Activation Gestures page entirely.
> ActivationGestures.qml:30
> + Kirigami.FormData.label: i18n("Activation Gestures")
> + text: i18n("Use gestures for activating sticky keys and slow keys")
> + checked: kcm.keyboardSettings.stickyKeys
Can be shortened to "Use gestures for activating sticky and slow keys"
> ActivationGestures.qml:50
> + QQC2.CheckBox {
> + Kirigami.FormData.label: i18n("Notification")
> + text: i18n("Use the system bell whenever a gesture is used to toggle
> an accessibility feature")
This section doesn't have anything to do with activation gestures. Its
checkboxes might be better placed on other pages more relevant to their content.
> ActivationGestures.qml:69
> + QQC2.Button {
> + text: i18n("Configure Notifications")
> + onClicked: kcm.configureKNotify();
add ellipsis and an icon (`preferences-desktop-notification`)
> ModifierKeys.qml:38
> + enabled: !kcm.keyboardSettings.isImmutable("StickyKeysLatch") &&
> kcm.keyboardSettings.stickyKeys
> + text: i18n("Lock sticky keys")
> + checked: kcm.keyboardSettings.stickyKeysLatch
Can be shortened to just "Lock"
> ModifierKeys.qml:45
> + enabled: !kcm.keyboardSettings.isImmutable("StickyKeysAutoOff") &&
> kcm.keyboardSettings.stickyKeys
> + text: i18n("Turn off sticky keys when two keys are pressed
> continuously")
> + checked: kcm.keyboardSettings.stickyKeysAutoOff
Can be shortened to "Disable when two keys are held down"
> ModifierKeys.qml:51
> + enabled: !kcm.keyboardSettings.isImmutable("StickyKeysBeep") &&
> kcm.keyboardSettings.stickyKeys
> + text: i18n("Use system bell whenever a modifier gets latched, locked
> or unlocked")
> + checked: kcm.keyboardSettings.stickyKeysBeep
Can be shortened to "Use system bell when modifier keys are used" (I *think*
this is what the string is trying to communicate; correct me if I'm wrong)
> ModifierKeys.qml:58
> + Kirigami.FormData.label: i18n("Locking Keys")
> + text: i18n("Use system bell whenever a locking key gets toggled")
> + checked: kcm.keyboardSettings.toggleKeysBeep
Can be slightly shortened to "Use system bell when locking keys are toggled"
> ModifierKeys.qml:64
> + QQC2.CheckBox {
> + text: i18n("Show a notification for modifier or locking key state
> changes")
> + checked: kcm.keyboardSettings.kNotifyModifiers
Can be shortened to "Show notification when modifier or locking keys are used"
> MouseNavigation.qml:31
> + Kirigami.FormData.label: i18n("Activation Gestures")
> + text: i18n("Move pointer with keyboard (using the num pad)")
> + checked: kcm.mouseSettings.mouseKeys
Change to "Enable using keyboard number pad to move cursor"
> MouseNavigation.qml:35
> + enabled: !kcm.mouseSettings.isImmutable("MouseKeys")
> + }
> + QQC2.SpinBox {
Add spacing after this
> ScreenReader.qml:30
> + Kirigami.FormData.label: i18n("Screen Reader")
> + text: i18n("enabled")
> + checked: kcm.screenReaderSettings.enabled
In this case, just put the whole text in the checkbox:
[ ] Enable screen reader
[ Launch Orca Configuration...]
> ScreenReader.qml:36
> + QQC2.Button {
> + text: i18n("Launch Orca Configuration")
> + onClicked: kcm.screenReaderSettings.launchOrcaConfiguration()
What's Orca? Isn't that a kind of whale? :p Make change the text to be
"Configure Orca Screen Reader..."
> ScreenReader.qml:37
> + text: i18n("Launch Orca Configuration")
> + onClicked: kcm.screenReaderSettings.launchOrcaConfiguration()
> + enabled: !kcm.screenReaderSettings.isImmutable("Enabled")
If Orca isn't installed, clicking on this button does nothing. In this case, it
should be disabled with a message below it explaining why it's disabled.
> main.qml:37
> + QQC2.ScrollView {
> + id: bgObject
> + Component.onCompleted: bgObject.background.visible = true
should be more descriptive, like `sidebarScrollView` or something
> main.qml:50
> +
> + delegate: Kirigami.BasicListItem {
> + width: listView.width
needs to force focus to the scrollview when clicked: `onClicked:
listView.forceActiveFocus();`
> main.qml:53
> + icon: model.icon
> + label: i18n(model.title)
> + }
title should already be translated; no need to put this in an `i18n()` call
> main.qml:77
> + icon: "preferences-desktop-notification-bell"
> + title: QT_TR_NOOP("Bell")
> + }
Shouldn't these be `i18n()` calls?
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
