~leon_plickat/lavalauncher

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
3 2

[PATCH v2] Add multi finger touch support

Details
Message ID
<20200712220059.1379770-1-nicolai@dagestad.fr>
DKIM signature
pass
Download raw message
Patch: +51 -44
---
As things are implmented here, each touch point is hanlded separately, 
so if you touch one icon with 2 fingers it will be triggered twice.
But if you drag out of the icon's area with any finger, the operation 
for that finger will be canceled.

To support actions with multiple fingers, we would probably need to use 
frames. (I suppose, I have done much/any wayland developpement before .-. )

EDIT: Fixed a missplaced brace from last patch...

 src/input.c | 80 ++++++++++++++++++++++++++---------------------------
 src/seat.c  |  1 +
 src/seat.h  | 14 +++++++---
 3 files changed, 51 insertions(+), 44 deletions(-)

diff --git a/src/input.c b/src/input.c
index c9ba753..21ca762 100644
--- a/src/input.c
+++ b/src/input.c
@@ -249,32 +249,24 @@ static void touch_handle_up (void *raw_data, struct wl_touch *wl_touch,
		uint32_t serial, uint32_t time, int32_t id)
{
	struct Lava_seat *seat = (struct Lava_seat *)raw_data;
	if ( seat->touch.bar == NULL )
	struct Lava_touchpoint *tmp=NULL, *current=NULL;
	wl_list_for_each(tmp, &seat->touch.touchpoints, link)
	{
		fputs("ERROR: Touch-Up event could not be handled: "
				"Bar could not be found.\n", stderr);
		return;
		if ( tmp->id == id )
		{
			current=tmp;
			break;
		}
	}

	if ( current == NULL )
		return;

	if (seat->data->verbose)
		fputs("Touch up.\n", stderr);

	/* If this touch event does not have the same id as the touch-down event
	 * which initiated the touch operation, abort. Also abort if no touch
	 * operation has been initiated.
	 */
	if ( ! seat->touch.item || id != seat->touch.id )
	{
		seat->touch.item = NULL;
		seat->touch.bar  = NULL;
		return;
	}

	/* At this point, we know for sure that we have received a touch-down
	 * and the following touch-up event over the same item, so we can
	 * interact with it.
	 */
	item_interaction(seat->touch.bar, seat->touch.item, TYPE_TOUCH);
	item_interaction(current->bar, current->item, TYPE_TOUCH);
	wl_list_remove(&current->link);
}

static void touch_handle_down (void *raw_data, struct wl_touch *wl_touch,
@@ -283,45 +275,53 @@ static void touch_handle_down (void *raw_data, struct wl_touch *wl_touch,
		wl_fixed_t fx, wl_fixed_t fy)
{
	struct Lava_seat *seat = (struct Lava_seat *)raw_data;

	uint32_t x = wl_fixed_to_int(fx), y = wl_fixed_to_int(fy);

	if (seat->data->verbose)
		fprintf(stderr, "Touch down: x=%d y=%d\n", x, y);

	seat->touch.bar = bar_from_surface(seat->data, surface);
	struct Lava_touchpoint *finger = calloc(1, sizeof(struct Lava_touchpoint));

	seat->touch.id   = id;
	seat->touch.item = item_from_coords(seat->touch.bar, x, y);
	finger->id   = id;
	finger->bar  = bar_from_surface(seat->data, surface);
	finger->item = item_from_coords(finger->bar, x, y);

	wl_list_insert(&seat->touch.touchpoints, &finger->link);
}

static void touch_handle_motion (void *raw_data, struct wl_touch *wl_touch,
		uint32_t time, int32_t id, wl_fixed_t x, wl_fixed_t y)
		uint32_t time, int32_t id, wl_fixed_t fx, wl_fixed_t fy)
{
	struct Lava_seat *seat = (struct Lava_seat *)raw_data;
	struct Lava_touchpoint *tmp=NULL, *current=NULL;
	wl_list_for_each(tmp, &seat->touch.touchpoints, link)
	{
		if ( tmp->id == id )
		{
			current=tmp;
			break;
		}
	}

	if (seat->data->verbose)
		fputs("Touch move.\n", stderr);

	if ( id != seat->touch.id )
	if ( current == NULL )
		return;

	/* The touch point has been moved, meaning it likely is no longer over
	 * the item, so we simply abort the touch operation.
	if (seat->data->verbose)
		fprintf(stderr,"Touch move\n");

	/* If the item under the touch point is not the same we first touched,
	 * we simply abort the touch operation.
	 */
	seat->touch.item = NULL;
	seat->touch.bar  = NULL;
	uint32_t x = wl_fixed_to_int(fx), y = wl_fixed_to_int(fy);
	if ( item_from_coords(current->bar, x, y) != current->item )
		wl_list_remove(&current->link);

}

