Implement seatop_touch

[stacyharper]

Atm we got issue with the touch position sent to the clients. While holding contact, leaving the initial container will continue to send motion event to the client but with the new local position from the new container.

This seatop goal is to send the position of the touch event, relatively to the initial container layout position.

This will solve: https://github.com/swaywm/sway/issues/6454

I'm pretty new to Wayland and even more with the Sway codebase.

[stacyharper]

For record, I firstly tried to rely on seatop_down but it use cursor as much as possible. As touches can be multiple, I think we have to implement a new dedicated seatop.

[stacyharper]

Oops, sorry I missclicked on ready for review, which is not !

[kennylevinsen]

Just click "Convert to draft" in upper right near the reviewer section.

[stacyharper]

I pushed fixes that you mentioned.

is fine, but I think we should resolve 1). To do that, I think you'd want to merge theseatop_down and seatop_touch_down files, so that the touch down seatop can have pointer/tablet motion handlers. It's fine if struct seatop_down_event has a struct wl_list point_events that's only ever used for touch input.

I agree that those pointer move still must be send to the client.

I tried you solution. The thing is that the seatop_down pointer_motion think we pressed the pointer to begin. It rely on the cursor initial position. I think it make no sense to handle it that way cause this really is a touch event.

I think we should instead implement a pointer_motion in the seatop_touch to just notify with the current cursor position.

ps: this is fact what the seatop_down does for pointer axis and buttons. We could just do the same in seatop_touch

What do you think about this ?

[stacyharper]

Okay I thought a little more about this and in fact you right. I have to send cursor position relative to the cont position. I just have to setup the initial cursor position in my begin_touch_down correctly.

[stacyharper]

Okay I tried to merge both. I let this change in a dedicated commit.

I dont think this is a good idea and it introduce more complexity to this simple seatop_touch.

Plus anyway. the cursor disappear when I use the touch screen. I have to click with the mouse to display it again. And this action stop the previous seatop. I think we should just drop this commit :S

[stacyharper]

Okay I've done everything you said. It looks to works at least for the touch part (when there is no cursor handling)

The cursor sometime display itself when using the mouse, and sometime it stay hidden. I still dont know really why. Anyway, it does not send motion event to the client. There probably is a case we want to handle in the pointer_motion in cursor.c.

Clicking with the mouse break the seatop but that is probably what we expect. Starting the seatop with a mouse click hold, then add touches, then up touches will stop the seatop. It may not be what we expect.

[Xyene]

The cursor sometime display itself when using the mouse, and sometime it stay hidden.

That's weird, but likely unrelated to this PR. Unfortunately, I don't own a touchscreen device with which to test this myself.

Anyway, it does not send motion event to the client.

Hmm, are you sure? pointer_motion will call into seatop_pointer_motion,

https://github.com/swaywm/sway/blob/bb3fd0abc56f39e35dc7f4e86f25da1b4a6efbd7/sway/input/cursor.c#L378-L380

which should be sending motion events here

https://github.com/swaywm/sway/blob/bb3fd0abc56f39e35dc7f4e86f25da1b4a6efbd7/sway/input/seatop_down.c#L49

Clicking with the mouse break the seatop but that is probably what we expect.

Maybe. I don't have a good sense for how that is to use (in Chrome OS, it doesn't, you can click multiple times with the pointer while scrolling via touch), but it doesn't sound terrible.

Starting the seatop with a mouse click hold, then add touches, then up touches will stop the seatop. It may not be what we expect.

Yeah, that's a little weird. Not sure we can do a lot about this with the current structure. The seatop split works well for pointer/tablet input which is single-point, but doesn't generalize well to multi-touch input. Fixing this would be a lot of work.

[stacyharper]

At least now client can implement correctly touch holded event :]

[stacyharper]

Okay nice ! It is very easy to test with wev.

Thanks a lot for your reviews and assistance on this @Xyene !

[stacyharper]

Okay maybe it is not that ready yet. I got crashes while touching the swaybar and other layer-shells

[stacyharper]

con may be NULL. I have to check it before container_raise_floating

[Xyene]

This should be fixed in https://github.com/swaywm/sway/pull/6418 (there are some other cases that could crash too, see that PR for details). We should get that merged before this one.

[stacyharper]

Oh yes, that's exactly that. I'll have both on my device then. Should I remove my check in this MR ?

[Xyene]

Yeah, it'll prevent a merge conflict.

[stacyharper]

Okay I dont have any crashes with both. But it will cause build conflicts cause I removed the time arg from the default seatop begin.

I can push this MR with the other commit. We could then merge them in the correct order.

Should I ?

