D27188: KCM Notifications : Manage app-specific notifications with KCconfigXT's magic

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kcm.cpp:277
> +    if (!m_currentBehavior) {
> +        setBehaviorSettingsToLoad(m_behaviorSettingsList.begin().key());
> +    }

What about m_behaviorSettingsList is empty? I'm not sure we can guarantee that it'll always contain something.

> kcm.cpp:321
> +    bool needSave = false;
> +    for (auto *behaviorSettings : m_behaviorSettingsList) {
> +        needSave |= behaviorSettings->isSaveNeeded();

Replace with any_of ? This would break the loop earlier.

> kcm.h:93
>  
> +    Q_INVOKABLE void setBehaviorSettingsToLoad(QString identifier);
> +

Should be a const ref

> kcm.h:108
> +    bool isSaveNeeded() const override;
> +    void createConnections(NotificationManager::BehaviorSettings* settings);
>  

Space before * not after

> sourcesmodel.cpp:380
> +    QVector<QPair<QString, QString>> res;
> +    for (auto &source : qAsConst(m_data)) {
> +        if (source.desktopEntry.isEmpty()) {

const auto &

> sourcesmodel.h:98
> +    // List of config settings in the form of Application, DesktopEntry of Services, NotifyrcName to load.
> +    QVector<QPair<QString, QString>> settingsList() const;
> +

It's generally a better idea to define a small trivial struct when roll with QPair

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27188

To: crossi, #plasma, ervin, broulik, bport, meven
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 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/20200212/ac42904d/attachment-0001.html>