Port launch feedback KCM to Qt Quick and implement VDG redesign

Authored by hein on Nov 20 2017, 10:40 AM.


  • Implements VDG mockup M112/364
    • But using "Task Manager" naming for consistency instead of "Taskbar"
    • Additional UI tweak of using i18np for spinbox suffices
  • Switched from generic name "launch" to "launchfeedback"
    • This will require i18n pot and doc HTML renames to match
  • Original business logic is preserved as-is, but code is simplified and cleaned-up (coding style, etc.)

Depends on D8641.

Diff Detail

R119 Plasma Desktop
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
hein created this revision.Nov 20 2017, 10:40 AM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 20 2017, 10:40 AM

I don't see any QML files

hein updated this revision to Diff 22634.Nov 20 2017, 10:45 AM

Add missing package.

hein added a comment.Nov 20 2017, 10:46 AM

This is on git as branch kcm-redesign/launchfeedback.

hein added a comment.Nov 20 2017, 10:50 AM


Your Messages.sh doesn't include qml (copy the code in Marco's mouse diff)

Given you're now familiar with this code, can you look through:


(there's only 3) and see if this fixes them, or mark any as wontfix.


how does plasmashell get notified about new TaskbarButtonSettings?


not used


once the user clicks the spinbox this binding will break.

When defaults is clicked, how does this update?


you might want to see valueModified

hein added a comment.Nov 20 2017, 12:07 PM

Review comments: Some of those also apply to the fonts KCM rewrite I based this on, e.g. the broken bindings. Will fix tomorrow, thanks :)

hein added inline comments.Nov 20 2017, 12:08 PM

KDirWatch on the rc file iirc

Note the old business logic doesn't have any anything more there.

mart added inline comments.Nov 20 2017, 1:20 PM

i would like to have a base component for this (which admittedly doesn't exists yet as i need beforehand to merge the cursrs stuff which introduced the kcm components) tough for now would be just for "semantics" and future proofing

ngraham added a subscriber: ngraham.EditedNov 20 2017, 2:06 PM

Can you add "Fixes T7284" so this it will close that Maniphest task once this goes in?


As discussed on https://phabricator.kde.org/T7284#116912, could we change

"Startup indication timeout" to "Stop animation after" ?

ngraham added inline comments.Nov 20 2017, 2:10 PM

"Task Manager notification: [checkbox] Enable Task Manager notification" a bit redundant and overly wordy. How about the following?

"Task Manager notification: [checkbox]"
"[checkbox] Enable Task Manager notification"

Paging @abetts for better ideas. :)

I am ok with that idea. I agree that it is redundant.

hein marked 3 inline comments as done.Nov 21 2017, 8:38 AM
hein added inline comments.

Will do former.


When defaults is clicked, how does this update?

After fixing the binding bug: Via kcm.* signal emission, defaults() calls the setters which emit as needed.


valueModified is new in Qt Quick Controls 2.2 (Qt 5.9) and I can't import that because of versioning conflicts with the imports in Kirigami. There are ways to hack around this but they're too awful.

hein updated this revision to Diff 22681.Nov 21 2017, 8:40 AM
hein marked 3 inline comments as done.

Address all review comments.

hein added a comment.Nov 21 2017, 8:44 AM

Updated screenshot:

I have a question: As per the mockup, I have no colons after the labels. I'm not sure I like that. Thoughts? @abetts, @ngraham?

Can you add "Fixes T7284" so this it will close that Maniphest task once this goes in?

Will do when pushing.

I don't know if I am allowed to write here, so sorry if not ...
I just wanted to share my point of view (daily user).
While the first screens look good for me, the second one here seems to be very restless and so I wondered if I should share what I would like to see here.

  1. I think the labels with controls next to them need colons. It looks weird otherwise.
  2. We should vertically align the labels and their controls, just like @mmustac showed. That looks radically better.

Also as discussed in the VDG room this morning, there's the open question of whether we actually need a launch feedback KCM or whether these settings should be in the Task Manager's own settings.

hein added a comment.Nov 21 2017, 3:48 PM

The above exchange is seriously upsetting and frustrating to me. I implemented the VDG mockup, and now there's discussion about the design in my code review. Please get your act together and make sure the mockups are ready for implementing so the review process of code work isn't this painful.

I'm sorry @hein. I understand that this is pretty frustrating. Sometimes there's no substitute for seeing the actual implementation, I guess!

broulik added a comment.EditedNov 21 2017, 4:20 PM
In D8911#170262, @hein wrote:

Updated screenshot:

According to KDE HIG a check box must always have a label.

These are the good examples of what I think it should be:

TITLE [ ] Verb/action explanation


  • verb/explanation


  • verb/explanation

But never

Verb/Explanation [X ]

Verb/Explanation [X ] verb/explanation

Another Example:

Task Manager [X ] Enable task management globally after: [seconds]

Is also ok, or

Task Manager [X] Enable task management globaly after:


The alignment is a little off in this text box but I hope it makes sense.

This comment was removed by ngraham.

After a lot of design work in VDG, we all agreed on the following:

hein added a comment.Nov 22 2017, 4:01 PM

Thanks, will implement tomorrow!

hein updated this revision to Diff 22828.Nov 23 2017, 2:21 PM
  • Implement latest VDG mockup.

This simplifies the UI from two seperate timeout settings to one,
but in the config files we still have two. We write both, but the
code needs to make a decision which one to read and show in the
UI. I went with the cursor one. A seperately user-configured
task manager setting is therefore lost once changes are written
through this redesigned KCM.

  • Add missing i18n extraction for the QML file.
hein added a comment.Nov 23 2017, 2:24 PM

Updated screenshot:

Branch is re-pushed as well.

Visually, it looks great to me!

hein added a comment.Nov 24 2017, 8:13 AM

Given you're now familiar with this code, can you look through:

Done. Two reassigned/retitled, one closed.

anthonyfieroni added inline comments.

My personal note is not mandatory but this piece of code can be seen 2 times in C++ part and 1 in QML. Can we add it on C++ part and called here too.

hein added inline comments.Dec 3 2017, 11:54 AM

I hear you, but IMHO it's not really worth it to get inventive for some basic boilerplate. Down the line we just want generated code for this stuff (e.g. using KConfigXT more).

hein updated this revision to Diff 23325.Dec 3 2017, 12:06 PM
  • Port to SimpleKCM as base item
  • Fix a crash
hein added a comment.Dec 3 2017, 12:07 PM

Argh, I have no idea what happened to the diff ...

hein updated this revision to Diff 23326.Dec 3 2017, 12:07 PM

Try to fix diff

hein updated this revision to Diff 23327.Dec 3 2017, 12:08 PM

Try again

hein added a comment.Dec 3 2017, 12:09 PM

Sorry for the noise.

This is now ready from my end, except for this build warning:

AutoMoc warning:


The file includes the moc file "launchfeedback.moc", but does not contain a Q_OBJECT, Q_GADGET or Q_NAMESPACE macro.

I'm blind somehow and don't see why, the includes are correct.

mart accepted this revision.Feb 6 2018, 10:12 AM

I think it's about time to merge this in master

This revision is now accepted and ready to land.Feb 6 2018, 10:12 AM
This revision was automatically updated to reflect the committed changes.