[Xyene]

Since https://github.com/swaywm/sway/pull/6418 has been merged, could you rebase this PR?

[stacyharper]

Okay I just rebased, resolved conflict as better as I tried and pushed it back.

I now got crashes. I guess the container_raise_floating(con) from seatop_begin_down still is in cause. I tried to add back my check condition and I did not crashed as soon as before. But I still had a crash later on my tries.

What should I know about https://github.com/swaywm/sway/pull/6418 ?

[stacyharper]

It looks like I fixed my crashes using directly seatop_begin_down_on_surface and not seatop_begin_down. The con in my context can be a NULL pointer so relying on it looks dangerous.

[stacyharper]

Nope, it crash when playing too much with swaybar :(

[stacyharper]

This line make Sway to crash wl_signal_add(&e->surface->events.destroy, &e->surface_destroy); when touching swaybar then draggin above when there is no window.

ps: I mentionned Swaybar but I got the same issue for any layer-shell. And I dont have to drag. Just touching if make this line to crash if there is no focused window.

ps 2 : err: [2127657.009] -> [email protected](wayland.c:1153: )

[stacyharper]

Okay I think I fixed my crashes:

• I had to toggle off simulating_pointer_from_touch before starting my seatop in cursor.c handle_touch_down. The reason is that it is possible to have a finger or a touch on an empty workspace before a layer shell pop out. Which stuck the cursor in emulation mode. Then the new touch_down touches will behave wrongly and cause crashes.
• I fixed my surface determination in the default seatop handle_touch_down. I used the cursor x and y which is wrong and cause a NULL pointer as surface. Attaching destroy event to this pointer was causing a crash

[stacyharper]

Hello there ! Someone got an idea of when this could be merged ? :D It prevent us in sxmo to chaim officialy the support of sway

[stacyharper]

I pushed a rebase I had in my tree. I use this tree for some month now without crashes

[emersion]

Apart from these comments, this looks good to me (both code and testing).

[stacyharper]

Apart from these comments, this looks good to me (both code and testing).

One things remains to me after some of my last tries:

I added a little bit of logic to prevent the seatop_down to handle touches initiated from other surface than the initial one. But it looks like it doesnt works as much as I expect it. If I touch two time in another surface, it looks like it reset the seatop :S I'm not sure of what I am missing atm

[stacyharper]

If I touch two time in another surface, it looks like it reset the seatop :S I'm not sure of what I am missing atm

to be more precise, my patch works for surfaces that handle touch events. By example, with two wev windows, I cannot interact with the second one when I have already a seatop_down on the first one. But in the case of wev + foot, touching foot seems to stop the seatop_down. Maybe it is related to mouse simulation ? cause foot does not handle touch events yet

[stacyharper]

sway/input/cursor.c:481 handle_touch_down

→   if (surface && wlr_surface_accepts_touch(wlr_seat, surface)) { 
→   →   if (seat_is_input_allowed(seat, surface)) { 
→   →   →   cursor->simulating_pointer_from_touch = false; 
→   →   →   seatop_touch_down(seat, event); 
→   →   } 
→   } else if (!cursor->simulating_pointer_from_touch && 
→   →   →   (!surface || seat_is_input_allowed(seat, surface))) { 
→   →   // Fallback to cursor simulation. 
→   →   // The pointer_touch_id state is needed, so drags are not aborted when over 
→   →   // a surface supporting touch and multi touch events don't interfere. 
→   →   cursor->simulating_pointer_from_touch = true; 
→   →   cursor->pointer_touch_id = seat->touch_id; 
→   →   double dx, dy; 
→   →   dx = lx - cursor->cursor->x; 
→   →   dy = ly - cursor->cursor->y; 
→   →   pointer_motion(cursor, event->time_msec, event->device, dx, dy, dx, dy); 
→   →   dispatch_cursor_button(cursor, event->device, event->time_msec, 
→   →   →   →   BTN_LEFT, WLR_BUTTON_PRESSED); 
→   } 

We should find a way to prevent the else if when the surface does not accept touches if a seatop already exists (if it is not the seatop_default ?)

ps: I'll try to move all this logic in the seatop_default as a seatop_down start condition

[stacyharper]

We should find a way to prevent the else if when the surface does not accept touches if a seatop already exists (if it is not the seatop_default ?)

Okay, in the last commit I moved the block inside the seatop_default. So now, while a seatop_down is running, we can only add new touches from the same initial surface until all touches are released.

[stacyharper]

@emersion does the last patches seems still good to you ? (I'm sorry for pinging but I feels like the MR tends to stall after the moment I send multiple comments to myself as I did)