D20265: Introduce libnotificationmanager

[David Edmundson]

davidedmundson added a comment.


  It's a big review :/
  
  The general infrastructure and design makes sense.
  I shall review the individual classes separately during the week.

INLINE COMMENTS

> CMakeLists.txt:3
> +if(BUILD_TESTING)
> +   #add_subdirectory(autotests)
> +endif()

That's one way to avoid test failures.

> CMakeLists.txt:26
> +
> +set(HAVE_PULSEAUDIOQT ${KF5PulseAudioQt_FOUND})
> +configure_file(config-notificationmanager.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/config-notificationmanager.h )

I don't know the status of this, but please make sure we don't depend on something in review

> notificationgroupcollapsingproxymodel.cpp:126
> +
> +    QPersistentModelIndex persistentIdx(idx);
> +    if (expanded) {

It seems safer to take the pesistent index from the sourceModel

We're invalidating this layer repeatedly which could invalidate our persistent index

> notificationgroupcollapsingproxymodel.cpp:133
> +
> +    invalidateFilter();
> +

you shouldn't need this, the data is changing so it'll get re-evaluated

REPOSITORY
  R120 Plasma Workspace

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

To: broulik, #plasma
Cc: davidedmundson, ngraham, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20190409/97b7d456/attachment.html>