Depends on D8641.
| No Linters Available |
| No Unit Test Coverage |
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.
| kcms/launch/launchfeedback.cpp | ||
|---|---|---|
| 169 | how does plasmashell get notified about new TaskbarButtonSettings? | |
| kcms/launch/package/contents/ui/main.qml | ||
| 22 | not used | |
| 87 | once the user clicks the spinbox this binding will break. When defaults is clicked, how does this update? | |
| 88 | you might want to see valueModified | |
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 :)
| kcms/launch/launchfeedback.cpp | ||
|---|---|---|
| 169 | KDirWatch on the rc file iirc Note the old business logic doesn't have any anything more there. | |
| kcms/launch/package/contents/ui/main.qml | ||
|---|---|---|
| 26 | 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 | |
Can you add "Fixes T7284" so this it will close that Maniphest task once this goes in?
| kcms/launch/package/contents/ui/main.qml | ||
|---|---|---|
| 49 | As discussed on https://phabricator.kde.org/T7284#116912, could we change "Startup indication timeout" to "Stop animation after" ? | |
| kcms/launch/package/contents/ui/main.qml | ||
|---|---|---|
| 69 | Will do former. | |
| 87 |
After fixing the binding bug: Via kcm.* signal emission, defaults() calls the setters which emit as needed. | |
| 88 | 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. | |
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.
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.
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!
These are the good examples of what I think it should be:
TITLE [ ] Verb/action explanation
TITLE
IF
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:
[seconds]
The alignment is a little off in this text box but I hope it makes sense.
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.
Given you're now familiar with this code, can you look through:
Done. Two reassigned/retitled, one closed.
| kcms/launch/package/contents/ui/main.qml | ||
|---|---|---|
| 31–40 | 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. | |
| kcms/launch/package/contents/ui/main.qml | ||
|---|---|---|
| 31–40 | 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). | |
Sorry for the noise.
This is now ready from my end, except for this build warning:
AutoMoc warning:
"/home/eike/devel/src/kde/workspace/plasma-desktop/kcms/launch/launchfeedback.cpp"
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.
