D26039: [Plasma Style KCM] Add search filter

[Kevin Ottens]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20191223/a7420581/attachment-0001.html>