Differential D20198 Diff 55455 kcms/cursortheme/kcmcursortheme.cpp

Changeset View

Standalone View

View Options

kcms/cursortheme/kcmcursortheme.cpp

Context not available.
462 if (dialog.exec()) { 462​ if (dialog.exec()) {
463 KNS3::Entry::List list = dialog.changedEntries(); 463​ KNS3::Entry::List list = dialog.changedEntries();
464 if (!list.isEmpty()) { 464​ if (!list.isEmpty()) {
465 m_model->refreshList(); 465​ for (const KNS3::Entry& entry : list) {
466​ if (entry.status() == KNS3::Entry::Deleted) {
ngrahamUnsubmitted
Done

Coding style: space between if and (

ngraham: Coding style: space between `if` and `(`
apolUnsubmitted
Done

This reads weird: how about:

if (selectedIndex().isValid()) {
    const CursorTheme* theme = m_proxyModel->theme(selectedIndex());
    ...
}
apol: This reads weird: how about: ``` if (selectedIndex().isValid()) { const CursorTheme* theme…
leinirAuthorUnsubmitted
Done

Thanks @ngraham :)

Good point @apol, it's a straight up copy from the save() function, and i'd just not unpacked it all the way. Yes, that definitely reads better, i'll swap it for that :)

leinir: Thanks @ngraham :) Good point @apol, it's a straight up copy from the save() function, and i'd…
467​ for (const QString& deleted : entry.uninstalledFiles()) {
468​ QVector<QStringRef> list = deleted.splitRef(QLatin1Char('/'));
apolUnsubmitted
Done

Use splitRef if you're just taking the last one.

apol: Use splitRef if you're just taking the last one.
leinirAuthorUnsubmitted
Not Done

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 :)

leinir: Quite - still need to convert that last to a string later, but less string creation's good :)…
469​ if (list.last() == QLatin1Char('*')) {
470​ list.takeLast();
471​ }
472​ QModelIndex idx = m_model->findIndex(list.last().toString());
apolUnsubmitted
Done

Maybe another solution would be having the model insert/remove instead of resetting, although that works too I guess.

apol: Maybe another solution would be having the model insert/remove instead of resetting, although…
leinirAuthorUnsubmitted
Not Done

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.

leinir: New patch does this - less invasive change as well, though there's now a non-zero number of…
473​ if (idx.isValid()) {
474​ m_model->removeTheme(idx);
475​ }
476​ }
477​ } else if (entry.status() == KNS3::Entry::Installed) {
478​ for (const QString& created : entry.installedFiles()) {
479​ QStringList list = created.split(QLatin1Char('/'));
480​ if (list.last() == QLatin1Char('*')) {
481​ list.takeLast();
482​ }
483​ // Because we sometimes get some extra slashes in the installed files list
484​ list.removeAll({});
485​ // Because we'll also get the containing folder, if it was not already there
486​ // we need to ignore it.
487​ if (list.last() == QLatin1String(".icons")) {
488​ continue;
489​ }
490​ m_model->addTheme(list.join(QLatin1Char('/')));
491​ }
492​ }
493​ }
466 } 494​ }
467 } 495​ }
468} 496​}
Context not available.