Start of the accessibility KCM
Needs ReviewPublic

Authored by tcanabrava on Nov 18 2019, 6:32 PM.

Details

Reviewers
ngraham
ervin
Summary

This rewrites the Accessibility KCM to use Qml and the new Managed
Config classes.

Diff Detail

Repository
R119 Plasma Desktop
Branch
arcpatch-D25375
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20957
Build 20975: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Nov 21 2019, 3:07 PM
tcanabrava updated this revision to Diff 70749.EditedDec 2 2019, 1:47 PM
  • Fixes
  • Fixes texts and UI
  • Simplify Algorithm
  • Fix Enable / Disable status

The code is still not enabling the Apply button, I'm investigating as this should be taken care by the Managed Config Module.

tcanabrava updated this revision to Diff 70751.Dec 2 2019, 1:51 PM
tcanabrava marked an inline comment as done.
  • Fix factory name
tcanabrava updated this revision to Diff 70754.Dec 2 2019, 2:08 PM
tcanabrava marked 6 inline comments as done.
  • Add missing setButtons
  • Fix Typo
tcanabrava marked an inline comment as done.Dec 2 2019, 2:12 PM

Missing apply button being enabled seems something related to ManagedConfigModule. I don't know what to do there yet.

ngraham requested changes to this revision.Dec 2 2019, 5:07 PM

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.

