D28744: Rewrite of the global shortcuts kcm

This revision was automatically updated to reflect the committed changes. Closed by commit R119:1ee6660ceb62: Rewrite of the global shortcuts kcm (authored by davidre). REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=81441=81607 REVISION

D28744: Rewrite of the global shortcuts kcm

ngraham accepted this revision. ngraham added a comment. My spacebar shortcut issue will be fixed with D29292 . I think this deserves to get into Plasma 5.19, and the sooner the better so there's no rush before the deadline. REPOSITORY R119 Plasma

D28744: Rewrite of the global shortcuts kcm

ngraham added a comment. In D28744#659600 , @davidre wrote: > In D28744#659447 , @ngraham wrote: > > > I find that I'm not able to set shortcuts involving the space key. The key sequence get

D28744: Rewrite of the global shortcuts kcm

davidre added a comment. In D28744#659447 , @ngraham wrote: > I find that I'm not able to set shortcuts involving the space key. The key sequence get repeated twice in the button and the Apply button ever becomes enabled. Can you reproduce?

D28744: Rewrite of the global shortcuts kcm

ngraham added a comment. I find that I'm not able to set shortcuts involving the space key. The key sequence get repeated twice in the button and the Apply button ever becomes enabled. Can you reproduce? REPOSITORY R119 Plasma Desktop BRANCH kcmkeys2 (branched from master) REVISION

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 81441. davidre added a comment. - Reinstate the pointingHand mouseAreas - Make delegate automatically expand if it's the only one REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=81435=81441 BRANCH

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 81435. davidre added a comment. - Make the delegate use states REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=81305=81435 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

ngraham accepted this revision. ngraham added a comment. UI approved :) REPOSITORY R119 Plasma Desktop BRANCH kcmkeys2 (branched from master) REVISION DETAIL https://phabricator.kde.org/D28744 To: davidre, #vdg, #plasma, ngraham Cc: GB_2, broulik, davidedmundson, nicolasfella,

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 81305. davidre added a comment. rebasae REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=81304=81305 BRANCH kcmkeys2 (branched from master) REVISION DETAIL https://phabricator.kde.org/D28744

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 81304. davidre added a comment. - Use model.* properties - Use placeholder message - Still don't know why model properties are undefined when switching components REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 81016. davidre added a comment. - Don't drop installing scheme files REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80767=81016 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

ngraham added a comment. Also you can now use `Kirigami.PlaceholderMessage` in this diff instead of making the messages manually using level 3 Headings. REPOSITORY R119 Plasma Desktop BRANCH kcmkeys2 (branched from master) REVISION DETAIL https://phabricator.kde.org/D28744 To:

D28744: Rewrite of the global shortcuts kcm

ngraham added a comment. JFYI once this is ready to land, before doing so, please do an `arc amend` locally to pick up the changes in the description that I've made and ensure that all those bugs get closed! REPOSITORY R119 Plasma Desktop BRANCH kcmkeys2 (branched from master)

D28744: Rewrite of the global shortcuts kcm

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. Boom. REPOSITORY R119 Plasma Desktop BRANCH kcmkeys2 (branched from master) REVISION DETAIL https://phabricator.kde.org/D28744 To: davidre, #vdg, #plasma, ngraham Cc: GB_2,

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80767. davidre added a comment. - Fix defaults - Add pending deletion thing REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80675=80767 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre added a comment. In D28744#652925 , @ngraham wrote: > Here's how it looks with the old KCM: F8249724: Screenshot_20200420_125405.png > > So my KDE Daemon category has one entry in French for

D28744: Rewrite of the global shortcuts kcm

ngraham added a comment. Here's how it looks with the old KCM: F8249724: Screenshot_20200420_125405.png So my KDE Daemon category has one entry in French for some odd reason. And the two KRunner entries are indeed still there and different, not

D28744: Rewrite of the global shortcuts kcm

davidre added a comment. In D28744#652854 , @ngraham wrote: > Yay. Almost there! I just see a few more things: > > 1. If I delete a whole item from the left-most list, there's no obvious way to get it back (what if I delete a system entry by

D28744: Rewrite of the global shortcuts kcm

GB_2 added a comment. Nitpick: put the "Add Application..." button on the left (T10384 ). REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D28744 To: davidre, #vdg, #plasma Cc: GB_2, broulik, davidedmundson, nicolasfella,

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80675. davidre added a comment. - Implement Kai's idea for importing - Fix importing REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80673=80675 BRANCH kcmkeys2 (branched from master) REVISION

