Yeah, the MetaWindowActor needs to queue_redraw() on itself when meta_window_appears_focused(priv->window) changes.

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

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

Created attachment 184208 [details] [review]
window: add an appears-focused property, redraw shadows when it changes

We need to redraw a window's shadow any time the value of
meta_window_appears_focused() changes. So make that into a property so
we can get notifications on it.

====

Not necessarily for 3.0.0, just something I decided to fix while bugzilla
was down, because it annoyed me.

sync_focus_appearance() is perhaps overly cautious; it seems like only
one window should be able to have has_focus set at any given time (or at
least, any situation where multiple windows have has_focus set is very
transient and will probably disappear before the next redraw), and it
could be optimized a bit if that's true. But I wasn't sure if there was
some scenario where a window and its transient could both be considered
has_focus...

Review of attachment 184208 [details] [review]:

Ah - didn't twig onto the fact that a change in focus would cause the frame to be redrawn, but that would go through XDamage and be a *clipped* redraw and not redraw the shadow. Now that you've pointed this out with clicking on notifications, it's going to annoy the heck out of me, at least in the rare case where I have non-maximized windows. Also easily reproducible with a window always on top - the reason it normally doesn't show up is that restacking causes a total repaint of the MetaWindowGroup.

Hmm, does this work right if a focusd dialog is unmanaged? I guess the X server reliably sends a focus out before unmapping or destroying a window, which are the only cases where we unmanage, so no real need to worry about that.

What it doesn't handle is the case where the transient_for hint is changed on the fly. A corner cases, but probably makes sense to handle rather than leaving for someone to debug late for some app which is tardy in setting the hint.

Undecided if this is 3.0.0 or 3.0.1 at this point - my feeling is that it's more like 3.0.0 - it's the kind of thing that catches the eye of someone testing the message tray and breaks the illusion, but see what you think is important in the issues I've pointed. If we can't converge pretty quickly on what's "good enough" then it will definitely be 3.0.1.

::: src/core/window.c
@@ +6394,3 @@
+  child = window;
+  parent = meta_window_get_transient_for (child);
+  while (child->type == META_WINDOW_MODAL_DIALOG && parent && parent != child)

Not a new problem but the parent != child doesn't keep us out of infinite loops. Add a tortoise variable updated every other loop cycle? I don't *obviously* see other places in Mutter that are vulnerable to infinite transient for loops, assuming that the stacking and placement constraint code is sound against contradictory constraints.

@@ +6396,3 @@
+  while (child->type == META_WINDOW_MODAL_DIALOG && parent && parent != child)
+    {
+      gboolean parent_new_attached_focus = child->has_focus || child->has_attached_focus;

Doesn't deal with parents with multiple attached children - really need an attached focus count on the parent rather than a boolean. (If we care.. the two attached children will also be stacked over each other in a completely messy ugly fashion. Fine to just document not caring.)

@@ +6398,3 @@
+      gboolean parent_new_attached_focus = child->has_focus || child->has_attached_focus;
+
+      if (parent->has_attached_focus == parent_new_attached_focus)

Hmm, more accurate to break on:

 (parent->has_attached_focus && parent->has_focus) == (parent_new_attached_focus && parent->has_focus)

though you still have to update parent->has_attached_focus in that case. So, like

 if (parent->has_focus) {
    parent->has_attached_focus = parent_new_attached_focus;
    break;
 }

and then skip the later check.

Created attachment 184303 [details] [review]
window: add an appears-focused property, redraw shadows when it changes

We need to redraw a window's shadow any time the value of
meta_window_appears_focused() changes. So make that into a property so
we can get notifications on it.

(In reply to comment #5)
> Not a new problem but the parent != child doesn't keep us out of infinite
> loops.

> Doesn't deal with parents with multiple attached children

OK, new version should fix both bugs by keeping track of the specific focused descendant window that is causing us to be appears-focus.

    - When an attached child gains focus, it sets each
      parent->attached_focus_window to the focus window, until it reaches
      the top or finds a window that already has the right
      attached_focus_window, which will break any loops

    - When the attached child loses focus, it sets each
      parent->attached_focus_window to NULL, until it reaches the top
      of finds a window that didn't have this window as its
      attached_focus_window

In both cases, the second break condition will trigger if it sees the value it wrote before, so we can't get caught in a loop.

In the multiple-attached-children case, even if we get the focus-in before the focus-out, it still works, because the focus-in will overwrite parent->attached_focus_window to point to the new child, but the focus-out won't overwrite it with NULL, because it no longer matches the right child.

Created attachment 184459 [details]
test case changing the transient_for hint on the fly

This test case shows that since we propagate focus appearance after updating the transient for hint, we can leave a parent dialog "stuck" with attached_modal_dialog set.

Though I also get stuck focus appearance when I simply close an attached modal dialog - say like the gedit file save dialog.

Created attachment 184462 [details]
Test case with a focus chain loop

Here's a test case with a loop of three attached modal dialogs - some things are definitely related to constraints/positioning, but no hang, which is the important thing.

Review of attachment 184303 [details] [review]:

This feels like a robust approach to me, and the only bug I saw in the code was that meta_window_propagate_focus_appearance() is called from window-props.c after xtransient_for is already changed. But there seems to be another bug where closing an attached modal dialog leaves the parent with a stuck attached_focus_window, which isn't obvious to me without debugging.

Created attachment 184463 [details]
Corrected version of the transient-for loop test case

Last version of the transient-for-loop test case didn't actually work - though it seems like a GTK+ or Mutter bug that it didn't:

w1 = gtk.Window()
w1.set_type_hint(gtk.gdk.WINDOW_TYPE_HINT_DIALOG)
w1.set_modal(True)
w1.show_all()

w2 = gtk.Window()
w2.show_all()
w1.set_transient_for (w2)

Produces:

Invalid WM_TRANSIENT_FOR window 0x2800020 specified for 0x2800003 (transient.).

Changing the second show_all() to a show_now() [so wait until we get a MapEvent back from the window manager] makes it work. Here's the focus loop corrected, and it crashes mutter with your patch applied. Haven't checked yet if it crashes in the focus appearance code or not.

(In reply to comment #12)
> Changing the second show_all() to a show_now() [so wait until we get a MapEvent
> back from the window manager] makes it work. Here's the focus loop corrected,
> and it crashes mutter with your patch applied. Haven't checked yet if it
> crashes in the focus appearance code or not.

Infinite recursion through:

 meta_window_move
 meta_window_move_resize_internal
 move_attached_dialog
 meta_window_move
 [...]

So unrelated to this patch and this bug.

Created attachment 184475 [details] [review]
window: add an appears-focused property, redraw shadows when it changes

- add a flag to propagate_focus_appearance() so we can control
    whether it propagate "focus" or "unfocus"

  - call propagate_focus_appearance() both at the start and the end of
    reload_transient_for(), to remove focus appearance from previous
    parent (if any), and add it to the new parent (if any)

  - call propagate_focus_appearance() from meta_window_unmanage(),
    because even if the window does get a FocusOut on destroy, we
    won't be paying attention to it any more at that point.

Review of attachment 184475 [details] [review]:

Good for me pending release-team approval
http://mail.gnome.org/archives/release-team/2011-March/msg00475.html

Review of attachment 184475 [details] [review]:

Has release team sign-off 

http://mail.gnome.org/archives/release-team/2011-March/msg00475.html

Attachment 184475 [details] pushed as f464b85 - window: add an appears-focused property, redraw shadows when it changes