kcms/access/package/contents/ui/ActivationGestures.qml
29 ↗(On Diff #70754)

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.

30 ↗(On Diff #70754)

Can be shortened to "Use gestures for activating sticky and slow keys"

50 ↗(On Diff #70754)

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.

69 ↗(On Diff #70754)

add ellipsis and an icon (preferences-desktop-notification)

kcms/access/package/contents/ui/ModifierKeys.qml
39

Can be shortened to just "Lock"

46

Can be shortened to "Disable when two keys are held down"

52

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)

59

Can be slightly shortened to "Use system bell when locking keys are toggled"

65

Can be shortened to "Show notification when modifier or locking keys are used"

kcms/access/package/contents/ui/MouseNavigation.qml
32

Change to "Enable using keyboard number pad to move cursor"

36

Add spacing after this

kcms/access/package/contents/ui/ScreenReader.qml
31

In this case, just put the whole text in the checkbox:

[ ] Enable screen reader
[ Launch Orca Configuration...]
37

What's Orca? Isn't that a kind of whale? :p Make change the text to be "Configure Orca Screen Reader..."

38

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.

kcms/access/package/contents/ui/main.qml
38

should be more descriptive, like sidebarScrollView or something

51

needs to force focus to the scrollview when clicked: onClicked: listView.forceActiveFocus();

54

title should already be translated; no need to put this in an i18n() call

78

Shouldn't these be i18n() calls?

This revision now requires changes to proceed.Dec 2 2019, 5:07 PM
tcanabrava updated this revision to Diff 71005.Dec 6 2019, 11:52 AM
tcanabrava marked an inline comment as done.
  • Typos and onToggled instead of onCheckStateChanged
  • Better UX / Texts
  • Text updates
  • Remove ActivationGestures page, Fix orca information
tcanabrava updated this revision to Diff 71008.Dec 6 2019, 12:37 PM
  • Fix localization calls

Updated texts by ngraham sugestions.

kcms/access/package/contents/ui/ActivationGestures.qml
50 ↗(On Diff #70754)

Agree. This is a 1 to 1 translation of the old KCM. I'm doing the changes you requested.

kcms/access/package/contents/ui/ModifierKeys.qml
52

I can't correct you here, those where the original texts and since haven't got a clue on what they are talking about I choose not to change them.

ngraham requested changes to this revision.Dec 6 2019, 2:15 PM

Now the KCM doesn't load:

"file:///home/nate/kde/usr/share/kpackage/kcms/kcmaccess/contents/ui/main.qml" 
 "Error loading QML file.\n76: ListElement: cannot use script for property value\n"

Also can you mark the inline comments as done once they're addressed? That makes it easier to review. Thanks!

kcms/access/package/contents/ui/ModifierKeys.qml
52

since haven't got a clue on what they are talking about

That's a great sign that they're in desperate need of being changed. :)

This revision now requires changes to proceed.Dec 6 2019, 2:15 PM
ervin requested changes to this revision.Dec 12 2019, 8:15 AM
ervin added a subscriber: ervin.

I think we discussed this already, but I'm not sure so before it gets lost... :-)

Please consider using both key and name for your kcfg items, this allows to both respect the past storage format and provide nicer property names on the C++/QML side. Currently the storage used names are leaking to the internal API.

cblack added a subscriber: cblack.Tue, Jan 7, 3:13 AM

The QML code could use some help in the style department. Some changes I would do:

  1. Make ID separate from other properties
  2. Chunk together related properties in a consistent manner

https://doc.qt.io/qt-5/qml-codingconventions.html and https://community.kde.org/Plasma/QMLStyle are relevant here.

Also, I wonder if it would be better to spin out each tab into a KCM of its own instead of retaining a single Accessibility KCM as a catch-all for accessibility stuff that doesn't go anywhere else.

kcms/access/CMakeLists.txt
17–18

What purpose does this serve? It looks like you're simply printing a list of source files with a Portuguese header.

kcms/access/kcmaccess.cpp
199

You could consider using glib for a stable ABI instead of invoking a command line that could change at any time. Also would reduce dependencies for most distros, as glib's command line tools are often packaged separately from glib itself.

kcms/access/package/contents/ui/ModifierKeys.qml
99

Redundant semicolon; there's only one statement on this line.

kcms/access/package/contents/ui/ScreenReader.qml
36

There should be an ellipsis here since this is opening another configuration window.

tcanabrava marked an inline comment as done.Wed, Jan 8, 11:44 AM

I think we discussed this already, but I'm not sure so before it gets lost... :-)

Please consider using both key and name for your kcfg items, this allows to both respect the past storage format and provide nicer property names on the C++/QML side. Currently the storage used names are leaking to the internal API.

Done.

kcms/access/CMakeLists.txt
17–18

stray debug. fixed.

kcms/access/kcmaccess.cpp
199

care to elaborate? This is old code and I haven't touched it.

tcanabrava updated this revision to Diff 73057.Wed, Jan 8, 12:00 PM
tcanabrava marked 5 inline comments as done.
  • ListElement does not handle i18n calls, use raw array
  • Dont complain about null background - Failing on Qt 5.14
  • Add Key entries to the kconfigxt generator
  • ArtsBell* -> CustomBell*
  • kModifiers* -> KeyboardModifiers*
  • Rename mouse settings
  • Remove deprecated call
  • Fix nitpicks
  • Fix Qml Style
tcanabrava marked 4 inline comments as done.Wed, Jan 8, 12:01 PM
tcanabrava added inline comments.
kcms/access/package/contents/ui/main.qml
54

this was a ListElement Q_NOOP_TR issue

tcanabrava updated this revision to Diff 73058.Wed, Jan 8, 1:11 PM
  • Use QFileDialog to look for audio files
broulik added a subscriber: broulik.Wed, Jan 8, 1:33 PM
broulik added inline comments.
kcms/access/kcmaccess.cpp
144–151

Are you sure this shouldn't be something like

modifiers & (ScrollMask | LockMask | NumMask)
175

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

183–187

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.

247–248

Don't ever use nested event loops in QML. You can use QtQuick.Dialogs FileDialog for that.

kcms/access/kcmaccess.desktop
9

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.

kcms/access/kcmaccess.h
41

You're not writing to this from QML, doesn't need a WRITE accessor

42

Where is this being used?

53–57

We typically use Q_INVOKABLE for methods exported to QML

61

Make this private and no slot

kcms/access/package/contents/ui/Bell.qml
55

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.

63

Typically a "browse" button is just a folder icon. (Don't forget a ToolTip and Accessible.name)

90

Why are you writing to a different property than you check for immutability?

106

onToggled

114

Use onAccepted

123

Use onValueModified

kcms/access/package/contents/ui/KeyboardFilters.qml
58

Add some i18n context i18nc("Use system bell when a key is pressed", "...")
Perhaps also an Accessible.name

107

Use onValueModified (and everywhere else)

kcms/access/package/contents/ui/ScreenReader.qml
45

This looks like the perfect use case for an InlineMessage?

tcanabrava updated this revision to Diff 73274.Sat, Jan 11, 2:00 PM
tcanabrava marked 16 inline comments as done.
  • ListElement does not handle i18n calls, use raw array
  • Dont complain about null background - Failing on Qt 5.14
  • Add Key entries to the kconfigxt generator
  • ArtsBell* -> CustomBell*
  • kModifiers* -> KeyboardModifiers*
  • Rename mouse settings
  • Remove deprecated call
  • Fix nitpicks
  • Fix Qml Style
  • Use QFileDialog to look for audio files
  • Fix Binary checks
  • Mark as Q_INVOKABLE, not Q_SCRIPTABLE
  • Use QtQuick Dialogs for the audio file
  • Fix orca calls
  • Use Label instead of placeholder text
  • Fix icon & text for search audio files
  • Fix Triggers
  • Trigger orcaInstalled() only when entering ScreenReader
  • Add explanation for magic number
  • Add transient window for the KNofityConfigWidget dialog
  • Fix KConfig Misuse
  • Explain property
  • Audible Bell Layout
  • Add string context
cblack added inline comments.Sun, Jan 12, 1:19 AM
kcms/access/kcmaccess.cpp
199

In that case, I can submit a separate patch to use glib instead of the gsettings command line interface.

If you're going to do this faux sidebar thing, why not just split the accessibility KCM into multiple KCMs so it can use the proper system settings sidebar?