Merge pull request #824 from emersion/fix-subsurface-at

Redesign surface input functions

10 files changed, 109 insertions, 193 deletions

diff --git a/include/wlr/types/wlr_surface.h b/include/wlr/types/wlr_surface.h
index 4d03df73..380e5e03 100644
--- a/include/wlr/types/wlr_surface.h
+++ b/include/wlr/types/wlr_surface.h
@@ -122,20 +122,24 @@ void wlr_surface_make_subsurface(struct wlr_surface *surface,
 		struct wlr_surface *parent, uint32_t id);
 
 /**
- * Get the top of the subsurface tree for this surface.
+ * Get the root of the subsurface tree for this surface.
  */
-struct wlr_surface *wlr_surface_get_main_surface(struct wlr_surface *surface);
+struct wlr_surface *wlr_surface_get_root_surface(struct wlr_surface *surface);
 
 /**
- * Find a subsurface within this surface at the surface-local coordinates.
- * Returns the surface and coordinates in the topmost surface coordinate system
- * or NULL if no subsurface is found at that location.
+ * Check if the surface accepts input events at the given surface-local
+ * coordinates. Does not check the surface's subsurfaces.
  */
-struct wlr_subsurface *wlr_surface_subsurface_at(struct wlr_surface *surface,
-		double sx, double sy, double *sub_x, double *sub_y);
+bool wlr_surface_point_accepts_input(struct wlr_surface *surface,
+		double sx, double sy);
 
-bool wlr_surface_point_accepts_input(
-		struct wlr_surface *surface, double sx, double sy);
+/**
+ * Find a surface in this surface's tree that accepts input events at the given
+ * surface-local coordinates. Returns the surface and coordinates in the leaf
+ * surface coordinate system or NULL if no surface is found at that location.
+ */
+struct wlr_surface *wlr_surface_surface_at(struct wlr_surface *surface,
+		double sx, double sy, double *sub_x, double *sub_y);
 
 void wlr_surface_send_enter(struct wlr_surface *surface,
 		struct wlr_output *output);
diff --git a/include/wlr/types/wlr_wl_shell.h b/include/wlr/types/wlr_wl_shell.h
index 63b1a837..89e950c9 100644
--- a/include/wlr/types/wlr_wl_shell.h
+++ b/include/wlr/types/wlr_wl_shell.h
@@ -142,13 +142,13 @@ void wlr_wl_shell_surface_configure(struct wlr_wl_shell_surface *surface,
 	enum wl_shell_surface_resize edges, int32_t width, int32_t height);
 
 /**
- * Find a popup within this surface at the surface-local coordinates. Returns
- * the popup and coordinates in the topmost surface coordinate system or NULL if
- * no popup is found at that location.
+ * Find a surface within this wl-shell surface tree at the given surface-local
+ * coordinates. Returns the surface and coordinates in the leaf surface
+ * coordinate system or NULL if no surface is found at that location.
  */
-struct wlr_wl_shell_surface *wlr_wl_shell_surface_popup_at(
+struct wlr_surface *wlr_wl_shell_surface_surface_at(
 		struct wlr_wl_shell_surface *surface, double sx, double sy,
-		double *popup_sx, double *popup_sy);
+		double *sub_sx, double *sub_sy);
 
 bool wlr_surface_is_wl_shell_surface(struct wlr_surface *surface);
 