D28744: Rewrite of the global shortcuts kcm

ngraham added a comment. Yay. Almost there! I just see a few more things: 1. If I delete a whole item from the left-most list, there's no obvious way to get it back (what if I delete a system entry by accident?) 2. The Defaults button is present, but always disabled 3. I see

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80673. davidre added a comment. - Fix scrollview and use level 3 heading - use better string REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80672=80673 BRANCH kcmkeys2 (branched from master)

D28744: Rewrite of the global shortcuts kcm

davidre marked an inline comment as done. davidre added a comment. In D28744#652703 , @ngraham wrote: > Clicking the Apply button makes System Settings crash for me: Fixed now. REPOSITORY R119 Plasma Desktop REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80672. davidre added a comment. - Fix errors and leave error message disabled REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80631=80672 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

ngraham added a comment. Clicking the Apply button makes System Settings crash for me: P591 System Setting scrash log INLINE COMMENTS > main.qml:118 > +icon.width: Kirigami.Units.iconSizes.small > +

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80631. davidre added a comment. Use Nate's string REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80629=80631 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80629. davidre added a comment. - Fix index reset when clicking reset button and use own property for shortcuts listview currentIndex was reset to 0 when resetting, now warnings are generated when switching components need to figure out why

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80622. davidre added a comment. - Add tooltip to remove toolbutton REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80620=80622 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80620. davidre added a comment. Make it build again REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80553=80620 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

ngraham added inline comments. INLINE COMMENTS > shortcutsmodel.cpp:440 > +} > +const QString = m_components[index.row()].uniqueName); > +auto watcher = new > QDBusPendingCallWatcher(m_globalAccelInterface->getComponent(uniqueName)); compile error REPOSITORY R119 Plasma Desktop

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80553. davidre added a comment. - Make loading async - Make everything async REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80405=80553 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80405. davidre added a comment. - Use less DBus calls for loading REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80398=80405 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre added inline comments. INLINE COMMENTS > broulik wrote in shortcutsmodel.cpp:38 > Hmm... > how about > > QStringList actionId; > actionId.reserve(4); This actually crashes now REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D28744 To: davidre,

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80398. davidre added a comment. - Fix defaults REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80387=80398 BRANCH kcmkeys2 (branched from master) REVISION DETAIL https://phabricator.kde.org/D28744

D28744: Rewrite of the global shortcuts kcm

ngraham added a comment. This is looking really good. Other than my previous comments (most of which have been resolved, yay) I have just nitpicks left, really. INLINE COMMENTS > ShortcutActionDelegate.qml:165 > +onClicked: >

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80387. davidre added a comment. - Remove components as good as the current kcm REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80224=80387 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80224. davidre added a comment. - Add section checkbox REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80216=80224 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80216. davidre marked an inline comment as done. davidre added a comment. - Use pointing hand cursor - Rename error signals to errorOccured REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80198=80216

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80198. davidre added a comment. - Update the delegate REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80137=80198 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80137. davidre marked 3 inline comments as done. davidre added a comment. - Use QCollator and one more coding style REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80136=80137 BRANCH kcmkeys2

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80136. davidre marked an inline comment as done. davidre added a comment. - Set transient window for application dialog REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80135=80136 BRANCH kcmkeys2

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80135. davidre added a comment. - redo exportActive and exportWarning bindings REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=80088=80135 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre marked 5 inline comments as done. davidre added inline comments. INLINE COMMENTS > broulik wrote in main.qml:94 > Is `contentItem` the default property? Yes REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D28744 To: davidre, #vdg, #plasma Cc: broulik,

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80088. davidre added a comment. - Search when entering text and correctly color delegates - use fallback if action doesn't have a friendly name - Remove friend declaration REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE

D28744: Rewrite of the global shortcuts kcm

