When installing a new cursor theme (or ditto uninstalling) from KNewStuff, the current theme would become deselected due to a full refresh of the theme list.
BUG:406084
Details
Details
Diff Detail
Diff Detail
- Repository
- R119 Plasma Desktop
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
Why do we need this do-it-once helper function?
| kcms/cursortheme/kcmcursortheme.cpp | ||
|---|---|---|
| 466 | Coding style: space between if and ( | |
| kcms/cursortheme/kcmcursortheme.cpp | ||
|---|---|---|
| 466 | This reads weird: how about: if (selectedIndex().isValid()) {
const CursorTheme* theme = m_proxyModel->theme(selectedIndex());
...
} | |
| 472 | Maybe another solution would be having the model insert/remove instead of resetting, although that works too I guess. | |
| kcms/cursortheme/kcmcursortheme.h | ||
| 150 ↗ | (On Diff #55261) | :) this sounds familiar. Maybe we should keep this in the cpp file? it's only used there. |
Comment Actions
To avoid having the lambda being called multiple times for subsequent installations, especially with incorrect parameters... It's a bit of a workaround for some missing metaprogramming abilities in C++/Qt that Discover also uses in a couple of places, it's quite handy and i can't help but think it wants to be somewhere more centrally available, but i also am not sure where that'd want to be ;)
| kcms/cursortheme/kcmcursortheme.cpp | ||
|---|---|---|
| 466 | ||
| kcms/cursortheme/kcmcursortheme.h | ||
| 150 ↗ | (On Diff #55261) | It certainly does ;) Hmm... true, it doesn't actually need moc-ing, at least when using the modern connect syntax like is being done here, so... yup, i'll pop it in there :) |
Comment Actions
Less invasive change, but with more (what seem like fairly reasonable) assumptions. We now operate explicitly on installed and uninstalled files as presented by KNS, rather than simply rechecking the theme locations (which it turns out is surprisingly intensive, but then again, it /is/ a disk IO operation, so perhaps not really that surprising).
| kcms/cursortheme/kcmcursortheme.cpp | ||
|---|---|---|
| 472 | New patch does this - less invasive change as well, though there's now a non-zero number of assumptions being made about what things look like on the filesystem. | |
Comment Actions
Otherwise makes sense to me.
| kcms/cursortheme/kcmcursortheme.cpp | ||
|---|---|---|
| 468 | Use splitRef if you're just taking the last one. | |
| kcms/cursortheme/kcmcursortheme.cpp | ||
|---|---|---|
| 468 | Quite - still need to convert that last to a string later, but less string creation's good :) Kind of a microoptimisation, given we're dealing with disk io as well, but it doesn't hurt readability anyway, so, yup :) | |
Comment Actions
Address @apol's comment (can't use splitref in the second bit, as we're joining the list again at the end)
Comment Actions
FYI the patch gets closed automatically only if the commit message has Differential Revision: https://phabricator.kde.org/D20198 somewhere in it. If you use arc land or cherry-pick the commit hash of the arc-created patch, that happens automatically.