diff --git a/include/wlr/types/wlr_xdg_shell.h b/include/wlr/types/wlr_xdg_shell.h
index b779017f..29b54dba 100644
--- a/include/wlr/types/wlr_xdg_shell.h
+++ b/include/wlr/types/wlr_xdg_shell.h
@@ -215,19 +215,19 @@ uint32_t wlr_xdg_toplevel_set_resizing(struct wlr_xdg_surface *surface,
 void wlr_xdg_surface_send_close(struct wlr_xdg_surface *surface);
 
 /**
- * Compute the popup position in surface-local coordinates.
+ * Compute the popup position in its parent's surface-local coordinate system.
  */
 void wlr_xdg_surface_popup_get_position(struct wlr_xdg_surface *surface,
 		double *popup_sx, double *popup_sy);
 
 /**
- * Find a popup within this surface at the surface-local coordinates. Returns
- * the popup and coordinates in the topmost surface coordinate system or NULL if
- * no popup is found at that location.
+ * Find a surface within this xdg-surface tree at the given surface-local
+ * coordinates. Returns the surface and coordinates in the leaf surface
+ * coordinate system or NULL if no surface is found at that location.
  */
-struct wlr_xdg_surface *wlr_xdg_surface_popup_at(
+struct wlr_surface *wlr_xdg_surface_surface_at(
 		struct wlr_xdg_surface *surface, double sx, double sy,
-		double *popup_sx, double *popup_sy);
+		double *sub_x, double *sub_y);
 
 bool wlr_surface_is_xdg_surface(struct wlr_surface *surface);
 
diff --git a/include/wlr/types/wlr_xdg_shell_v6.h b/include/wlr/types/wlr_xdg_shell_v6.h
index 04c1f324..b646f3fe 100644
--- a/include/wlr/types/wlr_xdg_shell_v6.h
+++ b/include/wlr/types/wlr_xdg_shell_v6.h
@@ -233,19 +233,19 @@ uint32_t wlr_xdg_toplevel_v6_set_resizing(struct wlr_xdg_surface_v6 *surface,
 void wlr_xdg_surface_v6_send_close(struct wlr_xdg_surface_v6 *surface);
 
 /**
- * Compute the popup position in surface-local coordinates.
+ * Compute the popup position in its parent's surface-local coordinate system.
  */
 void wlr_xdg_surface_v6_popup_get_position(struct wlr_xdg_surface_v6 *surface,
 		double *popup_sx, double *popup_sy);
 
 /**
- * Find a popup within this surface at the surface-local coordinates. Returns
- * the popup and coordinates in the topmost surface coordinate system or NULL if
- * no popup is found at that location.
+ * Find a surface within this xdg-surface tree at the given surface-local
+ * coordinates. Returns the surface and coordinates in the leaf surface
+ * coordinate system or NULL if no surface is found at that location.
  */
-struct wlr_xdg_surface_v6 *wlr_xdg_surface_v6_popup_at(
+struct wlr_surface *wlr_xdg_surface_v6_surface_at(
 		struct wlr_xdg_surface_v6 *surface, double sx, double sy,
-		double *popup_sx, double *popup_sy);
+		double *sub_x, double *sub_y);
 
 /**
  * Get the geometry for this positioner based on the anchor rect, gravity, and
diff --git a/rootston/desktop.c b/rootston/desktop.c
index f6801332..e9c1ed30 100644
--- a/rootston/desktop.c
+++ b/rootston/desktop.c
@@ -577,42 +577,30 @@ static bool view_at(struct roots_view *view, double lx, double ly,
 		view_sy = ry + (double)box.height/2;
 	}
 
-	if (view->type == ROOTS_XDG_SHELL_V6_VIEW) {
-		double popup_sx, popup_sy;
-		struct wlr_xdg_surface_v6 *popup =
-			wlr_xdg_surface_v6_popup_at(view->xdg_surface_v6,
-				view_sx, view_sy, &popup_sx, &popup_sy);
-
-		if (popup) {
-			*sx = view_sx - popup_sx;
-			*sy = view_sy - popup_sy;
-			*surface = popup->surface;
-			return true;
-		}
-	}
-
-	if (view->type == ROOTS_WL_SHELL_VIEW) {
-		double popup_sx, popup_sy;
-		struct wlr_wl_shell_surface *popup =
-			wlr_wl_shell_surface_popup_at(view->wl_shell_surface,
-				view_sx, view_sy, &popup_sx, &popup_sy);
-
-		if (popup) {
-			*sx = view_sx - popup_sx;
-			*sy = view_sy - popup_sy;
-			*surface = popup->surface;
-			return true;
-		}
-	}
-
-	double sub_x, sub_y;
-	struct wlr_subsurface *subsurface =
-		wlr_surface_subsurface_at(view->wlr_surface,
-			view_sx, view_sy, &sub_x, &sub_y);
-	if (subsurface) {
-		*sx = view_sx - sub_x;
-		*sy = view_sy - sub_y;
-		*surface = subsurface->surface;
+	double _sx, _sy;
+	struct wlr_surface *_surface;
+	switch (view->type) {
+	case ROOTS_XDG_SHELL_V6_VIEW:
+		_surface = wlr_xdg_surface_v6_surface_at(view->xdg_surface_v6,
+			view_sx, view_sy, &_sx, &_sy);
+		break;
+	case ROOTS_XDG_SHELL_VIEW:
+		_surface = wlr_xdg_surface_surface_at(view->xdg_surface,
+			view_sx, view_sy, &_sx, &_sy);
+		break;
+	case ROOTS_WL_SHELL_VIEW:
+		_surface = wlr_wl_shell_surface_surface_at(view->wl_shell_surface,
+			view_sx, view_sy, &_sx, &_sy);
+		break;
+	case ROOTS_XWAYLAND_VIEW:
+		_surface = wlr_surface_surface_at(view->wlr_surface,
+			view_sx, view_sy, &_sx, &_sy);
+		break;
+	}
+	if (_surface != NULL) {
+		*sx = _sx;
+		*sy = _sy;
+		*surface = _surface;
 		return true;
 	}
 
@@ -623,13 +611,6 @@ static bool view_at(struct roots_view *view, double lx, double ly,
 		return true;
 	}
 
-	if (wlr_surface_point_accepts_input(view->wlr_surface, view_sx, view_sy)) {
-		*sx = view_sx;
-		*sy = view_sy;
-		*surface = view->wlr_surface;
-		return true;
-	}
-
 	return false;
 }
 
diff --git a/types/wlr_compositor.c b/types/wlr_compositor.c
index f27655c0..181d0323 100644
--- a/types/wlr_compositor.c
+++ b/types/wlr_compositor.c
@@ -126,7 +126,7 @@ static void subcompositor_get_subsurface(struct wl_client *client,
 		return;
 	}
 
-	if (wlr_surface_get_main_surface(parent) == surface) {
+	if (wlr_surface_get_root_surface(parent) == surface) {
 		wl_resource_post_error(resource,
 			WL_SUBCOMPOSITOR_ERROR_BAD_SURFACE,
 			"%s%d: wl_surface@%d is an ancestor of parent",
diff --git a/types/wlr_surface.c b/types/wlr_surface.c
index e9bd6a66..4d2a20e7 100644
--- a/types/wlr_surface.c
+++ b/types/wlr_surface.c
@@ -137,6 +137,7 @@ static void surface_set_input_region(struct wl_client *client,
 		pixman_region32_t *region = wlr_region_from_resource(region_resource);
 		pixman_region32_copy(&surface->pending->input, region);
 	} else {
+		pixman_region32_fini(&surface->pending->input);
 		pixman_region32_init_rect(&surface->pending->input,
 			INT32_MIN, INT32_MIN, UINT32_MAX, UINT32_MAX);
 	}
@@ -868,43 +869,41 @@ void wlr_surface_make_subsurface(struct wlr_surface *surface,
 }
 
 
-struct wlr_surface *wlr_surface_get_main_surface(struct wlr_surface *surface) {
-	struct wlr_subsurface *sub;
-
-	while (surface && (sub = surface->subsurface)) {
+struct wlr_surface *wlr_surface_get_root_surface(struct wlr_surface *surface) {
+	while (surface != NULL) {
+		struct wlr_subsurface *sub = surface->subsurface;
+		if (sub == NULL) {
+			return surface;
+		}
 		surface = sub->parent;
 	}
+	return NULL;
+}
 
-	return surface;
+bool wlr_surface_point_accepts_input(struct wlr_surface *surface,
+		double sx, double sy) {
+	return sx >= 0 && sx <= surface->current->width &&
+		sy >= 0 && sy <= surface->current->height &&
+		pixman_region32_contains_point(&surface->current->input, sx, sy, NULL);
 }
 
-struct wlr_subsurface *wlr_surface_subsurface_at(struct wlr_surface *surface,
+struct wlr_surface *wlr_surface_surface_at(struct wlr_surface *surface,
 		double sx, double sy, double *sub_x, double *sub_y) {
 	struct wlr_subsurface *subsurface;
 	wl_list_for_each(subsurface, &surface->subsurface_list, parent_link) {
 		double _sub_x = subsurface->surface->current->subsurface_position.x;
 		double _sub_y = subsurface->surface->current->subsurface_position.y;
-		struct wlr_subsurface *sub =
-			wlr_surface_subsurface_at(subsurface->surface, _sub_x + sx,
-				_sub_y + sy, sub_x, sub_y);
-		if (sub) {
-			// TODO: This won't work for nested subsurfaces. Convert sub_x and
-			// sub_y to the parent coordinate system
+		struct wlr_surface *sub = wlr_surface_surface_at(subsurface->surface,
+			sx - _sub_x, sy - _sub_y, sub_x, sub_y);
+		if (sub != NULL) {
 			return sub;
 		}
+	}
 
-		int sub_width = subsurface->surface->current->buffer_width;
-		int sub_height = subsurface->surface->current->buffer_height;
-		if ((sx > _sub_x && sx < _sub_x + sub_width) &&
-				(sy > _sub_y && sy < _sub_y + sub_height)) {
-			if (pixman_region32_contains_point(
-						&subsurface->surface->current->input,
-						sx - _sub_x, sy - _sub_y, NULL)) {
-				*sub_x = _sub_x;
-				*sub_y = _sub_y;
-				return subsurface;
-			}
-		}
+	if (wlr_surface_point_accepts_input(surface, sx, sy)) {
+		*sub_x = sx;
+		*sub_y = sy;
+		return surface;
 	}
 
 	return NULL;
@@ -952,10 +951,3 @@ void wlr_surface_set_role_committed(struct wlr_surface *surface,
 	surface->role_committed = role_committed;
 	surface->role_data = role_data;
 }
-
-bool wlr_surface_point_accepts_input(
-		struct wlr_surface *surface, double sx, double sy) {
-	return sx >= 0 && sx <= surface->current->width &&
-		sy >= 0 && sy <= surface->current->height &&
-		pixman_region32_contains_point(&surface->current->input, sx, sy, NULL);
-}
diff --git a/types/wlr_wl_shell.c b/types/wlr_wl_shell.c
index a2123bce..2cbdaf3a 100644
--- a/types/wlr_wl_shell.c
+++ b/types/wlr_wl_shell.c
@@ -658,42 +658,23 @@ void wlr_wl_shell_surface_configure(struct wlr_wl_shell_surface *surface,
 	wl_shell_surface_send_configure(surface->resource, edges, width, height);
 }
 
-struct wlr_wl_shell_surface *wlr_wl_shell_surface_popup_at(
+struct wlr_surface *wlr_wl_shell_surface_surface_at(
 		struct wlr_wl_shell_surface *surface, double sx, double sy,
-		double *popup_sx, double *popup_sy) {
+		double *sub_sx, double *sub_sy) {
 	struct wlr_wl_shell_surface *popup;
 	wl_list_for_each(popup, &surface->popups, popup_link) {
 		if (!popup->popup_mapped) {
 			continue;
 		}
-		double _popup_sx = popup->transient_state->x;
-		double _popup_sy = popup->transient_state->y;
-		int popup_width =
-			popup->surface->current->buffer_width;
-		int popup_height =
-			popup->surface->current->buffer_height;
-
-		struct wlr_wl_shell_surface *_popup =
-			wlr_wl_shell_surface_popup_at(popup,
-				popup->transient_state->x,
-				popup->transient_state->y,
-				popup_sx, popup_sy);
-		if (_popup) {
-			*popup_sx = sx + _popup_sx;
-			*popup_sy = sy + _popup_sy;
-			return _popup;
-		}
 
-		if ((sx > _popup_sx && sx < _popup_sx + popup_width) &&
-				(sy > _popup_sy && sy < _popup_sy + popup_height)) {
-			if (pixman_region32_contains_point(&popup->surface->current->input,
-						sx - _popup_sx, sy - _popup_sy, NULL)) {
-				*popup_sx = _popup_sx;
-				*popup_sy = _popup_sy;
-				return popup;
-			}
+		double popup_sx = popup->transient_state->x;
+		double popup_sy = popup->transient_state->y;
+		struct wlr_surface *sub = wlr_wl_shell_surface_surface_at(popup,
+			sx - popup_sx, sy - popup_sy, sub_sx, sub_sy);
+		if (sub != NULL) {
+			return sub;
 		}
 	}
 
-	return NULL;
+	return wlr_surface_surface_at(surface->surface, sx, sy, sub_sx, sub_sy);
 }
diff --git a/types/wlr_xdg_shell.c b/types/wlr_xdg_shell.c
index d70021fd..f85c6acf 100644
--- a/types/wlr_xdg_shell.c
+++ b/types/wlr_xdg_shell.c
@@ -1638,45 +1638,24 @@ void wlr_xdg_surface_popup_get_position(struct wlr_xdg_surface *surface,
 		surface->geometry.y;
 }
 
-struct wlr_xdg_surface *wlr_xdg_surface_popup_at(
+struct wlr_surface *wlr_xdg_surface_surface_at(
 		struct wlr_xdg_surface *surface, double sx, double sy,
-		double *popup_sx, double *popup_sy) {
-	// XXX: I think this is so complicated because we're mixing geometry
-	// coordinates with surface coordinates. Input handling should only deal
-	// with surface coordinates.
+		double *sub_x, double *sub_y) {
 	struct wlr_xdg_popup *popup_state;
 	wl_list_for_each(popup_state, &surface->popups, link) {
 		struct wlr_xdg_surface *popup = popup_state->base;
 
-		double _popup_sx =
-			surface->geometry.x + popup_state->geometry.x;
-		double _popup_sy =
-			surface->geometry.y + popup_state->geometry.y;
-		int popup_width =  popup_state->geometry.width;
-		int popup_height =  popup_state->geometry.height;
-
-		struct wlr_xdg_surface *_popup =
-			wlr_xdg_surface_popup_at(popup,
-				sx - _popup_sx + popup->geometry.x,
-				sy - _popup_sy + popup->geometry.y,
-				popup_sx, popup_sy);
-		if (_popup) {
-			*popup_sx = *popup_sx + _popup_sx - popup->geometry.x;
-			*popup_sy = *popup_sy + _popup_sy - popup->geometry.y;
-			return _popup;
-		}
+		double popup_sx, popup_sy;
+		wlr_xdg_surface_popup_get_position(popup, &popup_sx, &popup_sy);
 
-		if ((sx > _popup_sx && sx < _popup_sx + popup_width) &&
-				(sy > _popup_sy && sy < _popup_sy + popup_height)) {
-			if (pixman_region32_contains_point(&popup->surface->current->input,
-						sx - _popup_sx + popup->geometry.x,
-						sy - _popup_sy + popup->geometry.y, NULL)) {
-				*popup_sx = _popup_sx - popup->geometry.x;
-				*popup_sy = _popup_sy - popup->geometry.y;
-				return popup;
-			}
+		struct wlr_surface *sub = wlr_xdg_surface_surface_at(popup,
+			sx - popup_sx + popup->geometry.x,
+			sy - popup_sy + popup->geometry.y,
+			sub_x, sub_y);
+		if (sub != NULL) {
+			return sub;
 		}
 	}
 
-	return NULL;
+	return wlr_surface_surface_at(surface->surface, sx, sy, sub_x, sub_y);
 }
diff --git a/types/wlr_xdg_shell_v6.c b/types/wlr_xdg_shell_v6.c
index 5c83db70..fa5523fc 100644
--- a/types/wlr_xdg_shell_v6.c
+++ b/types/wlr_xdg_shell_v6.c
@@ -1602,47 +1602,26 @@ void wlr_xdg_surface_v6_popup_get_position(struct wlr_xdg_surface_v6 *surface,
 		surface->geometry.y;
 }
 
-struct wlr_xdg_surface_v6 *wlr_xdg_surface_v6_popup_at(
+struct wlr_surface *wlr_xdg_surface_v6_surface_at(
 		struct wlr_xdg_surface_v6 *surface, double sx, double sy,
-		double *popup_sx, double *popup_sy) {
-	// XXX: I think this is so complicated because we're mixing geometry
-	// coordinates with surface coordinates. Input handling should only deal
-	// with surface coordinates.
+		double *sub_x, double *sub_y) {
 	struct wlr_xdg_popup_v6 *popup_state;
 	wl_list_for_each(popup_state, &surface->popups, link) {
 		struct wlr_xdg_surface_v6 *popup = popup_state->base;
 
-		double _popup_sx =
-			surface->geometry.x + popup_state->geometry.x;
-		double _popup_sy =
-			surface->geometry.y + popup_state->geometry.y;
-		int popup_width =  popup_state->geometry.width;
-		int popup_height =  popup_state->geometry.height;
-
-		struct wlr_xdg_surface_v6 *_popup =
-			wlr_xdg_surface_v6_popup_at(popup,
-				sx - _popup_sx + popup->geometry.x,
-				sy - _popup_sy + popup->geometry.y,
-				popup_sx, popup_sy);
-		if (_popup) {
-			*popup_sx = *popup_sx + _popup_sx - popup->geometry.x;
-			*popup_sy = *popup_sy + _popup_sy - popup->geometry.y;
-			return _popup;
-		}
+		double popup_sx, popup_sy;
+		wlr_xdg_surface_v6_popup_get_position(popup, &popup_sx, &popup_sy);
 
-		if ((sx > _popup_sx && sx < _popup_sx + popup_width) &&
-				(sy > _popup_sy && sy < _popup_sy + popup_height)) {
-			if (pixman_region32_contains_point(&popup->surface->current->input,
-						sx - _popup_sx + popup->geometry.x,
-						sy - _popup_sy + popup->geometry.y, NULL)) {
-				*popup_sx = _popup_sx - popup->geometry.x;
-				*popup_sy = _popup_sy - popup->geometry.y;
-				return popup;
-			}
+		struct wlr_surface *sub = wlr_xdg_surface_v6_surface_at(popup,
+			sx - popup_sx + popup->geometry.x,
+			sy - popup_sy + popup->geometry.y,
+			sub_x, sub_y);
+		if (sub != NULL) {
+			return sub;
 		}
 	}
 
-	return NULL;
+	return wlr_surface_surface_at(surface->surface, sx, sy, sub_x, sub_y);
 }
 
 void wlr_xdg_popup_v6_get_anchor_point(struct wlr_xdg_popup_v6 *popup,