Split out the X11 mouse event filtering for EffectsHandlerImpl
ClosedPublic

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

Details

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

  1. 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
  2. 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()

Code does what it says but one comment

  1. 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

  1. 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.

davidedmundson accepted this revision.Sep 15 2017, 6:50 PM

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

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 ;-)

This revision was automatically updated to reflect the committed changes.