davidre marked an inline comment as done. davidre added inline comments. INLINE COMMENTS > shortcutsmodel.h:59 > +friend FilteredShortcutsModel; > +enum Roles { > +SectionRole = Qt::UserRole, It does in filterAcceptRows to not have to go through data each time REPOSITORY R119

D28744: Rewrite of the global shortcuts kcm

davidre added inline comments. INLINE COMMENTS > shortcutsmodel.h:59 > +friend FilteredShortcutsModel; > +enum Roles { > +SectionRole = Qt::UserRole, Nevermind it goes through data, apparently I forgot that I changed that :) REPOSITORY R119 Plasma Desktop REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre added inline comments. INLINE COMMENTS > filteredmodel.cpp:41 > +bool displayMatches = > index.data(Qt::DisplayRole).toString().contains(m_filter, > Qt::CaseInsensitive); > +if (!source_parent.isValid() || displayMatches) { > +return displayMatches; If it's a toplevel

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 80074. davidre marked 33 inline comments as done. davidre added a comment. Minor things and codestyle REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=79940=80074 BRANCH kcmkeys2 (branched from master)

D28744: Rewrite of the global shortcuts kcm

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 = >

D28744: Rewrite of the global shortcuts kcm

davidre edited subscribers, added: davidedmundson; removed: crossi. davidre added a comment. Answering general comments first: In D28744#647378 , @ngraham wrote: > - When a shortcut is to launch an app, and that shortcut's name would be

D28744: Rewrite of the global shortcuts kcm

crossi added inline comments. INLINE COMMENTS > shortcutsmodel.h:83 > +void save(); > +bool needsSave(); > +bool isDefault(); should be const > shortcutsmodel.h:84 > +bool needsSave(); > +bool isDefault(); > + should be const REPOSITORY R119 Plasma Desktop REVISION

D28744: Rewrite of the global shortcuts kcm

ngraham added inline comments. INLINE COMMENTS > ShortcutActionDelegate.qml:67 > +Layout.alignment: Qt.AlignRight > +icon.name: "arrow-down" > +onClicked: { use `expand` > ShortcutActionDelegate.qml:94 > +

D28744: Rewrite of the global shortcuts kcm

nicolasfella added a comment. When I open the KCM and type 'Krun' + Enter I find KRunner as expected, but I still need to click on the KRunner item to actually see the shortcuts. When I then type 'Yaku' + Enter Yakuake is shown correctly in the results, but clicking on the entry does not

D28744: Rewrite of the global shortcuts kcm

nicolasfella added a comment. I always get two horizontal scrollbars, no matter the window size: F8234349: Screenshot_20200413_212457.png The first Choqok shortcut is missing a name, not sure if that's an issue here or in Choqok F8234357:

D28744: Rewrite of the global shortcuts kcm

nicolasfella added a comment. Cool stuff! INLINE COMMENTS > kcm_keys.cpp:110 > +{ > +auto includedComponents = > m_shortcutsModel->match(m_shortcutsModel->index(0, 0), > ShortcutsModel::CheckedRole, true, -1); > +qCDebug(KCMKEYS) << "Exporting to " << url.toLocalFile(); const >

D28744: Rewrite of the global shortcuts kcm

ngraham added a comment. This is excellent work. A radical improvement over the existing KCM. In addition to the inline comments, here are some more general ones: - When a shortcut is to launch an app, and that shortcut's name would be identical to the app name, maybe we should

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 79940. davidre added a comment. Actually show the dialog REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=79938=79940 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 79938. davidre added a comment. Do not use dialog.exec() REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=79907=79938 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 79907. davidre added a comment. forgot to implement defaults() REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=79831=79907 BRANCH kcmkeys2 (branched from master) REVISION DETAIL

D28744: Rewrite of the global shortcuts kcm

ngraham added a comment. Super-awesome! INLINE COMMENTS > ShortcutActionDelegate.qml:54 > +spacing: Kirigami.Units.smallSpacing > +QQC2.Label { > +Layout.fillWidth: true I might right-align the text for improved visual parse-ability

D28744: Rewrite of the global shortcuts kcm

davidre updated this revision to Diff 79831. davidre added a comment. enabled REPOSITORY R119 Plasma Desktop CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28744?vs=79829=79831 BRANCH kcmkeys2 (branched from master) REVISION DETAIL https://phabricator.kde.org/D28744

D28744: Rewrite of the global shortcuts kcm

davidre added a comment. - I omitted the remove button for now because removing things correctly from KGlobalAccel is hard and any program could add itself again, but I could add it again. - The expanded ui needs some work. Open for any idea there. REPOSITORY R119 Plasma Desktop

D28744: Rewrite of the global shortcuts kcm

davidre created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. davidre requested review of this revision. REVISION SUMMARY This is a total rewrite of the global shortcuts kcm from scratch. It uses an abstract item model backend with a qml frontend.