D28744: Rewrite of the global shortcuts kcm
Kai Uwe Broulik
noreply at phabricator.kde.org
Tue Apr 14 09:41:02 BST 2020
broulik added a comment.
Very nice!
I really like the default shortcuts with checkboxes with additional ones to the right.
INLINE COMMENTS
> filteredmodel.cpp:40
> + const QModelIndex index = sourceModel()->index(source_row, 0, source_parent);
> + bool displayMatches = index.data(Qt::DisplayRole).toString().contains(m_filter, Qt::CaseInsensitive);
> + if (!source_parent.isValid() || displayMatches) {
`const bool`
> filteredmodel.cpp:41
> + bool displayMatches = index.data(Qt::DisplayRole).toString().contains(m_filter, Qt::CaseInsensitive);
> + if (!source_parent.isValid() || displayMatches) {
> + return displayMatches;
Why this `!source_parent.isValid()` check?
> filteredmodel.cpp:45
> +
> + if (index.parent().data(Qt::DisplayRole).toString().contains(m_filter, Qt::CaseInsensitive)) {
> + return true;
Shouldn't recursive filtering take care of this?
> filteredmodel.cpp:50
> + const auto &defaultShortcuts = index.data(ShortcutsModel::DefaultShortcutsRole);
> + for (const auto& shortcut : defaultShortcuts.value<QSet<QKeySequence>>()) {
> + if (shortcut.toString(QKeySequence::NativeText).contains(m_filter, Qt::CaseInsensitive)) {
Put the `.value<QSet<...>>` in the line above outside the `for`
> filteredmodel.h:26
> +
> +class FilteredShortcutsModel : public QSortFilterProxyModel {
> + Q_OBJECT
Coding style, `{` on next line
> filteredmodel.h:32
> +public:
> + FilteredShortcutsModel(QObject *parent);
> +
`explicit`
> filteredmodel.h:34
> +
> + bool filterAcceptsRow(int source_row, const QModelIndex & source_parent) const override;
> +
Coding style: `const QModelIndex &soure_parent`
> kcm_keys.cpp:51
> + KAboutData *about = new KAboutData(QStringLiteral("kcm_keys"), i18n("Global Shortcuts"),
> + QStringLiteral("1.0"), QString(), KAboutLicense::GPL);
> + about->addAuthor(i18n("David Redondo"), QString(), QStringLiteral("kde at david-redondo.de"));
I guess this can be 2.0 at this point :)
> kcm_keys.cpp:106
> +{
> + return m_lastError;
> +}
Not a huge fan of this `lastError` bookkeeping in the KCM
> kcm_keys.cpp:153
> +{
> + auto dialog = new KOpenWithDialog;
> + dialog->hideRunInTerminal();
Set a transient parent on the dialog like so:
void KCMKeys::addApplication(QQuickItem *ctx)
{
auto *dialog = new KOpenWithDialog;
if (ctx && ctx->window()) {
dialog->winId(); // so it creates windowHandle
dialog->windowHandle()->setTransientParent(QQuickRenderControl::renderWindowFor(ctx->window()));
dialog->setWindowModality(Qt::WindowModal);
}
...
}
Then call in QML like `onClicked: kcm.addApplication(this)`
Also, you might want to set `dialog->setAttribute(Qt::WA_DeleteOnClose);` so you don't need to `deleteLater()` yourself.
(I believe the `QQuickRenderControl` use may not be necessary when in the same process but I recall it behaving funky in `QQuickWidget` in some cases on Wayland otherwise)
> kcm_keys.cpp:159
> + const KService::Ptr service = dialog->service();
> + QString desktopFileName = service->entryPath().split('/').last();
> + if (m_shortcutsModel->match(m_shortcutsModel->index(0, 0), ShortcutsModel::ComponentRole, desktopFileName).isEmpty()) {
Perhaps use `KService::desktopEntryName`
> kcm_keys.h:51
> + Q_INVOKABLE void loadScheme(const QUrl &url);
> + Q_INVOKABLE QVariantList defaultSchemes();
> +
`const`
> kcm_keys.h:54
> + Q_INVOKABLE void addApplication();
> + Q_INVOKABLE QString keySequenceToString(const QKeySequence &keySequence);
> +
I presume `QKeySequence` is opaque to QML? Also, `const`
> ShortcutActionDelegate.qml:30
> + property int oldHeight: height
> + sourceComponent: ListView.isCurrentItem ? editRepresentation : compactRepresentation
> + ListView.onIsCurrentItemChanged: {
I find the transition between editing and display state somewhat jarring
> ShortcutActionDelegate.qml:58
> + if (model.activeShortcuts.length != 0) {
> + return model.display + ": " + model.activeShortcuts.map(s => kcm.keySequenceToString(s)).join(", ")
> + } else {
Please use `i18n`
> main.qml:33
> + enabled: kcm.lastError == ""
> + property bool exportActive: exportInfo.visible
> + ColumnLayout{
This appears to loop? `exportInfo`'s `visible` is bound to this
> main.qml:34
> + property bool exportActive: exportInfo.visible
> + ColumnLayout{
> + // otherwise we get scrollbars
Coding style: `ColumnLayout {`
> main.qml:36
> + // otherwise we get scrollbars
> + height: root.availableHeight - Kirigami.Units.smallSpacing
> + width: root.availableWidth
Huh?
> main.qml:40
> + Layout.fillWidth: true
> + visible: kcm.lastError != ""
> + text: kcm.lastError
Coding style, also `!==`
> main.qml:53
> + enabled: exportWarning.visible
> + function onNeedsSaveChanged () {
> + exportWarning.visible = kcm.needsSave
What Qt version was that changed, 5.14?
Also why not just bind?
> main.qml:87
> + Component.onCompleted: background.visible = true
> + Layout.preferredWidth: 300
> + Layout.fillHeight:true
`Kirigami.Units.gridUnit` and/or maybe proportional to the window width?
> main.qml:94
> + delegate: Kirigami.AbstractListItem {
> + RowLayout {
> + Kirigami.Icon {
Is `contentItem` the default property?
> main.qml:102
> + Layout.fillWidth: true
> + text: model.display
> + }
Make sure to replicate the color behavior on hover and when selected, right now text stays black with the blue selection background behind it
> main.qml:114
> + section.delegate: Kirigami.ListSectionHeader {
> + label: section
> + }
Perhaps you could put a checkbox here so one check all items in a section
> main.qml:141
> + icon.name: "list-add"
> + text: i18n("Add application...")
> + onClicked: {
Capitalize: "Add Application..."
How do I remove applications again, though?
> main.qml:157
> + icon.name: "document-export"
> + text:i18n("Export Scheme...")
> + onClicked: {
I kinda think this button should turn into a "Cancel Export" button when exporting? Not sure that closing the message widget is obvious enough to the user?
> main.qml:160
> + if (kcm.needsSave) {
> + exportWarning.visible = true
> + } else {
You're doing a `Connections` above already.
> main.qml:163
> + search.text = ""
> + kcm.filteredModel.filter = ""
> + exportInfo.visible = true
This should just be bound to the `search.text` and work automatically
> main.qml:164
> + kcm.filteredModel.filter = ""
> + exportInfo.visible = true
> + }
You seem to love being overly explicit with this? :P You have a binding above already...
Perhaps run the application with `QT_LOGGING_RULES="qt.qml.binding.removal.info=true"` to clean those up
> main.qml:224
> + }
> + importSheet.close()
> + }
Found it a bit odd that the overlaysheet suddenly closed as a result.
I would have expected clicking "Select File", choosing one, and then it being added to and selected in the combox list and then confirming this by clicking "Import"
> main.qml:30
> + id: root
> + implicitWidth: 1000
> + implicitHeight: 400
`Kirigami.Units.gridUnit`
> shortcutsmodel.cpp:38
> +{
> + QStringList actionId{"", "", "", ""};
> + actionId[KGlobalAccel::ComponentUnique] = componentUnique;
Hmm...
how about
QStringList actionId;
actionId.reserve(4);
> shortcutsmodel.cpp:60
> + m_components.clear();
> + QDBusReply<QList<QStringList>> componentsReply = m_globalAccelInterface->allMainComponents();
> + if (!componentsReply.isValid()) {
Do this asynchronously with `QDBusPendingCallWatcher`? Not sure if it really matters here
> shortcutsmodel.cpp:66
> + const auto components = componentsReply.value();
> + for(const auto &component : components) {
> + m_components.push_back(loadComponent(component));
Coding style, space after for `for (const auto &...)`
> shortcutsmodel.cpp:70
> + std::sort(m_components.begin(), m_components.end(), [](const Component &c1, const Component &c2){
> + return c1.type != c2.type ? c1.type < c2.type : c1.friendlyName < c2.friendlyName;
> + });
for comparing `friendlyName` use `QCollator`?
> shortcutsmodel.cpp:103
> + shortcut.friendlyName = actionFriendly;
> + QDBusReply<QList<int>> shortcutsReply = m_globalAccelInterface->shortcut(action);
> + if (!shortcutsReply.isValid()) {
I kinda feel this should be done on demand or asynchronously as it causes a significant load time of the KCM
> shortcutsmodel.cpp:130
> + std::sort(c.shortcuts.begin(), c.shortcuts.end(), [] (const Shortcut &s1, const Shortcut &s2) {
> + return s1.friendlyName < s2.friendlyName;
> + });
Use `QCollator`
> shortcutsmodel.cpp:140
> + if (shortcut.initialShortcuts != shortcut.activeShortcuts) {
> + QStringList actionId = buildActionId(component.uniqueName, component.friendlyName,
> + shortcut.uniqueName, shortcut.friendlyName);
`const`
> shortcutsmodel.cpp:194
> +{
> + if (row < 0 ) {
> + return QModelIndex();
Also check `if (column != 0) {`
(Also, coding style)
> shortcutsmodel.cpp:232
> +{
> + if (!index.isValid()) {
> + return QVariant();
`checkIndex()`?
> shortcutsmodel.cpp:251
> + }
> + default:
> + return QVariant();
No `default` case please in case we add new roles. Just `return` outside the `switch`, and below
> shortcutsmodel.cpp:260
> + case Qt::DecorationRole:
> + return QIcon::fromTheme(component.icon);
> + case SectionRole:
Do we want a `QIcon` here? I guess just returning an `iconName` and using a `Kirigami.Icon` in the UI should suffice
> shortcutsmodel.cpp:274
> +{
> + if (!checkIndex(index) || index.parent().isValid() || role != CheckedRole) {
> + return false;
Makes me wonder if `CheckedRole` should be the `Qt::EditableRole`
> shortcutsmodel.cpp:298
> +{
> + if (!checkIndex(index) || !index.parent().isValid()) {
> + return;
You can pass also `ParentIsInvalid` as flag to `checkIndex`
> shortcutsmodel.cpp:403
> + }
> + emit this->error(i18n("Error while communicating with the global shortcuts daemon"));
> +}
perhaps "service" instead of "daemon"?
> shortcutsmodel.h:54
> +
> +class ShortcutsModel : public QAbstractItemModel {
> + Q_OBJECT
Coding style, `{` on new line
> shortcutsmodel.h:58
> +public:
> + friend FilteredShortcutsModel;
> + enum Roles {
It doesnt appear to use any private members?
> shortcutsmodel.h:83
> + void save();
> + bool needsSave();
> + bool isDefault();
`const` and below
> shortcutsmodel.h:95
> +Q_SIGNALS:
> + void error(QString);
> +
`const QString &`, also perhaps `errorOccurred`?
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D28744
To: davidre, #vdg, #plasma
Cc: broulik, davidedmundson, nicolasfella, ngraham, iasensio, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200414/2897016e/attachment-0001.html>
More information about the Plasma-devel
mailing list