ervin requested changes to this revision. ervin added inline comments. This revision now requires changes to proceed.
INLINE COMMENTS > filterproxymodel.cpp:110 > + > +bool FilterProxyModel::filterAcceptsRow(int source_row, const QModelIndex > &source_parent) const > +{ Ditto > filterproxymodel.h:61 > + > + bool filterAcceptsRow(int source_row, const QModelIndex &source_parent) > const override; > + s/source_row/sourceRow/ same for source_parent > ThemePreview.qml:180 > Kirigami.Icon { > - visible: model.followsSystemColors > + visible: model.colorType == Private.ThemesModel.FollowsColorTheme > source: "color-profile" Better use === with this bloody javascript > themesmodel.cpp:161 > + QStringList themes; > + const QStringList &packs = > QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, > QStringLiteral("plasma/desktoptheme"), QStandardPaths::LocateDirectory); > + for(const QString &ppath : packs) { A ref on a temporary sounds like a bad idea to me. > themesmodel.cpp:165 > + const QStringList &entries = cd.entryList(QDir::Dirs | QDir::Hidden > | QDir::NoDotAndDotDot); > + Q_FOREACH (const QString &pack, entries) { > + const QString _metadata = ppath + QLatin1Char('/') + pack + > QStringLiteral("/metadata.desktop"); Don't use Q_FOREACH but a proper for in new code > themesmodel.cpp:173 > + > + for (const QString &theme : themes) { > + int themeSepIndex = theme.lastIndexOf(QLatin1Char('/'), -1); qAsConst(themes) > themesmodel.cpp:237 > + > + for (const auto &item : m_data) { > + if (item.pendingDeletion) { qAsConst(m_data) > themesmodel.h:65 > + > + int rowCount(const QModelIndex &parent) const override; > + QVariant data(const QModelIndex &index, int role) const override; This one and the two following are missing the default values for their parameters (which parents have, I wish override would check for that too). REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D26039 To: davidre, #plasma, #vdg, broulik, ndavis, ngraham, ervin Cc: ervin, ndavis, crossi, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart