D25375: Start of the accessibility KCM
Kai Uwe Broulik
noreply at phabricator.kde.org
Wed Jan 8 13:33:22 GMT 2020
broulik added inline comments.
INLINE COMMENTS
> kcmaccess.cpp:141
>
> - QString result;
> - if ((modifiers & ScrollMask) != 0)
> - if ((modifiers & LockMask) != 0)
> - if ((modifiers & NumMask) != 0)
> - result = i18n("Press %1 while NumLock, CapsLock and ScrollLock are active", keyname);
> - else
> - result = i18n("Press %1 while CapsLock and ScrollLock are active", keyname);
> - else if ((modifiers & NumMask) != 0)
> - result = i18n("Press %1 while NumLock and ScrollLock are active", keyname);
> - else
> - result = i18n("Press %1 while ScrollLock is active", keyname);
> - else if ((modifiers & LockMask) != 0)
> - if ((modifiers & NumMask) != 0)
> - result = i18n("Press %1 while NumLock and CapsLock are active", keyname);
> - else
> - result = i18n("Press %1 while CapsLock is active", keyname);
> - else if ((modifiers & NumMask) != 0)
> - result = i18n("Press %1 while NumLock is active", keyname);
> - else
> - result = i18n("Press %1", keyname);
> -
> - return result;
> + return modifiers & ScrollMask & LockMask & NumMask ? i18n("Press %1 while NumLock, CapsLock and ScrollLock are active", keyname)
> + : modifiers & ScrollMask & LockMask ? i18n("Press %1 while CapsLock and ScrollLock are active", keyname)
Are you sure this shouldn't be something like
modifiers & (ScrollMask | LockMask | NumMask)
> kcmaccess.cpp:170
>
> - ui.setupUi(this);
> -
> - connect(ui.soundButton, &QPushButton::clicked, this, &KAccessConfig::selectSound);
> - connect(ui.customBell, &QCheckBox::clicked, this, &KAccessConfig::checkAccess);
> - connect(ui.systemBell, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.customBell, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.soundEdit, &QLineEdit::textChanged, this, &KAccessConfig::configChanged);
> -
> - connect(ui.invertScreen, &QRadioButton::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.flashScreen, &QRadioButton::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.visibleBell, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.visibleBell, &QCheckBox::clicked, this, &KAccessConfig::checkAccess);
> - connect(ui.colorButton, &KColorButton::clicked, this, &KAccessConfig::changeFlashScreenColor);
> -
> - connect(ui.invertScreen, &QRadioButton::clicked, this, &KAccessConfig::invertClicked);
> - connect(ui.flashScreen, &QRadioButton::clicked, this, &KAccessConfig::flashClicked);
> -
> - connect(ui.duration, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &KAccessConfig::configChanged);
> -
> -
> - // modifier key settings -------------------------------
> -
> - connect(ui.stickyKeys, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.stickyKeysLock, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.stickyKeysAutoOff, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.stickyKeys, &QCheckBox::clicked, this, &KAccessConfig::checkAccess);
> -
> - connect(ui.stickyKeysBeep, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.toggleKeysBeep, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.kNotifyModifiers, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.kNotifyModifiers, &QCheckBox::clicked, this, &KAccessConfig::checkAccess);
> - connect(ui.kNotifyModifiersButton, &QPushButton::clicked, this, &KAccessConfig::configureKNotify);
> -
> - // key filter settings ---------------------------------
> -
> - connect(ui.slowKeysDelay, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &KAccessConfig::configChanged);
> - connect(ui.slowKeys, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.slowKeys, &QCheckBox::clicked, this, &KAccessConfig::checkAccess);
> -
> - connect(ui.slowKeysPressBeep, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.slowKeysAcceptBeep, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.slowKeysRejectBeep, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> -
> - connect(ui.bounceKeysDelay, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &KAccessConfig::configChanged);
> - connect(ui.bounceKeys, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.bounceKeysRejectBeep, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.bounceKeys, &QCheckBox::clicked, this, &KAccessConfig::checkAccess);
> -
> - // gestures --------------------------------------------
> -
> - QString shortcut = mouseKeysShortcut(QX11Info::display());
> - if (shortcut.isEmpty())
> - ui.gestures->setToolTip(i18n("Here you can activate keyboard gestures that turn on the following features: \n"
> - "Sticky keys: Press Shift key 5 consecutive times\n"
> - "Slow keys: Hold down Shift for 8 seconds"));
> - else
> - ui.gestures->setToolTip(i18n("Here you can activate keyboard gestures that turn on the following features: \n"
> - "Mouse Keys: %1\n"
> - "Sticky keys: Press Shift key 5 consecutive times\n"
> - "Slow keys: Hold down Shift for 8 seconds", shortcut));
> -
> - connect(ui.gestures, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.timeout, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.timeout, &QCheckBox::clicked, this, &KAccessConfig::checkAccess);
> - connect(ui.timeoutDelay, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &KAccessConfig::configChanged);
> - connect(ui.accessxBeep, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.gestureConfirmation, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.kNotifyAccess, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.kNotifyAccess, &QCheckBox::clicked, this, &KAccessConfig::checkAccess);
> - connect(ui.kNotifyAccessButton, &QPushButton::clicked, this, &KAccessConfig::configureKNotify);
> -
> - // keynboard navigation
> - connect(ui.mouseKeys, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.mk_delay, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &KAccessConfig::configChanged);
> - connect(ui.mk_interval, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &KAccessConfig::configChanged);
> - connect(ui.mk_time_to_max, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &KAccessConfig::configChanged);
> - connect(ui.mk_max_speed, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &KAccessConfig::configChanged);
> - connect(ui.mk_curve, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &KAccessConfig::configChanged);
> -
> - // screen reader
> - connect(ui.screenReaderEnabled, &QCheckBox::clicked, this, &KAccessConfig::configChanged);
> - connect(ui.launchOrcaConfiguration, &QPushButton::clicked, this, &KAccessConfig::launchOrcaConfiguration);
> -}
> + int tryOrcaRun = QProcess::execute(QStringLiteral("orca"), {QStringLiteral("--version")});
> + m_screenReaderInstalled = tryOrcaRun != -2;
This call takes 250ms on my machine, perhaps it's better to do that asynchronously, and/or only when actually entering the screen reader page.
Also document the magic number, please
> kcmaccess.cpp:183
> {
> - KNotifyConfigWidget::configure(this, QStringLiteral("kaccess"));
> + KNotifyConfigWidget::configure(nullptr, QStringLiteral("kaccess"));
> }
This dialog doesn't have a transient parent now.
In notifications KCM I had to create the dialog myself since we don't have a `QWidget` here.
> kcmaccess.cpp:241
> {
> - bool custom = ui.customBell->isChecked();
> - ui.soundEdit->setEnabled(custom);
> - ui.soundButton->setEnabled(custom);
> - ui.soundLabel->setEnabled(custom);
> -
> - bool visible = ui.visibleBell->isChecked();
> - ui.invertScreen->setEnabled(visible);
> - ui.flashScreen->setEnabled(visible);
> - ui.colorButton->setEnabled(visible);
> - ui.duration->setEnabled(visible);
> -
> - bool sticky = ui.stickyKeys->isChecked();
> - ui.stickyKeysLock->setEnabled(sticky);
> - ui.stickyKeysAutoOff->setEnabled(sticky);
> - ui.stickyKeysBeep->setEnabled(sticky);
> -
> - bool slow = ui.slowKeys->isChecked();
> - ui.slowKeysDelay->setEnabled(slow);
> - ui.slowKeysPressBeep->setEnabled(slow);
> - ui.slowKeysAcceptBeep->setEnabled(slow);
> - ui.slowKeysRejectBeep->setEnabled(slow);
> -
> - bool bounce = ui.bounceKeys->isChecked();
> - ui.bounceKeysDelay->setEnabled(bounce);
> - ui.bounceKeysRejectBeep->setEnabled(bounce);
> -
> - bool useTimeout = ui.timeout->isChecked();
> - ui.timeoutDelay->setEnabled(useTimeout);
> + return QFileDialog::getOpenFileName(nullptr, i18n("Choose audio file"), QDir::homePath(), i18n("Audio Files (*.mp3 *.wav *.ogg)"));
> }
Don't ever use nested event loops in QML. You can use `QtQuick.Dialogs` `FileDialog` for that.
> kcmaccess.desktop:9
> -X-KDE-Library=kcm_access
> -X-KDE-Init-Symbol=kcminit_access
> X-KDE-ParentApp=kcontrol
What about this? We still need to autostart kaccess on login.
Perhaps, while at it, you could port it over to proper autostart mechanism rather than abusing kcminit for that.
> kcmaccess.h:41
> + Q_PROPERTY(ScreenReaderSettings *screenReaderSettings MEMBER m_screenReaderSettings CONSTANT)
> + Q_PROPERTY(QString orcaLaunchFeedback READ orcaLaunchFeedback WRITE setOrcaLaunchFeedback NOTIFY orcaLaunchFeedbackChanged)
> + Q_PROPERTY(QString desktopShortcutInfo MEMBER m_desktopShortcutInfo CONSTANT)
You're not writing to this from QML, doesn't need a `WRITE` accessor
> kcmaccess.h:42
> + Q_PROPERTY(QString orcaLaunchFeedback READ orcaLaunchFeedback WRITE setOrcaLaunchFeedback NOTIFY orcaLaunchFeedbackChanged)
> + Q_PROPERTY(QString desktopShortcutInfo MEMBER m_desktopShortcutInfo CONSTANT)
> + Q_PROPERTY(bool screenReaderInstalled MEMBER m_screenReaderInstalled CONSTANT)
Where is this being used?
> kcmaccess.h:52
>
> -protected Q_SLOTS:
> + Q_SCRIPTABLE void configureKNotify();
> + Q_SCRIPTABLE void launchOrcaConfiguration();
We typically use `Q_INVOKABLE` for methods exported to QML
> kcmaccess.h:58
> +public Q_SLOTS:
> + void setOrcaLaunchFeedback(const QString& value);
> +
Make this `private` and no slot
> Bell.qml:54
> +
> + placeholderText: i18n("Sound to Play");
> + text: kcm.bellSettings.customBellFile
I think we generally don't use `placeholderText` for text fields like this, where it won't be obvious what they do when there's some text inside? Consider adding a label instead.
> Bell.qml:62
> + QQC2.Button {
> + text: i18n("Search")
> +
Typically a "browse" button is just a folder icon. (Don't forget a `ToolTip` and `Accessible.name`)
> Bell.qml:89
> +
> + enabled: !kcm.bellSettings.isImmutable("VisibleBellInvert") && kcm.bellSettings.visibleBell
> +
Why are you writing to a different property than you check for immutability?
> Bell.qml:105
> + checked: !kcm.bellSettings.invertScreen
> + onCheckedChanged: kcm.bellSettings.invertScreen = !checked
> + }
`onToggled`
> Bell.qml:113
> + color: kcm.bellSettings.visibleBellColor
> + onColorChanged: kcm.bellSettings.visibleBellColor = color
> + }
Use `onAccepted`
> Bell.qml:122
> + value: kcm.bellSettings.visibleBellPause
> + onValueChanged: kcm.bellSettings.visibleBellPause = value
> + }
Use `onValueModified`
> KeyboardFilters.qml:57
> + Kirigami.FormData.label: i18n("Use system bell:")
> + text: i18n("&when a key is pressed")
> +
Add some i18n context `i18nc("Use system bell when a key is pressed", "...")`
Perhaps also an `Accessible.name`
> KeyboardFilters.qml:106
> +
> + onValueChanged: kcm.keyboardSettings.bounceDelay = value
> + }
Use `onValueModified` (and everywhere else)
> ScreenReader.qml:44
> + QQC2.Label {
> + text: kcm.orcaLaunchFeedback
> + }
This looks like the perfect use case for an `InlineMessage`?
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D25375
To: tcanabrava, ngraham, ervin
Cc: broulik, cblack, ervin, ognarb, mart, ngraham, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200108/b3b3cc29/attachment-0001.html>
More information about the Plasma-devel
mailing list