Created attachment 209529 [details] [review]
screen: Fix thick border for alt-tab popup

Created attachment 209530 [details] [review]
tabpopup: Fix positioning for alt-tab outline popup

Created attachment 209534 [details] [review]
screen: Fix thick border for alt-tab popup

Since the landing of invisible borders, the math here to calculate
the outline is incorrect. Since this is a minor effect, it's not worth
it to have the complex logic be the same - just hardcode the logic
instead.

Created attachment 209535 [details] [review]
tabpopup: Fix positioning for alt-tab outline popup

The code here was playing a lot of tricks behind GDK's back for
unknown reasons. Replacing the sneaky stuff with more straightforward
and regular code works fine and fixes positioning bugs, so let's just
do that instead.




Add better commit messages.

Review of attachment 209534 [details] [review]:

Fine, but you do need not to leave the comment above describing the way it used to work!

Created attachment 209538 [details] [review]
screen: Fix thick border for alt-tab popup

Since the landing of invisible borders, the math here to calculate
the outline is incorrect. Since this is a minor effect, it's not worth
it to have the complex logic be the same - just hardcode the logic
instead.

I assume this is 3.5.x stuff as it would change the UI?

(In reply to comment #7)
> I assume this is 3.5.x stuff as it would change the UI?

This is about massively obvious bugs - try using alt-Escape. Not subject to the UI freeze as far as I can imagine.

The only use of this outline in the Shell is the hidden Alt+Escape feature, and the feature looks broken and glitched to hell without these patches. I cannot imagine this in any way being a UI break, unless we have "It looks like a monster ate your computer screen if you press Alt+Escape" in the documentation somewhere.

Review of attachment 209535 [details] [review]:

Can you spend a little more time and see if you can track down why this tricks behind GTK+'s (not GDK's) back are no longer working? - it is fairly clear why they are there.

I think the way forward here is to port the whole of tabpopup to clutter actors and move it from ui/ to compositor/ (and to do the highlight as something like meta_window_actor_set_highlighted() to avoid stacking order issues), but that is rather over-ambitious for this point in the cycle, so if we can't figure out some simple fix, I'm OK accepting some redraw regressions - after all it's been much more broken for a release or two.

::: src/ui/tabpopup.c
@@ -487,3 @@
-      gdk_window_move_resize (window,
-                              te->rect.x, te->rect.y,
-                              te->rect.width, te->rect.height);

Here I'm pretty sure that it is using move_resize() to avoid a visible two-step move-resize dance. This is less of a problem with composited single frames, but it's still possible we might get one of the ConfigureNotifies and not the other.

@@ -506,2 @@
-      /* This should piss off gtk a bit, but we don't want to raise
-       * above the tab popup.  So, instead of calling gtk_widget_show,

The comment *describes* why it is playing tricks.

(In reply to comment #10)
> Review of attachment 209535 [details] [review]:
> 
> Can you spend a little more time and see if you can track down why this tricks
> behind GTK+'s (not GDK's) back are no longer working? - it is fairly clear why
> they are there.

I'm not an experienced GTK+/GDK hacker, so I'm not the best person to ask. My best guess is "we were relying on the way GTK+ was using GDK", and that broke some time long ago.

> I think the way forward here is to port the whole of tabpopup to clutter actors
> and move it from ui/ to compositor/ (and to do the highlight as something like
> meta_window_actor_set_highlighted() to avoid stacking order issues), but that
> is rather over-ambitious for this point in the cycle, so if we can't figure out
> some simple fix, I'm OK accepting some redraw regressions - after all it's been
> much more broken for a release or two.

It wouldn't be too hard to do this, but it's not worth it at this point in the cycle in my opinion - there are higher priority issues.

> ::: src/ui/tabpopup.c
> @@ -487,3 @@
> -      gdk_window_move_resize (window,
> -                              te->rect.x, te->rect.y,
> -                              te->rect.width, te->rect.height);
> 
> Here I'm pretty sure that it is using move_resize() to avoid a visible two-step
> move-resize dance. This is less of a problem with composited single frames, but
> it's still possible we might get one of the ConfigureNotifies and not the
> other.

If we want to avoid a two-step move-resize dance, we should add API to GTK+, not go behind its back. We shape it as part of drawing, so even right now it might be possible that we don't get the ShapeNotify, right?

> @@ -506,2 @@
> -      /* This should piss off gtk a bit, but we don't want to raise
> -       * above the tab popup.  So, instead of calling gtk_widget_show,
> 
> The comment *describes* why it is playing tricks.

Yet if you try the patch, it stacks under the tab popup anyway. I don't know why, I just noticed that it worked.

*** Bug 637161 has been marked as a duplicate of this bug. ***

*** Bug 676681 has been marked as a duplicate of this bug. ***

I've got an other user report telling it is broken and two duplicates were opened already. I tested and the patch still applies and works like a charm.

Owen, I understand your concerns about move-resize, but don't you think it would be nice to get this in? Would you mind reviewing the second patch? This results in very obvious bugs like you said in comment #7.

Maybe we could add a note in the patch to explain why move-resize is not used anymore, with a link to this bug. And open a new one for the way forward you suggested, but which can be quite some work?

*** Bug 695415 has been marked as a duplicate of this bug. ***

*** Bug 725388 has been marked as a duplicate of this bug. ***