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 19389 Build 19407: 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 | ||
---|---|---|
30 | 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. | |
31 | Can be shortened to "Use gestures for activating sticky and slow keys" | |
51 | 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. | |
70 | 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? |
- 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 | ||
---|---|---|
51 | 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. |
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 |
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 | 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 | ||
190 | 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. |
- 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 | ||
---|---|---|
54 | this was a ListElement Q_NOOP_TR issue |
kcms/access/kcmaccess.cpp | ||
---|---|---|
140–147 | Are you sure this shouldn't be something like modifiers & (ScrollMask | LockMask | NumMask) | |
170 | 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 | |
178 | This dialog doesn't have a transient parent now. | |
228–231 | 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 | You're not writing to this from QML, doesn't need a WRITE accessor | |
42 | Where is this being used? | |
51–52 | We typically use Q_INVOKABLE for methods exported to QML | |
56 | 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", "...") | |
107 | Use onValueModified (and everywhere else) | |
kcms/access/package/contents/ui/ScreenReader.qml | ||
45 | 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 | ||
---|---|---|
190 | 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 | Redo the layout do be more like this: Audible bell: [] Enable [] Custom sound file: [ ] | |
55 | Remove this Kirigami.FormData.label; the checkbox above already has the same label | |
145 | 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 | 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 | 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 | It was used in the old code, I forgot to add in the Ui. | |
kcms/access/package/contents/ui/Bell.qml | ||
90 | 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. |