Support highlighting windows through EffectsHandlerImpl
ClosedPublic

Authored by graesslin on Aug 30 2016, 2:08 PM.

Details

Summary

So far TabBox used highlight windows by passing window ids around through
an X property. This doesn't work on Wayland where we don't have window
ids for our TabBox and the Wayland windows.

This change introduces a new Effect::Feature for HighlightWindows which
the HighlightWindowsEffect provides. The EffectsHandlerImpl has a new
method to highlightWindows which it delegates to that effect if it is
loaded by invoking a slot through the meta object.

The TabBoxHandler now passes the highlighting to the effects system
instead of updating the x11 property. Thus this works on Wayland and
at the same time improves the X11 side by no longer having to go through
the property protocol.

Test Plan

Verified that Alt+Tab highlights the windows on Wayland correctly.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
graesslin updated this revision to Diff 6371.Aug 30 2016, 2:08 PM
graesslin retitled this revision from to Support highlighting windows through EffectsHandlerImpl.
graesslin updated this object.
graesslin edited the test plan for this revision. (Show Details)
graesslin added reviewers: KWin, Plasma on Wayland.
Restricted Application added projects: Plasma on Wayland, KWin. · View Herald TranscriptAug 30 2016, 2:08 PM
Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript
luebking added inline comments.
effects.cpp
1560

Errr... wut?
Any reason to not *demand* some sort of soft API (slot signature) instead of digging for the next best thing that remotely looks like the right thing?

Maybe provide hard API like "Effect::performFeature(Feature f, QVariant v)"?

graesslin added inline comments.Aug 31 2016, 12:10 PM
effects.cpp
1560

Actually that was an experiment. I didn't want to use QMetaObject::invokeMethod with a string, so I wanted to try whether there's another way.

But maybe adding another method is the better solution, it's not really beautiful code I wrote there.

You could expose the relevant method index, but looking up the signature is more elegant (and this isn't a hot call)

graesslin updated this revision to Diff 6385.Aug 31 2016, 12:45 PM

Added a Effect::performFeature(Feature, QVariantList) to not do magic invokation

luebking added inline comments.Aug 31 2016, 1:09 PM
libkwineffects/kwineffects.h
496

maybe only "perform" (and have the signature fill in the information), but in general a much better solution ;-)

graesslin updated this revision to Diff 6388.Aug 31 2016, 1:17 PM

s/performFeature/perform

sebas added a subscriber: sebas.Sep 12 2016, 8:09 PM
sebas added inline comments.
effects.h
239 ↗(On Diff #6388)

EffectWindow* ?

effects/highlightwindow/highlightwindow.cpp
261 ↗(On Diff #6388)

whitespace

effects/highlightwindow/highlightwindow.h
58 ↗(On Diff #6388)

EffectWindow* (no space)

sebas accepted this revision.Sep 12 2016, 8:10 PM
sebas added a reviewer: sebas.
This revision is now accepted and ready to land.Sep 12 2016, 8:10 PM
graesslin added inline comments.Sep 13 2016, 6:11 AM
effects.h
239 ↗(On Diff #6388)

Please check the coding style. There is supposed to be a space before * and &. See both Qt style https://wiki.qt.io/Qt_Coding_Style:
"For pointers or references, always use a single space between the type and '*' or '&', but no space between the '*' or '&' and the variable name: "

Example:

// Wrong
char* blockOfMemory = (char* ) malloc(data.size());

// Correct
char *blockOfMemory = reinterpret_cast<char *>(malloc(data.size()));

And KDE coding style at https://community.kde.org/Policies/Kdelibs_Coding_Style reads:
"For pointers or references, use a single space before '*' or '&', but not after"

Unfortunately not with such a nice example as in Qt case.

This revision was automatically updated to reflect the committed changes.