Split out the X11 mouse event filtering for EffectsHandlerImpl
ClosedPublic
Actions

Authored by graesslin on Sep 15 2017, 4:39 PM.

Details

Reviewers

Group Reviewers

KWin
Plasma

Commits

R108:0c6fdeef2db4: Split out the X11 mouse event filtering for EffectsHandlerImpl

Summary

This change introduces a dedicated X11EventFilter for the mouse
interception on X11. The filter gets created together with the start
of mouse interception and destroyed again when the mouse interception
ends. Thus we don't need to check for each event like it was the case
so far.

Unfortunately the existing methods cannot be removed (yet) as they are
still used by TabBox. Needs investigation whether this is actually
needed.

Test Plan

Xephyr+kwin_x11+Present Windows

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 created this revision.Sep 15 2017, 4:39 PM

Restricted Application added a project: KWin. · View Herald TranscriptSep 15 2017, 4:39 PM

Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript

Code does what it says but one comment

I would move the m_mouseInterceptionWindow code with it, that way we have RAII on that window, and less X specific things in the EffectsHandler. Logically it's grouped
do we even need that fake window given we now have the XI filter?

effects.cpp
707	I know this is the wrong patch to comment on this, but it seems this wayland code is wrong. we setEffectsOverrideCursor() when the effects count goes to 1 we should ony be removing it when it goes to 0. i.e this should be inside m_grabbedMouseEffects.isEmpty()

In D7842#146171, @davidedmundson wrote:

Code does what it says but one comment

I would move the m_mouseInterceptionWindow code with it, that way we have RAII on that window, and less X specific things in the EffectsHandler. Logically it's grouped

I looked into it as I also thought it's a good idea. But it's not that easy, it is used in more areas than just for the mouse motion. It's used in e.g. EffectsHandler::defineCursor, EffectsHandler::desktopResized and a few more.

I agree this should be moved out, but I think this needs more work and probably a class X11EffectsHandler : public EffectsHandlerImpl together with an API hook in the Platform API

do we even need that fake window given we now have the XI filter?

Yes, otherwise the mouse events would go to the windows below which would be urgh ;-) and also needed for setting the cursor shape.

ok.

Also note that I made a comment about the (untouched) wayland code.

This revision is now accepted and ready to land.Sep 15 2017, 6:50 PM

In D7842#146206, @davidedmundson wrote:

ok.

Also note that I made a comment about the (untouched) wayland code.

thanks for the hint, I hadn't noticed it before. Yeah, looks wrong, I guess it evolved or something like that ;-)

Closed by commit R108:0c6fdeef2db4: Split out the X11 mouse event filtering for EffectsHandlerImpl (authored by graesslin). · Explain WhySep 16 2017, 6:04 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

		Path
M		CMakeLists.txt (1 line)
M		effects.h (2 lines)
M		effects.cpp (3 lines)
A	M	effects_mouse_interception_x11_filter.h (43 lines)
A	M	effects_mouse_interception_x11_filter.cpp (65 lines)
M		events.cpp (19 lines)