Adds the complete fileName of the autostart entries as tooltips.
Collapse home to ~ in command column.
Details
Details
- Add a desktop entry
- Edit a desktop entry
- Disable a desktop entry
- Remove a desktop entry
- Make a destktop entry run only in KDE
- Add a script Item with symlink
- Add a script Item without symlink
- Change the startup choice for a script Item
- Add twice the same script to the same startup choice
- Remove a script item
Before:
After:
Diff Detail
Diff Detail
- Repository
- R119 Plasma Desktop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
| kcms/autostart/autostartitem.cpp | ||
|---|---|---|
| 60 | Remove the space after the opening parenthesis on those three lines. It's much better like this BTW, the loop wasn't indeed buying us much for just three values. | |
| kcms/autostart/autostartmodel.cpp | ||
| 117 | Heh, so we're playing who's the most stubborn here? I still find that "switch not saying it's a static_cast" really ugly and non idiomatic. | |
| 123 | I'd still advise getting rid of the switch but if it doesn't disappear at least move that to the "default:" case to silence potential warning on type's non exhaustion. But really... static_cast needs to appear please. Alternatively to the if I was proposing you could also do: switch (index) {
case XdgAutoStart:
case XdgScripts:
case PlasmaShutdown:
case PlasmaStart:
return static_cast<AutostartEntrySource>(index);
default:
Q_UNREACHABLE();
}
return XdgAutoStart;At least it'd be DRY and it'd be more obvious we got a cast... | |
Comment Actions
One last nitpick
| kcms/autostart/autostartmodel.cpp | ||
|---|---|---|
| 122 | Please remove the spurious space before ( | |
| kcms/autostart/autostart.cpp | ||
|---|---|---|
| 49–50 | Put on new line | |
| 99–100 | const bool | |
| 101 | Coding style: if (enabled) {
...
} | |
| 186 | In preparation for a move to QML, can we please not exec() | |
| 198 | Coding style | |
| 217 | You probably want to loop for (int i = first; i <= last; ++i) { | |
| 217 | KFileItem kfi(QUrl::fromLocalFile(fileName)); | |
| 219 | Don't always ignore those ranges, we want to handle this model stuff properly | |
| 229 | qobject_cast | |
| kcms/autostart/autostart.h | ||
| 64 | Capitalize Changed | |
| 67–71 | const | |
| kcms/autostart/autostartmodel.cpp | ||
| 177 | but why? | |
| 262 | bool | |
| 292 | Coding style: } else { | |
| kcms/autostart/autostartmodel.h | ||
| 53 | Why not just use Qt::DisplayRole? | |
| kcms/autostart/autostart.cpp | ||
|---|---|---|
| 186 | Yes. | |
| 217 | rows inserted gives you a range of [first,last] which was added | |
| 229 | Ok | |
| kcms/autostart/autostartmodel.h | ||
| 53 | Why define DisplayRole as dedicated enum entry, rather than just using Qt::DisplayRole in the code everywhere | |
| kcms/autostart/autostartmodel.h | ||
|---|---|---|
| 53 | I see, I meant my code to have all Roles used explicitly declared here rather than relying on the developer knowing about Qt::DisplayRole. | |
| kcms/autostart/autostartmodel.h | ||
|---|---|---|
| 53 | Weeell... knowing about Qt::DisplayRole is kind of prerequisite to making your own model. :-) | |
| kcms/autostart/autostart.cpp | ||
|---|---|---|
| 101 | I think Kai's point was also that the curly braces are required for the if and the else | |
| 186 | Yes, otherwise you wouldn't get a modal dialog. | |
| 215–216 | This curly brace needs to be on the previous line. | |
| 217 | Good point, this still needs to be addressed. | |
| 219 | Yes, similar situation to rowInserted here. | |
| 229 | Yes, it used to also inherit QObject (which wasn't a great idea) but not anymore. | |
| kcms/autostart/autostart.h | ||
| 64 | Still needs being addressed | |
| kcms/autostart/autostartmodel.cpp | ||
| 178 | Makes me realize: are we sure it will always be the first entry? I'm not sure we can guarantee that over time. | |
| kcms/autostart/autostart.cpp | ||
|---|---|---|
| 167–169 | Space before * not after, or just use auto | |
| 186–192 | ditto | |
| 214 | Actually I think I'd Q_ASSERT(!parent.isValid()) | |
| 215 | This Q_UNUSED is now used ;-) | |
| 275 | Space before * not after, or use auto | |
| 295 | Since you touched the line, please drop the spaces between the parentheses | |
| 303 | Since you touched the line, please drop the spaces between the parentheses | |
| 320 | Since you touched the line please fix the * and { positions | |
| kcms/autostart/autostart.h | ||
| 64 | Still needs being addressed. :-) | |
| kcms/autostart/autostartmodel.cpp | ||
| 131 | Space before * not after | |
| 178 | Still needs being addressed. | |
| kcms/autostart/autostartmodel.h | ||
| 50 | Since you touched the line, please drop the spaces between the parentheses | |
| 82 | Space before * not after | |
| kcms/autostart/autostartmodel.h | ||
|---|---|---|
| 53 | As you want. | |