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

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> kcm.cpp:270
> +        m_sourcesModel->load();
> +        for (auto &settings : m_sourcesModel->settingsList()) {
> +            auto toAdd = new NotificationManager::BehaviorSettings(settings.type, settings.groupName, this);

This method is only used in this place, so I don't see why we would want a temporary list with a struct and all rather than just iterating here directly.

> kcm.cpp:271
> +        for (auto &settings : m_sourcesModel->settingsList()) {
> +            auto toAdd = new NotificationManager::BehaviorSettings(settings.type, settings.groupName, this);
> +            m_behaviorSettingsList[settings.groupName] = toAdd;

Please `auto *` with an asterisk

> ApplicationConfiguration.qml:45
>  
> -    function behavior() {
> -        if (configColumn.desktopEntry) {
> -            return kcm.settings.applicationBehavior(configColumn.desktopEntry);
> -        } else if (configColumn.notifyRcName) {
> -            return kcm.settings.serviceBehavior(configColumn.notifyRcName);
> -        }
> -        return -1;
> -    }
> -
> -    function setBehavior(flag, enable) {
> -        var newBehavior = behavior();
> -        if (enable) {
> -            newBehavior |= flag;
> +    onAppDisplayNameChanged: {
> +        if (desktopEntry !== "") {

I've seen duplicate apps in this list, e.g. snap vs properly installed, so I don't think you can rely on this.

> ApplicationConfiguration.qml:47
> +        if (desktopEntry !== "") {
> +            kcm.setBehaviorSettingsToLoad(desktopEntry)
>          } else {

While we generally do RDN for desktop entries these days I belive there can be cases where there's no telling if it's a desktop entry or notfiyrc? So we would need to pass that in

> sourcesmodel.h:57
> +{
> +    QString type; // Whether Applications or Services
> +    QString groupName; // Corresponding DesktopEntry or NotifyRcName

Why not an enum?

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, ragreen, ZrenBot, ngraham, 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/20200423/66e639bd/attachment-0001.html>