GNOME Bugzilla – Bug 681475
Event area not allocated correctly for monitor layout
Last modified: 2012-09-16 21:10:14 UTC
Created attachment 220722 [details] [review] Patch to fix the issue The event area is not done correctly for the monitor layout. It needs to take into account a lot of things, but does nothing in that regard. Without the patch you cannot properly drag the screens around. Doing one drag operation is fine, but after that the event areas are entirely wrong and you basically cannot start a new drag operation. Attaching a patch that fixes the issue.
Review of attachment 220722 [details] [review]: Can't comment on the process, as I don't know that portion of the code, but there seems to be at least one bug in the patch. ::: panels/display/scrollarea.c @@ +1185,3 @@ +typedef struct { + cairo_t *cr; + GtkAllocation allocation; I'd rather this be GtkAllocation *allocation. @@ +1190,3 @@ static void user_to_device (double *x, double *y, gpointer data) Rename to user_data here. @@ +1193,3 @@ { + gdouble ox, oy; + cairo_t *cr = ((user_to_device_data*)data)->cr; And do: user_to_device_data *data = user_data; Which means you can reference ->cr directly, without casting, or at least avoid assigning inside the declarations. @@ +1194,3 @@ + gdouble ox, oy; + cairo_t *cr = ((user_to_device_data*)data)->cr; + GtkAllocation allocation = ((user_to_device_data*)data)->allocation; If you use a GtkAllocation here, instead of a pointer to, you'll be copying by value each member of the struct. Pretty sure that's not the intended effect.
Created attachment 222193 [details] [review] Updated patch Good point. I have done the suggested changes.
I'm seeing a problem where I can neither drag monitors around, nor grab the panel and move it to a different monitor. This patch makes my problem go away.
Good catch, it fixed bugs I didn't know we had :)
This patch breaks the widget here. It seems there's an offset to the input area. Reverting it makes it work correctly.
Rui, could you provide a bit more information? What is your GTK+/GDK version, what is the direction of the offset? Does it maybe work for a monitor after it has been dragged? The page should take all the transformations into account that happen. I did encouter this weird thing, that GDK seems to handle a full window redraw differently to a partial redraw. In the one case, the cairo context was simply translated, in the other the device offset was changed.
(In reply to comment #6) > Rui, could you provide a bit more information? What is your GTK+/GDK version, > what is the direction of the offset? Does it maybe work for a monitor after it > has been dragged? gtk+ is 3.5.14 and the offset is there the same after dragging stuff around. The offset seems to be the distance from the toplevel window's left and top edges to the left and top edge of the FooScroolArea. Hmmm, and now that I'm playing with it, it seems that the offset gets "fixed" after I switch to another toplevel window and come back to g-c-c. Then it's wrong again if I go to g-c-c's overview and come back to the display panel.
That kind of sounds reversed to the issue that I have without the patch (GTK+ version 3.4.2 currently). I guess the code in the patch will fail if GDK creates the double buffering surface only for part of the window. Then getting the device offset from cairo will result in a wrong result. Instead of getting all the offsets later on, one could save the transformation at the start of foo_scroll_area_draw function and then subtract this. That way any changes in the backing surface wouldn't affect the transformation code. This whole foo_scroll_area seems a bit broken. It even has a backing surface which is never drawn because there is a cairo_fill and not cairo_paint in line 512. And I don't think the backing buffer ever gets any content.
One thing that might be good to investigate is what changed here to break things. I clearly remember the display panel working fine in 3.4. Did some gtk change invalidate assumptions in the scrollarea code ? Or something in cairo ?
I'm now seeing offset problems again myself. Maybe my testing of this patch wasn't thorough enough.
After some investigation it seems that there is a cairo bug (and the previous patch simply worked around it by accident). GTK+ passes the draw() function a cairo_context which is translated so that the (0, 0) coordinate is at the top left corner. ie. the transformation matrix is a shift by allocation.{x,y}. Now when a partial redraw happens, GDK only allocates big enough pixmap for the exposed area. It then sets a device_offset on the target pixmap, so that the user coordinates are again in pixel. What happens now is that cairo has three different coordinate systems: user, device, backend. The user coordinate system is relative to the widget, the device coordinate system is relative to the window and the backend coordinate system is relative to the target pixmap that is used for double buffering. What we are seeing is the offset between the backend and device coordinate systems. This is because cairo seems to convert the path from user->backend coordinates, but in "cairo_get_path" it converts from device->user system. The relevant cairo bug is https://bugs.freedesktop.org/show_bug.cgi?id=54732
Benjamin, I'm guessing that the patch needs to be reverted then, correct?
Created attachment 223916 [details] [review] patch on top of previous patch (222193) Not quite. The previous code did not apply any transformation matrix. And that should break for monitors that are rotated. This patch (which simply removes the translation based on the device offset) should work fine once cairo is fixed.
(In reply to comment #13) > Created an attachment (id=223916) [details] [review] > patch on top of previous patch (222193) > > Not quite. The previous code did not apply any transformation matrix. And that > should break for monitors that are rotated. > > This patch (which simply removes the translation based on the device offset) > should work fine once cairo is fixed. It is fixed now...
Oh, I forgot to remove the variables ox and oy in the last patch.
Bastien, I guess we should get the last patch in ? Ickle has said that we'll get a cairo release with the fix as soon as he's fixed some printing deadlock.