This rewrites the Accessibility KCM to use Qml and the new Managed
Config classes.
Details
- Reviewers
ngraham ervin - Maniphest Tasks
- T7261: Accessibility
Diff Detail
- Repository
- R119 Plasma Desktop
- Branch
- kcm_acess
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 19542 Build 19560: arc lint + arc unit
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 | ||
38 ↗ | (On Diff #70754) | Can be shortened to just "Lock" |
45 ↗ | (On Diff #70754) | Can be shortened to "Disable when two keys are held down" |
51 ↗ | (On Diff #70754) | 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) |
58 ↗ | (On Diff #70754) | Can be slightly shortened to "Use system bell when locking keys are toggled" |
64 ↗ | (On Diff #70754) | Can be shortened to "Show notification when modifier or locking keys are used" |
kcms/access/package/contents/ui/MouseNavigation.qml | ||
31 ↗ | (On Diff #70754) | Change to "Enable using keyboard number pad to move cursor" |
35 ↗ | (On Diff #70754) | Add spacing after this |
kcms/access/package/contents/ui/ScreenReader.qml | ||
30 ↗ | (On Diff #70754) | In this case, just put the whole text in the checkbox: [ ] Enable screen reader [ Launch Orca Configuration...] |
36 ↗ | (On Diff #70754) | What's Orca? Isn't that a kind of whale? :p Make change the text to be "Configure Orca Screen Reader..." |
37 ↗ | (On Diff #70754) | 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 | ||
37 ↗ | (On Diff #70754) | should be more descriptive, like sidebarScrollView or something |
50 ↗ | (On Diff #70754) | needs to force focus to the scrollview when clicked: onClicked: listView.forceActiveFocus(); |
53 ↗ | (On Diff #70754) | title should already be translated; no need to put this in an i18n() call |
77 ↗ | (On Diff #70754) | Shouldn't these be i18n() calls? |
- Typos and onToggled instead of onCheckStateChanged
- Better UX / Texts
- Text updates
- Remove ActivationGestures page, Fix orca information
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 | ||
51 ↗ | (On Diff #70754) | 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. |
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 | ||
---|---|---|
51 ↗ | (On Diff #70754) |
That's a great sign that they're in desperate need of being changed. :) |
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.
The QML code could use some help in the style department. Some changes I would do:
- Make ID separate from other properties
- 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–19 ↗ | (On Diff #70754) | 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 | ||
191 ↗ | (On Diff #70754) | 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 ↗ | (On Diff #70754) | Redundant semicolon; there's only one statement on this line. |
kcms/access/package/contents/ui/ScreenReader.qml | ||
36 ↗ | (On Diff #70754) | There should be an ellipsis here since this is opening another configuration window. |
- 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
kcms/access/package/contents/ui/main.qml | ||
---|---|---|
53 ↗ | (On Diff #70754) | this was a ListElement Q_NOOP_TR issue |
kcms/access/kcmaccess.cpp | ||
---|---|---|
140–147 ↗ | (On Diff #70754) | Are you sure this shouldn't be something like modifiers & (ScrollMask | LockMask | NumMask) |
171 ↗ | (On Diff #70754) | 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 |
179 ↗ | (On Diff #70754) | This dialog doesn't have a transient parent now. |
229–232 ↗ | (On Diff #70754) | 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. | |
kcms/access/kcmaccess.h | ||
41 ↗ | (On Diff #70754) | You're not writing to this from QML, doesn't need a WRITE accessor |
42 ↗ | (On Diff #70754) | Where is this being used? |
51–52 ↗ | (On Diff #70754) | We typically use Q_INVOKABLE for methods exported to QML |
56 ↗ | (On Diff #70754) | Make this private and no slot |
kcms/access/package/contents/ui/Bell.qml | ||
55 ↗ | (On Diff #70754) | 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 ↗ | (On Diff #70754) | Typically a "browse" button is just a folder icon. (Don't forget a ToolTip and Accessible.name) |
90 ↗ | (On Diff #70754) | Why are you writing to a different property than you check for immutability? |
106 ↗ | (On Diff #70754) | onToggled |
114 ↗ | (On Diff #70754) | Use onAccepted |
123 ↗ | (On Diff #70754) | Use onValueModified |
kcms/access/package/contents/ui/KeyboardFilters.qml | ||
58 ↗ | (On Diff #70754) | Add some i18n context i18nc("Use system bell when a key is pressed", "...") |
107 ↗ | (On Diff #70754) | Use onValueModified (and everywhere else) |
kcms/access/package/contents/ui/ScreenReader.qml | ||
45 ↗ | (On Diff #70754) | This looks like the perfect use case for an InlineMessage? |
- 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
kcms/access/kcmaccess.cpp | ||
---|---|---|
191 ↗ | (On Diff #70754) | 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?
Almost there!
kcms/access/package/contents/ui/Bell.qml | ||
---|---|---|
54 ↗ | (On Diff #70754) | Redo the layout do be more like this: Audible bell: [] Enable [] Custom sound file: [ ] |
55 ↗ | (On Diff #70754) | Remove this Kirigami.FormData.label; the checkbox above already has the same label |
145 ↗ | (On Diff #70754) | This isn't a grammatically correct sentence ("bell" is inappropriately used as a verb). How about: "Trigger when an accessibility feature is activated with a gesture" |
kcms/access/package/contents/ui/main.qml | ||
57 ↗ | (On Diff #70754) | This also needs a height set so that it fills the full height available in the KCM. probably anchors.fill: parent would be appropriate |
kcms/access/kcmaccess.cpp | ||
---|---|---|
140–147 ↗ | (On Diff #70754) | good catch. |
kcms/access/kcmaccess.desktop | ||
9 | Can you elaborate on that? I don't really know what you mean. | |
kcms/access/kcmaccess.h | ||
42 ↗ | (On Diff #70754) | It was used in the old code, I forgot to add in the Ui. |
kcms/access/package/contents/ui/Bell.qml | ||
90 ↗ | (On Diff #70754) | changed the names on the kcfg file and forgot to update here. it's a bit annoying that part of the kconfig is string based and other part is proper code. |