broulik added a comment.

  Pingeliping.

INLINE COMMENTS

> xwindowsystemeventbatcher.cpp:34
> +    connect(KWindowSystem::self(), &KWindowSystem::windowRemoved, this, 
> [this](WId wid) {
> +        if (m_cache.contains(wid)) {
> +            emit windowChanged(wid, m_cache[wid].properties, 
> m_cache[wid].properties2);

Avoid double lookup

> xwindowsystemeventbatcher.cpp:35
> +        if (m_cache.contains(wid)) {
> +            emit windowChanged(wid, m_cache[wid].properties, 
> m_cache[wid].properties2);
> +            m_cache.remove(wid);

Why emit a window change just before you emit a removal? Or is that what 
`KWindowSystem` usually does and we rely on that?

> xwindowsystemeventbatcher.cpp:45
> +        [this](WId window, NET::Properties properties, NET::Properties2 
> properties2) {
> +            m_cache[window].properties |= properties;
> +            m_cache[window].properties2 |= properties2;

Look up only once by caching reference?

> xwindowsystemeventbatcher.cpp:57
> +    Q_UNUSED(event);
> +    for(auto it = m_cache.constBegin(); it!= m_cache.constEnd(); it++) {
> +        emit windowChanged(it.key(), it.value().properties, 
> it.value().properties2);

Coding style

REPOSITORY
  R120 Plasma Workspace

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

To: davidedmundson, #plasma
Cc: ngraham, cfeck, broulik, hein, graesslin, plasma-devel, ZrenBot, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

Reply via email to