D15443: [server] Allow multiple touch interfaces per client

[Roman Gilg]

This revision was automatically updated to reflect the committed changes.
Closed by commit R127:7bb2a225521c: [server] Allow multiple touch interfaces 
per client (authored by romangg).

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15443?vs=41916=46752

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

AFFECTED FILES
  src/server/seat_interface.cpp
  src/server/seat_interface_p.h

To: romangg, #kwin, #frameworks, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R127 KWayland

BRANCH
  refactorSeatTouch

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

To: romangg, #kwin, #frameworks, davidedmundson
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client

[Roman Gilg]

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


  Do not forget id equals 0 check and add assert.

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15443?vs=41513=41916

BRANCH
  refactorSeatTouch

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

AFFECTED FILES
  src/server/seat_interface.cpp
  src/server/seat_interface_p.h

To: romangg, #kwin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client

[Roman Gilg]

romangg added inline comments.

INLINE COMMENTS

> davidedmundson wrote in seat_interface.cpp:1332
> what happened to the 
> if (id == 0 )
> 
> which is making sure if you multi touch we only send the first as a mouse 
> event.

Good catch. Thanks.

> davidedmundson wrote in seat_interface.cpp:1361
> If you have a release build Q_ASSERT get completely compiled out and no code 
> will run.

True, just our dev PCs will suffer. Ok, I will add the assert.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> seat_interface.cpp:1332
> -d->touchInterface.focus.touch->down(id, serial, globalPosition - 
> d->touchInterface.focus.offset);
> -} else if (id == 0 && focusedTouchSurface()) {
>  #if HAVE_LINUX_INPUT_H

what happened to the 
if (id == 0 )

which is making sure if you multi touch we only send the first as a mouse event.

> seat_interface.cpp:1361
> +// note: this function may not be called after cancel,
> +// but for performance reasons still do not assert like in touchUp case?
>  Q_D();

If you have a release build Q_ASSERT get completely compiled out and no code 
will run.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client

[Roman Gilg]

romangg retitled this revision from "[server] Allow multiple touch interfaces 
per client and remove fallback code" to "[server] Allow multiple touch 
interfaces per client".
romangg edited the summary of this revision.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client

[Roman Gilg]

romangg edited the summary of this revision.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client and remove fallback code

[Roman Gilg]

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


  Readd fall back code

REPOSITORY
  R127 KWayland

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15443?vs=41445=41513

BRANCH
  refactorSeatTouch

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

AFFECTED FILES
  src/server/seat_interface.cpp
  src/server/seat_interface_p.h

To: romangg, #kwin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client and remove fallback code

[Roman Gilg]

romangg added a comment.


  In D15443#324889 , @davidedmundson 
wrote:
  
  > I don't understand removing this. If I have an app that doesn't understand 
touch doesn't it make sense to synthesise a pointer?
  
  
  I didn't read the code correctly. Thought it is meant for compositors not 
supporting touch, but indeed it's meant for clients not supporting it. Hmm, I 
imagine since most Wayland clients use some sort of GUI library there are not 
many Wayland native clients not supporting touch. And Xwayland translates touch 
events. So maybe it would still make sense to remove the code. But I'll add it 
back for now. This way the change also becomes more narrow.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client and remove fallback code

[David Edmundson]

davidedmundson added a comment.


  Oh, I see what you're saying, I misread, sorry. That does indeed make sense.
  
  > Some undocumented fallback code has been removed as well. Platforms not 
supporting touch are just considered not capable instead.
  
  I don't understand removing this. If I have an app that doesn't understand 
touch doesn't it make sense to synthesise a pointer?

INLINE COMMENTS

> seat_interface_p.h:147
>  SurfaceInterface *surface = nullptr;
> -TouchInterface *touch = nullptr;
> +QVector touchs;
>  QMetaObject::Connection destroyConnection;

touchs -> touches

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client and remove fallback code

[Roman Gilg]

romangg added a comment.


  I believe it works like this: A seat has one global pointer, one global 
keyboard and one global touch "object" each. But clients can bind to them 
multiple times. That's why there are focus.pointers and focus.keyboards vectors 
although there is only one global pointer/keyboard.
  
  A different question is what happens on a multi-seat system: when there are 
several mice or keyboards, the compositor must support splitting them up to 
multiple seats (but every seat again has only one global keyboard and pointer 
object). This does not work of course for touch screens, so that's why the 
specs is reads differently here. But as said this is not directly related to 
focus.pointers, focus.keyboards or focus.touchs being vectors per seat or not.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client and remove fallback code

[David Edmundson]

davidedmundson added a comment.


  > has been for no apparent reason
  
  From the spec (emphasis mine)
  
  The wl_touch interface represents **a** touchscreen associated with a seat.
  
  The wl_pointer interface represents **one or more** input devices,
  
  The wl_keyboard interface represents **one or more** keyboards associated 
with a seat.
  
  I assume the rationale being that wl_touch already has multiplicity inside 
it; mutliple touch screens, just add more touches. Whereas wl_pointer is always 
just one.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #frameworks
Cc: davidedmundson, kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client and remove fallback code

[Roman Gilg]

romangg added a dependent revision: D15464: [server] Touch drag support.

REPOSITORY
  R127 KWayland

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

To: romangg, #kwin, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D15443: [server] Allow multiple touch interfaces per client and remove fallback code

[Roman Gilg]

romangg created this revision.
romangg added reviewers: KWin, Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
romangg requested review of this revision.

REVISION SUMMARY
  The touch related code in the seat interface class has been for no apparent
  reason very different to pointer and keyboard code.
  
  This patch makes touch reladet code more similar and by that allows a client
  to receive touch events through multiple interfaces.
  
  Some undocumented fallback code has been removed as well. Platforms not
  supporting touch are just considered not capable instead.

TEST PLAN
  Manually and auto tests still pass.

REPOSITORY
  R127 KWayland

BRANCH
  refactorSeatTouch

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

AFFECTED FILES
  src/server/seat_interface.cpp
  src/server/seat_interface_p.h

To: romangg, #kwin, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns