Adds the complete fileName of the autostart entries as tooltips.
Collapse home to ~ in command column.
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
- Repository
- R119 Plasma Desktop
- Branch
- D26934
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 24341 Build 24359: arc lint + arc unit
kcms/autostart/autostartitem.cpp | ||
---|---|---|
60 ↗ | (On Diff #78074) | 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 | ||
116 ↗ | (On Diff #78074) | 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. |
122 ↗ | (On Diff #78074) | 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... |
One last nitpick
kcms/autostart/autostartmodel.cpp | ||
---|---|---|
121 | Please remove the spurious space before ( |
kcms/autostart/autostart.cpp | ||
---|---|---|
182 ↗ | (On Diff #78074) | In preparation for a move to QML, can we please not exec() |
192 ↗ | (On Diff #78074) | Coding style |
231 ↗ | (On Diff #78074) | Don't always ignore those ranges, we want to handle this model stuff properly |
241 ↗ | (On Diff #78074) | qobject_cast |
261 ↗ | (On Diff #78074) | KFileItem kfi(QUrl::fromLocalFile(fileName)); |
49 | Put on new line | |
100 | const bool | |
102 | Coding style: if (enabled) { ... } | |
210 | You probably want to loop for (int i = first; i <= last; ++i) { | |
kcms/autostart/autostart.h | ||
64 | Capitalize Changed | |
67 | const | |
kcms/autostart/autostartmodel.cpp | ||
177 ↗ | (On Diff #78074) | but why? |
262 ↗ | (On Diff #78074) | bool |
292 ↗ | (On Diff #78074) | Coding style: } else { |
kcms/autostart/autostartmodel.h | ||
53 ↗ | (On Diff #78074) | Why not just use Qt::DisplayRole? |
kcms/autostart/autostart.cpp | ||
---|---|---|
182 ↗ | (On Diff #78074) | Yes. |
241 ↗ | (On Diff #78074) | Ok |
210 | rows inserted gives you a range of [first,last] which was added | |
kcms/autostart/autostartmodel.h | ||
53 ↗ | (On Diff #78074) | Why define DisplayRole as dedicated enum entry, rather than just using Qt::DisplayRole in the code everywhere |
kcms/autostart/autostartmodel.h | ||
---|---|---|
53 ↗ | (On Diff #78074) | 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 ↗ | (On Diff #78074) | Weeell... knowing about Qt::DisplayRole is kind of prerequisite to making your own model. :-) |
kcms/autostart/autostart.cpp | ||
---|---|---|
182 ↗ | (On Diff #78074) | Yes, otherwise you wouldn't get a modal dialog. |
231 ↗ | (On Diff #78074) | Yes, similar situation to rowInserted here. |
241 ↗ | (On Diff #78074) | Yes, it used to also inherit QObject (which wasn't a great idea) but not anymore. |
258 ↗ | (On Diff #78074) | This curly brace needs to be on the previous line. |
102 | I think Kai's point was also that the curly braces are required for the if and the else | |
210 | Good point, this still needs to be addressed. | |
kcms/autostart/autostart.h | ||
64 | Still needs being addressed | |
kcms/autostart/autostartmodel.cpp | ||
178 ↗ | (On Diff #78074) | 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 ↗ | (On Diff #78074) | Space before * not after, or just use auto |
185–186 ↗ | (On Diff #78074) | ditto |
207 ↗ | (On Diff #78074) | Actually I think I'd Q_ASSERT(!parent.isValid()) |
208 ↗ | (On Diff #78074) | This Q_UNUSED is now used ;-) |
277 ↗ | (On Diff #78074) | Space before * not after, or use auto |
282 ↗ | (On Diff #78074) | Since you touched the line, please drop the spaces between the parentheses |
289 ↗ | (On Diff #78074) | Since you touched the line, please drop the spaces between the parentheses |
311 ↗ | (On Diff #78074) | Since you touched the line please fix the * and { positions |
kcms/autostart/autostart.h | ||
64 | Still needs being addressed. :-) | |
kcms/autostart/autostartmodel.cpp | ||
131 ↗ | (On Diff #78074) | Space before * not after |
178 ↗ | (On Diff #78074) | Still needs being addressed. |
kcms/autostart/autostartmodel.h | ||
50 ↗ | (On Diff #78074) | Since you touched the line, please drop the spaces between the parentheses |
82 ↗ | (On Diff #78074) | Space before * not after |
kcms/autostart/autostartmodel.h | ||
---|---|---|
53 ↗ | (On Diff #78074) | As you want. |