Merge lp:~a-j-buxton/ubiquity/struts into lp:ubiquity

[Alistair Buxton]

This patch changes Ubiquity panel so that it doesn't set two overlapping struts on the top and left sides. Doing so confuses xfwm4.

This is a one line patch but it is quite difficult to explain why it is the correct thing to do. Also, while it is technically correct, it is possible the existing code is doing the wrong thing for a reason.

Background: struts are reserved areas of the display for panels etc. Struts are set using X11 atoms, _NET_WM_STRUT and _NET_WM_STRUT_PARTIAL.

_NET_WM_STRUT contains four cardinals which define the width of borders on the left, right, top and bottom edges of the screen.

_NET_WM_STRUT_PARTIAL contains the same four cardinals, followed by a further eight, which define the portion of the screen edge covered by each strut.

Here's a diagram showing what is happening:

https://bugs.launchpad.net/ubuntu/+source/ubiquity/+bug/1425690/+attachment/4328715/+files/rect2985.png

Basically, Ubiquity sets up the struts in a rather strange way. It creates a LEFT and a TOP strut which perfectly overlap in _NET_WM_STRUT_PARTIAL, but it then copies the first four cardinals from _NET_WM_STRUT_PARTIAL to _NET_WM_STRUT, which creates a LEFT strut which covers the whole screen.

According to the spec, window managers MUST ignore _NET_WM_STRUT if _NET_WM_STRUT_PARTIAL is set, which means it's kind of pointless to set both. If for some reason a WM only understands _NET_WM_STRUT, then Ubiquity has put bogus information in to it anyway. It is also rather pointless to set two struts which perfectly overlap.

So this patch simply prevents makes Ubiquity define the LEFT strut to be 0 and 0,0,0. That means only the TOP strut is defined. Since it isn't a partial strut, that means both atoms will now contain reasonable values.

So this is all technically correct up to now. The question is whether there is a reason why Ubiquity is setting overlapping struts this way. Checking the source history shows it has always done this since the code was first commited, so it seems unlikely to be part of a workaround, and more likely to just be an oversight.

Also note that I haven't tested this due to the complexity of replacing files on a running live install.

[Dimitri John Ledkov (xnox)]

I'm pretty certain these are similar to crazy things that I saw when I was porting ubiquity panel to run under compiz.

Specifically e.g. commit like this one:
http://bazaar.launchpad.net/~ubuntu-installer/ubiquity/trunk/revision/5778#src/panel/panel.c

However these strut bits are there to convince gtk to draw it as a "panel" rather than a window, and i'm pretty sure this was "inspired" by gnome-panel code for struts.

Can we drop that code altogether? And just keep the set_size_request and that's it?

I know that rotation works with this code, e.g. rotating screen with accelometer resizes the panel (as can be seen on nexus 7 desktop images from some time ago, these are not published any more).

[Alistair Buxton (a-j-buxton)]

I just noticed that my patch won't actually work because set_strut tries to force LEFT to be not zero here:

http://bazaar.launchpad.net/~ubuntu-installer/ubiquity/trunk/view/head:/src/panel/panel.c#L71

So the result is with this patch the struts won't be set at all.

[Adam Conrad (adconrad)]

Rejected based on the submitter's last comment.

Unmerged revisions

6275. By Alistair Buxton on 2015-02-26

Don't set two overlapping struts.

=== modified file 'src/panel/panel.c'
--- src/panel/panel.c	2015-01-05 13:31:55 +0000
+++ src/panel/panel.c	2015-02-26 21:04:09 +0000
@@ -254,7 +254,7 @@
 	gtk_widget_get_allocation(win, &allocation);
 	width = gdk_screen_width();
 	gtk_window_set_decorated (GTK_WINDOW (win), FALSE);
-	set_strut(GTK_WINDOW(win), width, 0, allocation.height, allocation.height, 0, width);
+	set_strut(GTK_WINDOW(win), 0, 0, 0, allocation.height, 0, width);
 	gtk_widget_set_size_request(GTK_WIDGET (win), width, -1);
 	// We don't care about showing the panel on all desktops just yet.
 	gtk_window_stick (GTK_WINDOW (win));