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

[Roman Gilg]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R127:24bdc06747ed: [server] Respect input region of 
sub-surfaces on pointer surface focus (authored by romangg).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7038?vs=44149=44151

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

AFFECTED FILES
  src/server/pointer_interface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: romangg, #frameworks, graesslin, davidedmundson
Cc: davidedmundson, zzag, kde-frameworks-devel, graesslin, plasma-devel, 
ragreen, Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[Roman Gilg]

romangg updated this revision to Diff 44149.
romangg added a comment.


  Rebase on master.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7038?vs=42733=44149

BRANCH
  inputRegionSubSurfaces

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

AFFECTED FILES
  src/server/pointer_interface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: romangg, #frameworks, graesslin, davidedmundson
Cc: davidedmundson, zzag, kde-frameworks-devel, graesslin, plasma-devel, 
ragreen, Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[David Edmundson]

davidedmundson added a comment.


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

REPOSITORY
  R127 KWayland

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

To: romangg, #frameworks, graesslin, davidedmundson
Cc: davidedmundson, zzag, kde-frameworks-devel, graesslin, plasma-devel, 
ragreen, Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[Vlad Zagorodniy]

zzag added a comment.


  > 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?

REPOSITORY
  R127 KWayland

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

To: romangg, #frameworks, graesslin, davidedmundson
Cc: davidedmundson, zzag, kde-frameworks-devel, graesslin, plasma-devel, 
ragreen, Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[Roman Gilg]

romangg updated this revision to Diff 42733.
romangg added a comment.


  - Update version number
  - Do code duplication instead of pointers
  - Add comment about code duplication

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7038?vs=42725=42733

BRANCH
  inputRegionSubSurfaces

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

AFFECTED FILES
  src/server/pointer_interface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: romangg, #frameworks, graesslin, davidedmundson
Cc: davidedmundson, zzag, kde-frameworks-devel, graesslin, plasma-devel, 
ragreen, Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[Roman Gilg]

romangg updated this revision to Diff 42725.
romangg added a comment.


  Rebase on master.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7038?vs=18059=42725

BRANCH
  inputRegionSubSurfaces

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

AFFECTED FILES
  src/server/pointer_interface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h
  src/server/surface_interface_p.h

To: romangg, #frameworks, graesslin, davidedmundson
Cc: davidedmundson, zzag, kde-frameworks-devel, graesslin, plasma-devel, 
ragreen, Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[David Edmundson]

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  I've written you a unit test.
  
  I can either take over this or upload as a separate review that you can merge 
in.

INLINE COMMENTS

> zzag wrote in surface_interface.cpp:819
> So, it would be fine to return `true` in the following case:
> 
>   QRectF(QPoint(0, 0), QSize(10, 10)).contains(QPointF(10, 10));
> 
> ?

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.

> surface_interface.cpp:824
>  {
> -if (!isMapped()) {
> +return sizeContains(size, QRegion(), position) && 
> input.contains(position.toPoint());
> +}

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.

REPOSITORY
  R127 KWayland

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

To: romangg, #frameworks, graesslin, davidedmundson
Cc: davidedmundson, zzag, kde-frameworks-devel, graesslin, plasma-devel, 
ragreen, Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[Roman Gilg]

romangg added a comment.


  In D7038#134955 , @graesslin wrote:
  
  > I like that refactoring  - though processAt might not be the best fitting 
name. Don't have a good name idea, maybe findChildSurfaceAt?
  
  
  Maybe `findSurfaceHelper`.

REPOSITORY
  R127 KWayland

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

To: romangg, #frameworks, graesslin
Cc: davidedmundson, zzag, kde-frameworks-devel, graesslin, plasma-devel, 
ragreen, Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> davidedmundson wrote in surface_interface.cpp:819
> position is a float

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

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

?

REPOSITORY
  R127 KWayland

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

To: romangg, #frameworks, graesslin
Cc: davidedmundson, zzag, kde-frameworks-devel, graesslin, plasma-devel, 
ragreen, Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> zzag wrote in surface_interface.cpp:819
> Shouldn't it be QRect instead?

position is a float

REPOSITORY
  R127 KWayland

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

To: romangg, #frameworks, graesslin
Cc: davidedmundson, zzag, kde-frameworks-devel, graesslin, plasma-devel, 
ragreen, Pitel, schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[Vlad Zagorodniy]

zzag added inline comments.

INLINE COMMENTS

> surface_interface.cpp:819
> +
> +return !size.isEmpty() && QRectF(QPoint(0, 0), size).contains(position);
> +}

Shouldn't it be QRect instead?

REPOSITORY
  R127 KWayland

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

To: romangg, #frameworks, graesslin
Cc: zzag, kde-frameworks-devel, graesslin, plasma-devel, ragreen, Pitel, 
schernikov, michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[Roman Gilg]

romangg added a comment.


  In D7038#265444 , @graesslin wrote:
  
  > 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.

REPOSITORY
  R127 KWayland

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

To: romangg, #frameworks, graesslin
Cc: kde-frameworks-devel, graesslin, plasma-devel, ragreen, Pitel, schernikov, 
michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[Martin Flöser]

graesslin added a comment.
Restricted Application added a subscriber: kde-frameworks-devel.


  any update on this?

REPOSITORY
  R127 KWayland

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

To: romangg, #frameworks, graesslin
Cc: kde-frameworks-devel, graesslin, plasma-devel, ragreen, Pitel, schernikov, 
michaelh, ZrenBot, ngraham, bruns, alexeymin, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein

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

[Martin Flöser]

graesslin added a comment.


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

REPOSITORY
  R127 KWayland

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

To: subdiff, #frameworks, graesslin
Cc: graesslin, plasma-devel, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

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

[Roman Gilg]

subdiff marked an inline comment as done.

REPOSITORY
  R127 KWayland

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

To: subdiff, #frameworks, graesslin
Cc: graesslin, plasma-devel, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

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

[Roman Gilg]

subdiff updated this revision to Diff 18059.
subdiff added a comment.


  Use function pointers.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7038?vs=17500=18059

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

AFFECTED FILES
  src/server/pointer_interface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h
  src/server/surface_interface_p.h

To: subdiff, #frameworks, graesslin
Cc: graesslin, plasma-devel, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

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

[Roman Gilg]

subdiff added a comment.


  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.

REPOSITORY
  R127 KWayland

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

To: subdiff, #frameworks, graesslin
Cc: graesslin, plasma-devel, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

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

[Martin Flöser]

graesslin added inline comments.

INLINE COMMENTS

> surface_interface.cpp:841
>  
> +SurfaceInterface *SurfaceInterface::inputSurfaceAt(const QPointF )
> +{

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

REPOSITORY
  R127 KWayland

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

To: subdiff, #frameworks, graesslin
Cc: graesslin, plasma-devel, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

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

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added a comment.
This revision now requires changes to proceed.


  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.

REPOSITORY
  R127 KWayland

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

To: subdiff, #frameworks, graesslin
Cc: graesslin, plasma-devel, leezu, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas

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

[Roman Gilg]

subdiff created this revision.
subdiff added a project: Plasma on Wayland.
Restricted Application added a subscriber: plasma-devel.
Restricted Application added a project: Frameworks.

REVISION SUMMARY
  I noticed that while working on my GSoC Xwayland project and using 
sub-surfaces with empty input region. 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.

REPOSITORY
  R127 KWayland

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

AFFECTED FILES
  src/server/pointer_interface.cpp
  src/server/surface_interface.cpp
  src/server/surface_interface.h

To: subdiff, #frameworks
Cc: plasma-devel, leezu, ZrenBot, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas