I've complained about this long ago...

Created attachment 185200 [details] [review]
window: only attach dialogs to NORMAL, DIALOG, and MODAL_DIALOG windows

Attaching dialogs to unusual windows (like the desktop) looks bad, so
don't do it. Add meta_window_is_attached_dialog() with all of the
correct logic for deciding whether or not to attach a window, and use
that in several places.

Created attachment 185201 [details] [review]
windowManager: use meta_window_is_attached_dialog()

Use meta_window_is_attached_dialog() so that we only dim/unfold dialog
windows that mutter is actually showing as attached

Review of attachment 185200 [details] [review]:

window.c:move_attached_dialog() needs a fix, or it will move dialogs that are detached by this patch to 0,0.

What's the effect on meta_window_propagate_focus_appearance()? It looks like to me that the parent will "appear focused" despite the lack of attachment because attached_focus_window will be set.

::: src/core/constraints.c
@@ -755,3 @@
-  if (!meta_prefs_get_attach_modal_dialogs ())
-    return TRUE;
-  if (window->type != META_WINDOW_MODAL_DIALOG || !parent || parent == window)

The parent == window check here is clearly ineffective for more complicated chains of loope attached windows, but I'm not comfortable removing it in the 3.0.x timescale. (And testing didn't find anything *too* bad with looped chains of attached windows when I wrote some test cases a few weeks ago, for whatever reason)

::: src/core/window.c
@@ +10227,3 @@
+    return FALSE;
+
+  switch (parent->type)

By adding this check, we've added an additional way that a window can transition from attached to non-attached or vice versa. Probably a bad idea to worry about fixing up for 3.0.x, but long term I think it would be best to see the tracking be accurate and correct rather than having a random subset of things work and another random subset not.

Review of attachment 185201 [details] [review]:

Patch certainly doesn't handle complicated cases where the parent changes window types or the transient-for setup changes - seems to get that to work, we'd need a notified ::has-attached-dialogs property mantained by the mutter core. But it seems like it shouldn't make things worse.

Patch needs to up the mutter required version in configure.ac, but I guess can't until that lands and we know what the appropriate version would be.

::: js/ui/windowManager.js
@@ +383,3 @@
                && parent) {
             this._checkDimming(parent, window);
+            if (!window.is_attached_dialog() || !this._shouldAnimate())

This check didn't make any sense before, and still doesn't; breaking on !shouldAnimate seems wrong.

Created attachment 186592 [details] [review]
window: only attach dialogs to NORMAL, DIALOG, and MODAL_DIALOG windows

- Always call move_attached_dialog() when moving a window, but call
  meta_window_is_attached_dialog() from inside it
- Use meta_window_is_attached_dialog() in propagate_focus_appearance()
- Add the "parent != window" check to meta_window_is_attached_dialog()

Created attachment 186593 [details] [review]
windowManager: use meta_window_is_attached_dialog()

- remove incorrect shouldAnimate() check.
- (still doesn't update the configure check)

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

Review of attachment 186592 [details] [review]:

One problem I see here (in addition to the not-really-fully-correct-but-OK-for-now caveats noted earlier)

::: src/core/window.c
@@ +6386,1 @@
     return;

I think you also need this check when walking up the chain - consider a transient-for for a window that is transient-for the desktop - the focus appearance would propagate up to the desktop, while it should stop at the immediate parent.

Review of attachment 186593 [details] [review]:

::: js/ui/windowManager.js
@@ +382,2 @@
         while (window.get_window_type() == Meta.WindowType.MODAL_DIALOG
                && parent) {

This check here seems duplicative of the is_attached_dialog check below - I'd expect something like:

 while (parent && window.is_attached_dialog()) {
     this._checkDimming(parent, window);
     ....

But then again, wait, this loop really makes not much sense to me - why would we do the rest of the loop for parents of a window being destroyed? And what the heck is the return doing at the end of the loop? This loop isn't a loop at all - it's just a loop so the break can be a goto? so probably the shouldAnimate was needed?

Created attachment 191999 [details] [review]
window: only attach dialogs to NORMAL, DIALOG, and MODAL_DIALOG windows

====

big change here is that we now freeze the "attached" status when the
dialog maps (and make no attempt to correct it for existing windows
when the preference changes), so nothing needs to worry about the
possibility of visible dialogs changing between attached and
unattached

Created attachment 192000 [details] [review]
windowManager: replace GLSL window fader with a Lightbox

The GLSL-based window fader apparently doesn't work on quite a lot of
otherwise-supported hardware. So replace it with something simpler.

====

I had to do this to be able to test that the code was dimming and
undimming windows correctly. I have no idea if the dimming percentage
is correct, since I have never seen the GLSL-based effect before...

Created attachment 192001 [details] [review]
windowManager: use meta_window_is_attached_dialog()

Use meta_window_is_attached_dialog() so that we only dim/unfold dialog
windows that mutter is actually showing as attached

(In reply to comment #12)
> Created an attachment (id=192000) [details] [review]
> windowManager: replace GLSL window fader with a Lightbox
> 
> The GLSL-based window fader apparently doesn't work on quite a lot of
> otherwise-supported hardware. So replace it with something simpler.

How exactly does it fail? Does it at least compile? Which hardware (and driver) is that?

(In reply to comment #12)
> Created an attachment (id=192000) [details] [review]
> 
> I had to do this to be able to test that the code was dimming and
> undimming windows correctly. I have no idea if the dimming percentage
> is correct, since I have never seen the GLSL-based effect before...

Here is a video of the current (GLSL-based) effect:

http://193.200.113.196/apache2-default/dim.webm 

(it is very subtle, you might just not have noticed it).

(In reply to comment #14)
> (In reply to comment #12)
> > Created an attachment (id=192000) [details] [review] [details] [review]
> > windowManager: replace GLSL window fader with a Lightbox
> > 
> > The GLSL-based window fader apparently doesn't work on quite a lot of
> > otherwise-supported hardware. So replace it with something simpler.
> 
> How exactly does it fail? Does it at least compile? Which hardware (and driver)
> is that?

Probably an older intel card which only supports ARB shaders.

As the existing shader desaturates the window rather than dimming it, I'm not sure the replacement is wanted unconditionally. On the other hand, the difference might not really matter in practice.

(In reply to comment #15)
> (it is very subtle, you might just not have noticed it).

Wow.

"Subtle" is hardly the word for that. I've spent a lot of time fiddling with modal dialogs for this bug, bug 647712, and bug 647613, and I knew that there was supposed to be a dimming effect, and I *still* never noticed it. (And Owen claimed in the original bug that the effect didn't work on any of his machines, but I wonder now if that's really true.)

Created attachment 192124 [details] [review]
window-dimmer: Make the effect more notice able

The window dimming effect seems to be to subtle that some people
don't even notice it, so make it a bit more notice able.

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > Created an attachment (id=192000) [details] [review] [details] [review] [details] [review]
> > > windowManager: replace GLSL window fader with a Lightbox
> > > 
> > > The GLSL-based window fader apparently doesn't work on quite a lot of
> > > otherwise-supported hardware. So replace it with something simpler.
> > 
> > How exactly does it fail? Does it at least compile? Which hardware (and driver)
> > is that?
> 
> Probably an older intel card which only supports ARB shaders.

I can't think of any such card, iirc even 915 does support GLSL and we don't really support anything older then that.

Created attachment 192125 [details] [review]
window-dimmer: Make the effect more notice able

The window dimming effect seems to be too subtle that some people
don't even notice it, so make it a bit more notice able.

---

*) Fixed commit message

(In reply to comment #17)
> (In reply to comment #15)
> > (it is very subtle, you might just not have noticed it).
> (And Owen
> claimed in the original bug that the effect didn't work on any of his machines,
> but I wonder now if that's really true.)

No, after an IRC discussion back then it ended up as "didn't notice it before" rather than "does not work".

(In reply to comment #19)
> > Probably an older intel card which only supports ARB shaders.
> 
> I can't think of any such card, iirc even 915 does support GLSL and we don't
> really support anything older then that.

You remember incorrectly, on my netbook with 915 I get:

  JS LOG: Error invoking Clutter.compile: GLSL shaders not supported

(In reply to comment #21)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > (it is very subtle, you might just not have noticed it).
> > (And Owen
> > claimed in the original bug that the effect didn't work on any of his machines,
> > but I wonder now if that's really true.)
> 
> No, after an IRC discussion back then it ended up as "didn't notice it before"
> rather than "does not work".

Yes, that was the case. But I think not drawing the eye isn't necessarily a bug. (Except when you are trying to understand if it's working correctly or not!)

But what is the point of having an effect at all if even people who are explicitly looking for it can't see it?

(In reply to comment #22)
> (In reply to comment #19)
> > > Probably an older intel card which only supports ARB shaders.
> > 
> > I can't think of any such card, iirc even 915 does support GLSL and we don't
> > really support anything older then that.
> 
> You remember incorrectly, on my netbook with 915 I get:
> 
>   JS LOG: Error invoking Clutter.compile: GLSL shaders not supported

Yeah was wrong, 915 does not support control flow (and vertex shaders in hw) so no GLSL.

(In reply to comment #24)
> But what is the point of having an effect at all if even people who are
> explicitly looking for it can't see it?

Presumably it still directs the users eye to the dialog? (One of the reasons it is subtle is that it depends on time, so it takes a second or two to fully darken. Presumably the fact that there's no sudden blink that takes your eye to the window that we are trying to deemphasize is a good thing.)

I know the designers do want some tweaking - I think they were unhappy with the fact that it blended with middle gray rather than darkening. (Though what is is supposed to do for dark-theme apps if we darken?) So, probably the best thing to do is to get them to review/mockup what they want rather than just intensifying what we have no.

Review of attachment 191999 [details] [review]:

The problem I see with freezing the status at attach time is that there's an assumption that any attached dialog has a parent that it's attached to. But we can't freeze the existence of a parent - the parent might get destroyed, after all. If we want to avoid handling an attached dialog becoming unattached, the only idea I have is to fake the window being withdrawn an unmapped - it seems like you should be able to do this with:

   meta_window_unmanage (window, timestamp);
   new_window = meta_window_new (display, xid, FALSE);

With the obvious caveat that we've changed the identity of the window, and emitted all sorts of "window created signals" - so we need to be pretty careful where we do this - in particular, we'd never want to do it inside any method that could be called from application code.

::: src/core/window.c
@@ +10256,3 @@
+
+  if (window->mapped)
+    return window->attached;

I don't think window->mapped, which is the state of the client window, really has anything to do with whether you want to use the frozen window or not - e.g., if someone shaded a (non-attached) dialog, you wouldn't want to unfreeze it's map state. It seems to me that you want to compute this once towards the end of the management process and leave it that way. (Which makes me think there shouldn't be a single function that either computes the unfrozen value or returns the frozen value, but they should be split apart.)

(In reply to comment #27)
> With the obvious caveat that we've changed the identity of the window, and
> emitted all sorts of "window created signals" - so we need to be pretty careful
> where we do this - in particular, we'd never want to do it inside any method
> that could be called from application code.

Right. I'd suggested on IRC yesterday that we could use the unmap/remap to just fix up the window any time it's should-be-attached state changed, but it's probably safer to not do that, and instead to only do it in the parent-destroyed case.

> It seems to me that you want to compute this once towards the
> end of the management process and leave it that way.

Yes, that's more or less what I meant, I was just confused.

Created attachment 192466 [details] [review]
window: make determination of attached dialog windows more consistent

Different bits of code were using slightly different checks to test
whether a window was an attached dialog. Add a new
meta_window_is_attached_dialog(), and use that everywhere.

Also, freeze the is-attached status when the window first shown, since
nothing else is really set up to deal with windows switching between
attached and detached. (However, if the parent window is destroyed, we
try to fix things up in that case.)

Remove some code in display.c that tried to fix existing windows if
the gconf setting changed, but which didn't actually do anything (at
least under gnome-shell). However, if 654643 was fixed then the new
behavior with this patch would be that changing the gconf setting
would affect new dialogs, but not existing ones.

Created attachment 192467 [details] [review]
window: only attach dialogs to NORMAL, DIALOG, and MODAL_DIALOG windows

Attaching dialogs to unusual windows (like the desktop) looks bad, so
don't do it.

(In reply to comment #29)
> attached and detached. (However, if the parent window is destroyed, we
> try to fix things up in that case.)

untested, btw... anyone got an easy way to check that, or do we need to write a test program specifically for that?

Review of attachment 192466 [details] [review]:

What about the case where the transient-for hint gets changed? That could equally result in a transient window without a parent. I think we need to handle it the same way. I don't think there's any reentrancy-from-application code issues, but some care would definitely be needed with the prop-hooks code for reentrancy.

(If you handle that, then it's going to be hard to write a GTK+ based test case for this case, since I think GTK+ will unset the transient-for relationship first.)

::: src/core/window.c
@@ +1127,3 @@
+  window->attached = should_attach_to_parent (window);
+  if (window->attached)
+    recalc_window_features (window);

Little weird to do this twice (it's done before out of meta_window_update_net_wm_type()), but any workaround would be added complexity for virtually no gain, think it's likely better like this, even if we get a double update of the allowed actions property on the window.

Created attachment 193089 [details]
attached dialog test program

compile with:

cc -o attach attach.c `pkg-config --cflags --libs gtk+-3.0`

Created attachment 193092 [details]
updated attach test, now with choice of two parent windows

Created attachment 193093 [details] [review]
window: make determination of attached dialog windows more consistent

Different bits of code were using slightly different checks to test
whether a window was an attached dialog. Add a new
meta_window_is_attached_dialog(), and use that everywhere.

Also, freeze the is-attached status when the window is first shown,
rather than recomputing it each time the caller asks, since this could
cause problems if a window changes its type after it has already been
attached, etc. However, if an attached window's parent is destroyed,
or an attached window changes its transient-for, then fix things up by
destroying the old MetaWindow and creating a new one (causing
compositor unmap and map events to be fired off, allowing the display
of the window to be fixed up).

Remove some code in display.c that tried to fix existing windows if
the gconf setting changed, but which didn't actually do anything (at
least under gnome-shell). However, if 654643 was fixed then the new
behavior with this patch would be that changing the gconf setting
would affect new dialogs, but not existing ones.

Created attachment 193094 [details] [review]
window: only attach dialogs to NORMAL, DIALOG, and MODAL_DIALOG windows

Attaching dialogs to unusual windows (like the desktop) looks bad, so
don't do it.

Created attachment 193103 [details] [review]
test-attached: new program for testing attached dialogs

====

figured out a place in mutter to stick this...

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

Review of attachment 193093 [details] [review]:

::: src/core/window-props.c
@@ +1538,3 @@
+          window->xtransient_for = old_transient_for;
+          timestamp = meta_display_get_current_time_roundtrip (window->display);
+          meta_window_unmanage (window, timestamp);

I'm unclear about when the window is created again - we seem to basically call meta_window_new() only from MapRequest and MapNotify.

If there's some reliable path, a comment might be useful.

Otherwise, patch looks fine to me.

Review of attachment 193103 [details] [review]:

Looks like it should provide a good test of all the basic possibilities.

::: .gitignore
@@ +61,3 @@
 test-resizing
 test-size-hints
+src/wm-tester/wm-tester

why was adding the path here necessary? It seems strangely inconsistent. (If harmless)

Review of attachment 193094 [details] [review]:

Looks good

(In reply to comment #39)
> Review of attachment 193093 [details] [review]:
> +          meta_window_unmanage (window, timestamp);
> 
> I'm unclear about when the window is created again - we seem to basically call
> meta_window_new() only from MapRequest and MapNotify.

Yeah, meta_window_unmanage() remaps it. It seems to be assuming that windows will only be unmanaged when either (a) they're withdrawn/destroyed, or (b) mutter is exiting, and in the latter case, it remaps the window "so other WMs know that it isn't Withdrawn". I'll add comments in both places.

(In reply to comment #40)
> +src/wm-tester/wm-tester
> 
> why was adding the path here necessary? It seems strangely inconsistent. (If
> harmless)

having just "wm-tester" causes the src/wm-tester directory to be ignored (which then causes git to complain when you try to add a new file to it), when we only wanted the src/wm-tester/wm-tester binary to be ignored.

Created attachment 194834 [details] [review]
windowManager: use meta_window_is_attached_dialog()

Use meta_window_is_attached_dialog() so that we only dim/unfold dialog
windows that mutter is actually showing as attached

====

oops, I forgot to attach the gnome-shell side of the patch along with
the mutter side in the last update. This fixes the weird pseudo-loop
from the original code and earlier versions of the patch

Review of attachment 194834 [details] [review]:

Looks right to me

Attachment 193093 [details] pushed as 7f8c596 - window: make determination of attached dialog windows more consistent
Attachment 193094 [details] pushed as 2be1574 - window: only attach dialogs to NORMAL, DIALOG, and MODAL_DIALOG windows
Attachment 193103 [details] pushed as 368a90c - test-attached: new program for testing attached dialogs

closing... if we want to change the dim effect we can open a new bug
for that

Attachment 194834 [details] pushed as a13af7f - windowManager: use meta_window_is_attached_dialog()

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