/* These are the handlers for touch events. We only want to interact with an
 * item, if both touch-down and touch-up were over the same item. To
 * achieve this, touch_handle_down() will store the touch id and the item in
 * the seat. touch_handle_up() checks whether a item is stored in the seat and
 * whether its id is the same as the one stored in the seat and if both are true
 * interacts with the item. touch_handle_motion() will simply remove the item
 * from the seat, effectively aborting the touch operation.
 *
 * Behold: Since I lack the necessary hardware, touch support is untested.
 * achieve this, each touch event is stored in the wl_list, inside seat->touch.
 * This ways we can follow each of them without needing any extra logic.
 */
const struct wl_touch_listener touch_listener = {
	.down        = touch_handle_down,
diff --git a/src/seat.c b/src/seat.c
index 0a83a29..695b6e4 100644
--- a/src/seat.c
+++ b/src/seat.c
@@ -97,6 +97,7 @@ bool create_seat (struct Lava_data *data, struct wl_registry *registry,

	seat->data    = data;
	seat->wl_seat = wl_seat;
	wl_list_init(&seat->touch.touchpoints);
	wl_list_insert(&data->seats, &seat->link);
	wl_seat_add_listener(wl_seat, &seat_listener, seat);

diff --git a/src/seat.h b/src/seat.h
index ae6f622..2eb58ef 100644
--- a/src/seat.h
+++ b/src/seat.h
@@ -55,13 +55,19 @@ struct Lava_seat

	struct
	{
		struct wl_touch  *wl_touch;
		int32_t           id;
		struct Lava_bar  *bar;
		struct Lava_item *item;
		struct wl_touch *wl_touch;
		struct wl_list   touchpoints;
	} touch;
};

struct Lava_touchpoint 
{
	struct wl_list    link;
	int32_t           id;
	struct Lava_bar  *bar;
	struct Lava_item *item;
};

bool create_seat (struct Lava_data *data, struct wl_registry *registry,
		uint32_t name, const char *interface, uint32_t version);
void destroy_all_seats (struct Lava_data *data);
-- 
2.27.0
Details
Message ID
<C452OLN0YWI9.2D395A1LY992D@tarazed>
In-Reply-To
<20200712220059.1379770-1-nicolai@dagestad.fr> (view parent)
DKIM signature
missing
Download raw message
Thank you very much. Overall this looks good to me and the behaviour
you describe sounds sane. I am especially happy that this fixes two
issues at once: Multiple touch points and aborting on motion being way
to sensitive. I can not test it since I lack touch hardware, but I
trust you did test this. There are just some minor fix-ups needed
after which I will commit your patch.

On Mon Jul 13, 2020 at 2:00 AM CEST, Nicolai Dagestad wrote:
> To support actions with multiple fingers, we would probably need to use
> frames.

Frames would be needed indeed. But for now I say we wait until there
is a reasonable use case for multi-touch before we overcomplicate
touch handling.


On Mon Jul 13, 2020 at 2:00 AM CEST, Nicolai Dagestad wrote:
> + struct Lava_touchpoint *tmp=NULL, *current=NULL;
> + wl_list_for_each(tmp, &seat->touch.touchpoints, link)
> {
> - fputs("ERROR: Touch-Up event could not be handled: "
> - "Bar could not be found.\n", stderr);
> - return;
> + if ( tmp->id == id )
> + {
> + current=tmp;
> + break;
> + }

I am not too happy about two Lava_touchpoint pointers. I'd rather have
a solution using goto, because that would be more verbose and
self-explanatory while needing less lines. Something akin to the
following:

	struct Lava_touchpoint *current = NULL;
	wl_list_for_each(current, &seat->touch.touchpoints, link)
	{
		if ( current->id == id )
			goto touchpoint_found;
	}

	return;

	touchpoint_found:
		// Rest of the code...


On Mon Jul 13, 2020 at 2:00 AM CEST, Nicolai Dagestad wrote:
> + struct Lava_touchpoint *finger = calloc(1, sizeof(struct
> Lava_touchpoint));

I am not too happy about calling this "finger". I would prefer
"touchpoint" or maybe "tp" for brevity.


On Mon Jul 13, 2020 at 2:00 AM CEST, Nicolai Dagestad wrote:
> + struct Lava_touchpoint *tmp=NULL, *current=NULL;
> + wl_list_for_each(tmp, &seat->touch.touchpoints, link)
> + {
> + if ( tmp->id == id )
> + {
> + current=tmp;
> + break;
> + }
> + }

See above.


On Mon Jul 13, 2020 at 2:00 AM CEST, Nicolai Dagestad wrote:
> + if ( item_from_coords(current->bar, x, y) != current->item )
> + wl_list_remove(&current->link);

I am pretty sure this is a memory leak. You remove the touch point,
but you do not free it.


And since we are at the topic of freeing: If LavaLauncher exits while
touchpoints still exist, they will also not be freed. Consider adding
some sort of destroy_all_touchpoints() function which gets called in
destroy_seat() (the function would go into seat.c). Maybe it would
make sense to have a create_touchpoint() and destroy_touchpoint()
function, but that I leave up to you.

Also please consider adding your name to the copyright header of
input.c, as your contribution is kinda important.

Thanks again for your contribution.


Friendly greetings,
Leon Plickat
Details
Message ID
<C45DNUH9N51H.3DQ2BL5UX3ZGX@bladestealth>
In-Reply-To
<C452OLN0YWI9.2D395A1LY992D@tarazed> (view parent)
DKIM signature
pass
Download raw message
On Mon Jul 13, 2020 at 2:34 AM CEST, Leon Plickat wrote:
> I am pretty sure this is a memory leak. You remove the touch point,
> but you do not free it.

Seems like one indeed


> And since we are at the topic of freeing: If LavaLauncher exits while
> touchpoints still exist, they will also not be freed. Consider adding
> some sort of destroy_all_touchpoints() function which gets called in
> destroy_seat() (the function would go into seat.c). Maybe it would
> make sense to have a create_touchpoint() and destroy_touchpoint()
> function, but that I leave up to you.

I'm not against adding this. Is there something special in the wayland 
world that makes ressources not be free automaticcaly when a process 
exits ?


> Also please consider adding your name to the copyright header of
> input.c, as your contribution is kinda important.

I'll address theese points for the next patch.
Details
Message ID
<C45GYOT86MQA.1K5REU043N5AH@tarazed>
In-Reply-To
<C45DNUH9N51H.3DQ2BL5UX3ZGX@bladestealth> (view parent)
DKIM signature
missing
Download raw message
On Mon Jul 13, 2020 at 1:10 PM CEST, Nicolai Dagestad wrote:
> I'm not against adding this. Is there something special in the wayland
> world that makes ressources not be free automaticcaly when a process
> exits ?

No, it is generally just good practice to clean up any allocated heap
object, despite the operating system de-allocing the memory on exit
anyway.

Another problem is reloading. When LavaLauncher reloads (due to a
config file change) it terminates the Wayland connection and restarts
it. Not cleaning up touchpoints would be another memory leak.

On Mon Jul 13, 2020 at 1:10 PM CEST, Nicolai Dagestad wrote:
> I'll address theese points for the next patch.

I am looking forward to it!


Friendly greetings,
Leon Plickat
Review patch Export thread (mbox)