[server] Respect input region of sub-surfaces on pointer surface focus
ClosedPublic

Authored by romangg on Aug 1 2017, 2:02 PM.

Details

Summary

KWayland takes always the top-most child surface at a given position for its
pointer input. But if a sub-surface sets its input region, it should not select
this one when the position is out of its input region, but rather try the
surface below.

Test Plan

My testing was only on my Xwayland branch. Supposed to also fix a problem
with Firefox native Wayland port.

Diff Detail

Repository
R127 KWayland
Branch
inputRegionSubSurfaces
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3429
Build 3447: arc lint + arc unit
romangg created this revision.Aug 1 2017, 2:02 PM
Restricted Application added a project: Frameworks. ยท View Herald TranscriptAug 1 2017, 2:02 PM
Restricted Application added a subscriber: plasma-devel. ยท View Herald Transcript
graesslin requested changes to this revision.Aug 1 2017, 3:01 PM
graesslin added a subscriber: graesslin.

Please verify that this does not break Qt! (Qt has a very special handling of subsurfaces and we have lots of workarounds for Qt there)

Also please add a unit test which exposes the problem.

This revision now requires changes to proceed.Aug 1 2017, 3:01 PM
graesslin added inline comments.Aug 1 2017, 3:05 PM
src/server/surface_interface.cpp
863

Could you try to share the logic between surfaceAt and inputSurfaceAt methods? I cannot spot a difference except the method it calls and that could be handled in a more generic way (e.g. function pointer).

I'm currently on a tight time budget, so this will need to wait a little bit. I'll roll with the change on my machine for a while and if everything works fine add a unit test in the end.

romangg updated this revision to Diff 18059.Aug 12 2017, 3:12 PM
romangg edited edge metadata.

Use function pointers.

romangg marked an inline comment as done.Aug 12 2017, 3:13 PM

I like that refactoring ๐Ÿ‘ - though processAt might not be the best fitting name. Don't have a good name idea, maybe findChildSurfaceAt?

any update on this?

Restricted Application added a subscriber: kde-frameworks-devel. ยท View Herald TranscriptMay 20 2018, 1:56 PM

any update on this?

Currently I'm working on other stuff and don't want to increase the mental load. I really only need this when looking again into sub-surfaces to handle Xwayland Present on child windows. But if there are some real-life problems associated with this patch missing I will look at it again.

zzag added a subscriber: zzag.Sep 14 2018, 7:18 AM
zzag added inline comments.
src/server/surface_interface.cpp
841

Shouldn't it be QRect instead?

davidedmundson added inline comments.
src/server/surface_interface.cpp
841

position is a float

zzag added inline comments.Sep 14 2018, 8:57 AM
src/server/surface_interface.cpp
841

So, it would be fine to return true in the following case:

QRectF(QPoint(0, 0), QSize(10, 10)).contains(QPointF(10, 10));

?

I like that refactoring ๐Ÿ‘ - though processAt might not be the best fitting name. Don't have a good name idea, maybe findChildSurfaceAt?

Maybe findSurfaceHelper.

davidedmundson requested changes to this revision.Sep 27 2018, 9:20 PM

I've written you a unit test.

I can either take over this or upload as a separate review that you can merge in.

src/server/surface_interface.cpp
839

This needs to be

&& (surface.inputIsInfinite() || input.contains(..))

which means either an extra arg in our function pointer or go back to the frankly simpler first revision. Writing N complex lines to save duplicating N simple lines doesn't make much sense to me, especially when they end up deviating.

841

I would have said so.

But then it means sizeContains and inputContains behave differently as

QRegion(QRect(0,0,10,10)).contains(10,10); returns false

which gives us some inconsistency.

This revision now requires changes to proceed.Sep 27 2018, 9:20 PM
romangg updated this revision to Diff 42725.Oct 2 2018, 2:37 PM

Rebase on master.

romangg updated this revision to Diff 42733.Oct 2 2018, 4:12 PM
  • Update version number
  • Do code duplication instead of pointers
  • Add comment about code duplication
romangg marked an inline comment as done.Oct 2 2018, 4:14 PM
davidedmundson accepted this revision.Oct 2 2018, 4:22 PM
zzag added a comment.Oct 2 2018, 4:23 PM

QRectF(QPoint(0, 0), size()).contains(position)

Shouldn't it be QRect instead? (again)
If it should be QRectF indeed, could someone please explain why?

I think maybe it should, but it should be changed with surfaceAt

romangg edited the summary of this revision. (Show Details)Oct 24 2018, 11:54 AM
romangg edited the test plan for this revision. (Show Details)
romangg updated this revision to Diff 44149.Oct 24 2018, 11:57 AM

Rebase on master.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 24 2018, 12:21 PM
This revision was automatically updated to reflect the committed changes.