[PATCH 5/7] shell: update position of surfaces with type none on map()

[Pekka Paalanen]

On Thu, 16 Feb 2012 16:42:03 +0200
Ander Conselvan de Oliveira <conselvan2 at gmail.com> wrote:

> On 02/16/2012 09:56 AM, Pekka Paalanen wrote:
> >> This changes shell->map interface to take sx and sy parameters and
> >> change dekstop shell implementation to update the position of a
> >> surface of type none according to those parameters. Since a surface
> >> of type none won't actually be mapped, the effect of this change is
> >> only visible for surfaces that are made visible by the compositor.
> >>
> >> Signed-off-by: Ander Conselvan de Oliveira<ander.conselvan.de.oliveira at intel.com>
> >> diff --git a/src/shell.c b/src/shell.c
> >> index d48633e..d9ba168 100644
> >> --- a/src/shell.c
> >> +++ b/src/shell.c
> 
> [...]
> 
> >> @@ -1308,6 +1308,10 @@ map(struct weston_shell *base,
> >>   		break;
> >>   	case SHELL_SURFACE_POPUP:
> >>   		shell_map_popup(shsurf, shsurf->popup.time);
> >> +	case SHELL_SURFACE_NONE:
> >> +		weston_surface_set_position(surface,
> >> +					    surface->geometry.x + sx,
> >> +					    surface->geometry.y + sy);
> >
> > I'd like to have a comment here saying that we are ignoring surface
> > transformations on purpose. The pattern "geometry.x + sx" is
> > usually a bug, especially without checking surface->transform.enabled,
> > because it mixes two different coordinate systems.
> 
> I actually had not consider the case where the drag surface is 
> transformed. We definitely want to move it according to the 
> transformation. For instance, the dnd demo uses the flower being dragged 
> as the icon and sets its position so it looks like that it is being 
> moved with the cursor.
> 
> I cooked the attached patch that I can squash into the series. I tested 
> it by adding a rotation to the mouse cursor and checking if the dnd demo 
> behavior was still the same.

When you rotated, around which point did you rotate? The
surface origin, i.e. top-left corner?

Ok, the new patch looks mostly fine, but I just can't quite grasp this
hunk:

@@ -1833,8 +1849,14 @@ weston_input_update_drag_surface(struct wl_input_device *input_device,
 			input_device->drag_surface;
 		device->drag_surface->pickable = 0;
 
+		surface_to_global_float(device->drag_surface, 0, 0,
+					&topl_x, &topl_y);
+		topl_x -= device->drag_surface->geometry.x;
+		topl_y -= device->drag_surface->geometry.y;
+
 		weston_surface_set_position(device->drag_surface,
-					    input_device->x, input_device->y);
+					    input_device->x - topl_x,
+					    input_device->y - topl_y);
 	}

Does it all work so, that when the drag_surface becomes visible, we call
map() with sx, sy being the surface-local coordinates of the hotspot?
So that will call your weston_surface_set_position_rel() to move the
surface so that surface-local hotspot matches with the input device
position? Hmm, no, it doesn't use the input device position there at
all... it just sets the position to something, and then
weston_input_update_drag_surface() actually does it right, I guess.

OK, I can't wrap my head around this tonight.

In the above quoted hunk, why do you transform the surface top-left
corner instead of the surface hotspot? How do you get the surface
hotspot match the input device position?


Btw. is there a weston_surface_update_transform() call somewhere
guaranteeing that the surface geometry is not dirty?
surface_{to,from}_global_float() functions are lower level functions
than surface_{to,from}_global() and do not call it.


Thanks,
pq