For reference, some other valuable guidance here:
http://developer.apple.com/Mac/library/documentation/UserExperience/Conceptual/AppleHIGuidelines/XHIGWindows/XHIGWindows.html#//apple_ref/doc/uid/20000961-DontLinkElementID_1828

Though I'm not sure we'll be using the same.

Created attachment 157585 [details] [review]
PoC: modal dialog like in Mac OS X

mutter patch

Created attachment 157586 [details] [review]
PoC: show animation of modal dialog appear process

Cool.  Great start.  Let's try a few things to differentiate the top dialog fromt the parent:

 * Add a 1px border around the dialog.  The color of which I imagine should at least be informed by either the widget or window theme.

 * Lightbox the dialog by dimming the parent dialog slightly.

 * We probably want window shadows here.

 * Let's try to align the top of the dialog with the titlebar.  Showing a menubar that you can't interact with isn't so nice.

Created attachment 157763 [details] [review]
PoC: modal dialog like in Mac OS X

Created attachment 157764 [details] [review]
PoC: show animation of modal dialog appear/disappear process

Created attachment 157973 [details] [review]
PoC: modal dialog like in Mac OS X

Created attachment 157974 [details] [review]
PoC: show animation of modal dialog appear/disappear process

1. reducing the contrast on parent window (animate contrast changes)
2. don't change the appearance of the titlebar to look unfocused/inactive on parent window

(Some themes does not support window with only border)

Created attachment 158276 [details] [review]
PoC: show animation of modal dialog appear/disappear process

use only GLSL 1.2 (should work on Intel video card)

Hmm, maybe I'm doing something wrong but it doesn't seem to change the contrast on my intel 965.

(In reply to comment #10)
> Hmm, maybe I'm doing something wrong but it doesn't seem to change the contrast
> on my intel 965.

It works fine on my GM45 ... do you see any warnings in stderr?

(In reply to comment #9)
> Created an attachment (id=158276) [details] [review]
> PoC: show animation of modal dialog appear/disappear process

One thing I noticed is that the animation also happens for modal dialogs with no parent (like the policykit dialog), you should check for that and not do any animation here.

Created attachment 158328 [details] [review]
PoC: show animation of modal dialog appear/disappear process

1. Use GLSL 1.0 (will work on all video card, that support shaders)
2. don't animate modal dialogs with no parent

Review of attachment 158328 [details] [review]:

Overall looks good and seem to work fine on both Intel (GM45) and NVIDIA (GTX 285).

Some comments below.

::: js/ui/windowManager.js
@@ +255,2 @@
     _destroyWindowDone : function(shellwm, actor) {
+        if (actor && actor.get_window_type() == Meta.CompWindowType.MODAL_DIALOG) {

You need to check for the parent here too.

::: src/shell-global.c
@@ +288,3 @@
 
+const char shader_src[] =
+"#version 100\n"

Shouldn't hurt but I doubt that this was the problem for Jon (as the 965 should work fine with 1.20).

@@ +292,3 @@
+"uniform float time;\n"
+"uniform float height;\n"
+"#define c -0.2 \n"

According to Eric Anholt this might cause problems when it gets expanded to --c, using a const float is safer here.

@@ +303,3 @@
+"  gl_FragColor = color * contrast + off;\n"
+"  float y = height * gl_TexCoord[0][1];\n"
+"  if (y < 60.0)\n"

Eric also said that using step is more efficient than if on Intel hardware, but it does not seem to be slow here and in the longer term the compilers job anyway.

@@ +323,3 @@
+  guint timeout;
+  gint time;
+} ShaderData;

Should be moved to the header file and add a comment for what it is used for.

@@ +334,3 @@
+  clutter_actor_set_shader_param_float (data->actor, "time", time);
+
+  data->time++;

This seems to be a frame counter, so call it frame rather than time.

@@ +349,3 @@
+}
+
+#define ANIMATION_TIME 2000

This seems a bit too long. Most of the time the window is closed before the animation completes.

@@ +350,3 @@
+
+#define ANIMATION_TIME 2000
+#define FRAMES 60

Both #defines should go to the top of the file or to the header (with comment).

@@ +358,3 @@
+  ClutterActor *actor = CLUTTER_ACTOR (texture);
+  static gboolean cant_compile = FALSE;
+  static GList *datas = NULL;

s/datas/data/ or better shaderList ;)

Review of attachment 158328 [details] [review]:

Overall looks good and seem to work fine on both Intel (GM45) and NVIDIA (GTX 285).

Some comments below.

::: js/ui/windowManager.js
@@ +255,2 @@
     _destroyWindowDone : function(shellwm, actor) {
+        if (actor && actor.get_window_type() == Meta.CompWindowType.MODAL_DIALOG) {

You need to check for the parent here too.

::: src/shell-global.c
@@ +288,3 @@
 
+const char shader_src[] =
+"#version 100\n"

Shouldn't hurt but I doubt that this was the problem for Jon (as the 965 should work fine with 1.20).

@@ +292,3 @@
+"uniform float time;\n"
+"uniform float height;\n"
+"#define c -0.2 \n"

According to Eric Anholt this might cause problems when it gets expanded to --c, using a const float is safer here.

@@ +303,3 @@
+"  gl_FragColor = color * contrast + off;\n"
+"  float y = height * gl_TexCoord[0][1];\n"
+"  if (y < 60.0)\n"

Eric also said that using step is more efficient than if on Intel hardware, but it does not seem to be slow here and in the longer term the compilers job anyway.

@@ +323,3 @@
+  guint timeout;
+  gint time;
+} ShaderData;

Should be moved to the header file and add a comment for what it is used for.

@@ +334,3 @@
+  clutter_actor_set_shader_param_float (data->actor, "time", time);
+
+  data->time++;

This seems to be a frame counter, so call it frame rather than time.

@@ +349,3 @@
+}
+
+#define ANIMATION_TIME 2000

This seems a bit too long. Most of the time the window is closed before the animation completes.

@@ +350,3 @@
+
+#define ANIMATION_TIME 2000
+#define FRAMES 60

Both #defines should go to the top of the file or to the header (with comment).

@@ +358,3 @@
+  ClutterActor *actor = CLUTTER_ACTOR (texture);
+  static gboolean cant_compile = FALSE;
+  static GList *datas = NULL;

s/datas/data/ or better shaderList ;)

Sorry for the double comment, both are the same, blame firefox ;)

Created attachment 158340 [details] [review]
PoC: show animation of modal dialog appear/disappear process

> Should be moved to the header file and add a comment for what it is used for.
It is for internal use.

> Both #defines should go to the top of the file or to the header (with comment).
It use only in this part.

(In reply to comment #16)

> > Both #defines should go to the top of the file or to the header (with comment).
> It use only in this part.

The animation time yes, but FRAMES should be used in the other places where you hardcode 60 too. (Shader and in on_shader_timeout).

Created attachment 159279 [details] [review]
PoC: show animation of modal dialog appear/disappear process

reduce animation time.

Looks good to me.  At some point we may want to tweak the appearance but this is a great start.

Review of attachment 157973 [details] [review]:

I'm pleasantly surprised about how little code this is and how well it works.

Trying it out, one thing that is apparent is that GNOME applications are a bit random about which dialogs are modal and which are not modal. So, it's always a surprise for each dialog whether it's attached or not attached. That may take some thinking about and improvements to the GNOME HIG.

We can't really make Mutter just behave like this unconditionally (unless MeeGo also wants to make this change) - so we need to add

 /apps/metacity/preferences/attach_modal_dialogs

Then we can use meta_prefs_override_preference_location() to make the default for gnome-shell.

::: src/core/core.c
@@ -299,2 +299,2 @@
   /* focus the default window, if needed */
-  if (window->has_focus)
+  if (meta_window_has_focus (window))

I don't think this is right - this really should be about actual focus not appearance of focus.

My feeling is that likely we should have code in here that explicitly handles sending transient dialogs and their parent to the back as a unit. (There seem to be some bugginess in Mutter in this area without your patch as compared to Metacity - I can get both the dialog and the parent showing as focused.)

And it doesn't work for me with your patch if I try to middle click on the parent of an attached modal dialog nothing happens.

::: src/core/window.c
@@ +3762,3 @@
+static gboolean
+notify_parent_moved_resize (MetaWindow *window,
+                            void       *data)

I'd call this function something like "move_attached_dialog" and check that the window is actually an attached dialog rather than moving all dialogs back to the same place whether attached or not.

@@ +3768,3 @@
+  meta_window_get_position (window, &x, &y);
+
+  meta_window_move (window, FALSE, x, y);

If you do that, then you could just use 0, 0 here (with an appopriate comment)

@@ +3845,3 @@
 
+  if (window->type == META_WINDOW_MODAL_DIALOG && parent)
+    {

I think this is possible to do as a constraint in constraint.c and it will be cleaner that way.

@@ +5987,3 @@
+          if (window->type == META_WINDOW_MODAL_DIALOG)
+            {
+              MetaWindow *parent = meta_window_get_transient_for (window);

This should be broken out into a helper function 'queue_redraw_on_ancestors()' say. And then you need a comment as to why you are calling it here.

@@ +8884,3 @@
 gboolean
 meta_window_has_focus (MetaWindow *window)
 {

I think it would be better to add:

 meta_window_appears_focused (window);

Rather than have meta_window_has_focus() mean something different than window->has_focus.

Review of attachment 159279 [details] [review]:

Generally looks promising. The dimming functionality didn't seem to work either place I tested:

 Radeon r500 (free drivers)
 Intel 945

But it didn't do anything bad and the effect looked OK without it.

::: js/ui/windowManager.js
@@ +3,2 @@
 const Clutter = imports.gi.Clutter;
+const Cogl = imports.gi.Cogl;

Left over import

@@ +148,3 @@
     },
 
+    _windowDimmingParentWindow: function(actor, enable, increase) {

You've packed multiple different functions together here using two boolean parameters making both this function and the calling code very difficult to read. :-)

Should be something like:

  _windowDimParent(actor);
  _windowUndimParent(actor);
  _windowCancelDimming(actor);

(I'm not sure if that's the right set of functions, but hopefully you get the idea.)

@@ +157,3 @@
+            return;
+        if (!parentActor.dimming)
+            parentActor.dimming = 0;

If you add a Javascript to a ClutterActor, it should be underscore prefixed to avoid conflicting with current or future GObject properties.

 parentActor._dimmingCount = 0;

::: src/shell-global.c
@@ +288,3 @@
 
+#define FRAMES 10
+#define FRAMES_STR "10.0"

You can avoid duplication here with:

 #define FRAMES 10
 #define FRAMES_STR G_STRINGIFY(FRAMES)

(or G_STRINGIFY(FRAMES) ".0" if the .0 on 10.0 is essential.)

@@ +294,3 @@
+"#version 100\n"
+"uniform sampler2D sampler0;\n"
+"uniform float frame;\n"

You should base your animation based on time rather than on frame # - by basing it on time you can adapt if you are redrawing at 60fps or 10fps. I'd compute the amount in the wrapper and then pass in a 'amount' parameter to the shadow.

@@ +297,3 @@
+"uniform float height;\n"
+"const float c = -0.2;\n"
+"mat4 contrast = mat4 (1.0 + c, 0.0, 0.0, 0.0,"

Looks like this could have been vec4 constrast, since its diagonal? Are you thinking of doing things like desaturation later that would require a non-diagonal matrix?

@@ +301,3 @@
+"                      0.0, 0.0, 1.0 + c, 0.0,"
+"                      0.0, 0.0, 0.0, 1.0);\n"
+"vec4 off = vec4(-c / 0.06 * 0.038, - c / 0.06 * 0.038, -c / 0.06 * 0.038, 0);\n"

You need a comment explaining where the 0.6 and 0.038 are coming from.

@@ +308,3 @@
+"  gl_FragColor = color * contrast + off;\n"
+"  gl_FragColor = color + (gl_FragColor - color) * min(y / "BORDER_MAX_HEIGHT_STR", 1.0);\n"
+"  gl_FragColor = color + (gl_FragColor - color) * min(frame / "FRAMES_STR", 1.0);\n"

This really needs an explanatory comment, maybe something along the lines of:

 // We only fully dim at a distance of BORDER_MAX_HEIGHT from the edge and
 // after FRAMES frames. For other locations and times we linearly interpolate
 // back to the original undimmed color.

Also, it will be clearer that "BORDER_MAX_HEIGHT_STR is a substituted string rather than a quoted string if you write

 " BORDER_MAX_HEIGHT_STRING "

@@ +387,3 @@
+      if (b->actor != actor)
+        continue;
+      a = b;

You should use g_object_set_data()

@@ +401,3 @@
+            a->frame = abs (a->frame);
+          else
+            a->frame = -abs (a->frame);

I don't like this use of the sign to pack in dimming vs. not-dimming. (And not an option if you use time rather than frame number.)

@@ +420,3 @@
+        interval = DISAPPEAR_ANIMATION_TIME / FRAMES;
+
+      a->timeout = g_timeout_add (interval, on_shader_timeout, a);

You should use a clutter timeline rather than a timeout; that will mean that the dimming amount is updated once per drawn frame.

::: src/shell-global.h
@@ +35,3 @@
                                                 GdkPixbuf      *pixbuf);
 
+void shell_clutter_texture_dimming (ClutterTexture *texture, gboolean enable, gboolean increase);

Again, you shouldn't pack together multiple functions that do separate things. Function docs should never look like:

 If enable is true, starts dimming or undimming the texture based on the value of increase. If enable is false, stops dimming or undimming the texture.

If there is common code to share, do that internally inside the implementation of separate functions.

Created attachment 162064 [details] [review]
modal dialog like in Mac OS X

Created attachment 162065 [details] [review]
show animation of modal dialog appear/disappear process

Review of attachment 162065 [details] [review]:

::: src/shell-global.c
@@ +292,3 @@
+#define ANIMATION_TIME 500
+#define ANIMATION_TIME_STR G_STRINGIFY(ANIMATION_TIME)".0"
+#define BORDER_MAX_HEIGHT_STR "60.0"

Hmm...
Not sure we should hardcode those numbers here (as this are generic methods which might get used by anything else besides the modal windows too).

Created attachment 162729 [details] [review]
show animation of modal dialog appear/disappear process

Review of attachment 162064 [details] [review]:

Looking good. Still some comments.

Subject and body need revision. Something like:

 Optionally attach modal dialogs

 Add a preference /apps/mutter/general/attach_modal_dialogs. When true, instead
 of having independent titlebars, modal dialogs appear attached to the titlebar
 of the parent window and are moved together with the parent window.

You can use the second sentence of that for the long description for the schema. The schema file addition is missing here and needed.

::: src/core/constraints.c
@@ +755,3 @@
+      meta_frame_calc_geometry (parent->frame, &fgeom);
+      y += fgeom.top_height;
+      meta_frame_calc_geometry (window->frame, &fgeom);

can't you use use info->fgeom for this?

@@ +759,3 @@
+    }
+  else
+    y = parent->rect.y;

You still need the offset for the dialog's fgeom.top_height in this case.

::: src/core/core.c
@@ +262,3 @@
 
+static gboolean
+user_lower_and_unfocus (MetaWindow *window, 

user_ here means "by the user" - which applies to the whole operation, not to what we are doing to just one window.

So, I'd call this function 'lower_and_unfocus' without the user_. Except that it doesn't unfocus - that's done in the caller. So, I'd call this 'lower_window_and_transients' or something

@@ +270,3 @@
 
+  if (has_focused_child)
+    meta_window_foreach_transient (window, user_lower_and_unfocus, NULL);

This doesn't work right - try opening a modal dialog, switching to a different app, then alt-middle-clicking on the titlebar of the first window - the window is lowered and the attached dialog is detached from it in the stacking order.

My feeling is that this function should always just lower a window and all its transients as a unit.... it's strange to leave non-modal transients around too. That probably should be split off as a separate patch, however, since it's weird to make that chance as part of a patch that adds attached dialogs.

(The other possibility is that we should add a constraint to stack.c that attached modal dialogs always stack immediately above their parent - I can't immediately find anything other ways to detach dialog and parent in the stacking order, but that doesn't mean that there aren't any. However, I think that can wait until we find another such case.)

::: src/core/display.c
@@ +5163,3 @@
+              int x, y;
+              meta_window_get_position (w, &x, &y);
+              meta_window_move (w, FALSE, x, y);

Should be consistent with window.c and use the move to 0,0. That does mean only doing this loop when the pref is turned *ON*, not off. (I don't think there is anything reasonable to do for positioning when the pref is turned off.)

::: src/core/window-private.h
@@ +442,3 @@
                                                     unsigned long  left,
                                                     unsigned long  right);
+gboolean    meta_window_appears_focused    (MetaWindow *window);

A) Add a blank line above, since it doesn't share indentation with the lines above
B) Since it doesn't share common indentation, just do:

 gboolean meta_window_appears_focused (MetaWindow *window);

Without any extra spaces/tabs.

::: src/core/window.c
@@ +3778,3 @@
+
+  if (window->type == META_WINDOW_MODAL_DIALOG && parent && parent != window)
+    /* It ignore x,y for such dialog  */

minor tweak: 'It ignores x,y for such a dialog'

@@ +5832,3 @@
 
+static void
+queue_redraw_on_ancestors (MetaWindow *window, gboolean emit_focus_signal)

This function doesn't just 'queue_redraw_on_ancestors' - it's very specific to the case of focus. So, I'd call it something like:

 check_ancestor_focus_appearance

Parameters need to be on multiple lines

I don't understand why you emit 'focus' when the parent gains the focus, but not when the parent loses the focus. Needs a comment.

@@ +8880,3 @@
+{
+  if (window->has_focus && window->type == META_WINDOW_MODAL_DIALOG)
+    *((gboolean*)data) = TRUE;

should be a space in 'gboolean *'

Review of attachment 162729 [details] [review]:

Seems to work well. From the point of code readability I'm not really entirely happy with the way the time is managed for the animatoin.

if (increase)
    actor_apply_time_based_shader (actor, shader, 1, ANIMATION_TIME, TRUE);
else
    actor_apply_time_based_shader (actor, shader, 2, ANIMATION_TIME, FALSE);

where the 1/2 is passed into the shader as 'x', and after some study turns out to be a factor of what fraction ANIMATION time it should dim/undim. A couple of suggestions about alternate approaches follow.

::: data/gnome-shell.schemas
@@ +226,3 @@
+        <default>true</default>
+        <locale name="C">
+          <short>attach modal dialog to the parent window</short>

capital A

::: js/ui/windowManager.js
@@ +154,3 @@
+            return false;
+        });
+        return count;

Since it is called 'has', you should 'return count != 0'

@@ +197,1 @@
         if (!this._shouldAnimate(actor)) {

The name shouldAnimate doesn't make any sense if we just started animation right above for a different set of windows!

::: src/gnome-shell-plugin.c
@@ +152,3 @@
   meta_prefs_override_preference_location ("/apps/metacity/general/button_layout",
                                            "/desktop/gnome/shell/windows/button_layout");
+  meta_prefs_override_preference_location ("/apps/metacity/general/attach_modal_dialogs",

Alphabetical ordering for keys

::: src/shell-global.c
@@ +291,3 @@
 
+#define ANIMATION_TIME 500
+#define ANIMATION_TIME_STR G_STRINGIFY(ANIMATION_TIME)".0"

If you make the driver code set the parameter to a completion fraction rather than a time, then you can avoid compiling the time into the shader, and I think several things would be more understandable.

The standard name in Clutter for this completion fraction is, strangely, 'alpha' (clutter_alpha_get_alpha().) This is so unreadable in graphics code, that I'd probably use something else for the shader parameter, probably 'fraction'

Then have two constants DIM_TIME, UNDIM_TIME

Store in object data a structure:

{
  ClutterAlpha *alpha;
  gdouble current_value;
  /* whatever other state you need */
}

when 'shell_global_dim_texture()' 'shell_global_undim_texture()' are called, check current_value, destroy the current alpha if any and create a new one, and animate from the current value to the destination value in the appropriate time.

Another more radical approach is to create an object, used from JS as:

 dimmer = new Shell.TextureDimmer(texture)
 [...]
 // When undimming
 Tweener.addTween(dimmer,
                  { fraction: 0,
                    time: UNDIM_TIME,
                    transition: 'linear',
                    onComplete: function() { dimmer.setTexture(null); });

This involves the typical GObject boilerplate, but completely takes the timing and policy out of the hands of the C code and leaves the C code doing what we can't do from JS.

(Though on the other hand, what is it we can't do from JS? I don't obviously see anything in the code here that can't be done in JS. From JS you can take the same approach as above, but instead of writing a GObject, write a JS object, and use a property setter/getter for notification when tweener updates the property. See handling of widthFraction in messageTray.js:SummaryItem)

@@ +308,3 @@
+"                      0.0, 1.0 + c, 0.0, 0.0,"
+"                      0.0, 0.0, 1.0 + c, 0.0,"
+"                      0.0, 0.0, 0.0, 1.0);\n"

You never answered my question about the use of a matrix:

 Looks like this could have been vec4 constrast, since its diagonal? Are you
 thinking of doing things like desaturation later that would require a
 non-diagonal matrix?

@@ +309,3 @@
+"                      0.0, 0.0, 1.0 + c, 0.0,"
+"                      0.0, 0.0, 0.0, 1.0);\n"
+"vec4 off = vec4(-c / 0.06 * 0.038, - c / 0.06 * 0.038, -c / 0.06 * 0.038, 0);\n"

Again to quote:

 You need a comment explaining where the 0.6 and 0.038 are coming from.

@@ +343,3 @@
+                               gint x,
+                               gint animation_time,
+                               gboolean time_increase)

Parameters aren't lined up

@@ +345,3 @@
+                               gboolean time_increase)
+{
+  ClutterTimeline *timeline = g_object_get_data (G_OBJECT (actor), "timeline");

You should use a "namespaced" key for object data, not something generic - that can cause very hard to debug conflicts between different parts of code.

@@ +372,3 @@
+
+void
+shell_clutter_texture_dimming (ClutterTexture *texture, gboolean increase)

Parameters on separate lines. Needs doc comment describing what the function does. But I think you want two functions, as sketched above, even if they share an internal helper functions.

@@ +385,3 @@
+      GError *error = NULL;
+      shader = clutter_shader_new ();
+      clutter_shader_set_fragment_source (shader, shader_src, strlen(shader_src));

You can pass in -1 to mean strlen(shader_src)

@@ +389,3 @@
+      if (cant_compile)
+        {
+          g_warning ("%s", error->message);

Add some context to the message here:

 "Error compiling window dimming shader: %s"

Created attachment 164696 [details] [review]
lower a window and all its transients as a unit

Created attachment 164697 [details] [review]
Optionally attach modal dialogs

Add a preference /apps/mutter/general/attach_modal_dialogs. When true, instead
of having independent titlebars, modal dialogs appear attached to the titlebar
of the parent window and are moved together with the parent window.

Created attachment 164822 [details] [review]
show animation of modal dialog appear/disappear process

> Looks like this could have been vec4 constrast, since its diagonal? Are you
> thinking of doing things like desaturation later that would require a
> non-diagonal matrix?
yes.

> You need a comment explaining where the 0.6 and 0.038 are coming from.
It come from change contrast formula.

Review of attachment 164696 [details] [review]:

Your commit message needs a body explaining the "why" of this change.

Other than that, just trivial stuff.

::: src/core/core.c
@@ -278,3 +277,3 @@
        */
       if (window->screen->active_workspace &&
-          meta_window_located_on_workspace (window, 
+          meta_window_located_on_workspace (window,

Lots of irrelevant whitespace changes make this patch read badly

@@ -298,3 @@
 
-  /* focus the default window, if needed */
-  if (window->has_focus)

This comment got dropped and is still relevant

Review of attachment 164697 [details] [review]:

Code looks god. I still have some questions about the comments.

::: src/core/display.c
@@ +5171,3 @@
+              int x, y;
+              /* Since constrain_modal_dialog change their return value,
+               * need call meta_window_move_resize_internal here.

Why do we need to call this in the case where the preference is turned off? 

Are you just trying to write the code to be generic? To not depend on exactly what the constraints do?

::: src/core/window.c
@@ +5833,3 @@
+static void
+check_ancestor_focus_appearance (MetaWindow *window,
+                                 gboolean    emit_focus_signal)

You seemed to have missed my comment:

 I don't understand why you emit 'focus' when the parent gains the focus, but
 not when the parent loses the focus. Needs a comment.

(In reply to comment #32)
> Why do we need to call this in the case where the preference is turned off? 
When go from 'on' to 'off', window change their size (header appear).
Without this call, header appear, but it height is 0.
So need recalculate size.

> Are you just trying to write the code to be generic? To not depend on exactly
> what the constraints do?
No.

> You seemed to have missed my comment:
Sorry

> I don't understand why you emit 'focus' when the parent gains the focus, but
> not when the parent loses the focus. Needs a comment.

'focus' signal mean window gain focus.
It should not be emit when window loose focus.

Review of attachment 164822 [details] [review]:

Thanks for redoing this to be all in Javascript. It looks like it works out fine that way.

::: data/Makefile.am
@@ +27,3 @@
 	theme/corner-ripple.png			\
 	theme/dialog-error.svg			\
+	theme/dim-window.glsl			\

This shouldn't be in the theme/ subdirectory directory. I think I'd:

 - Put it in data/ in the source tree
 - Install it to $datadir/shaders/dim-window.glsl

::: data/theme/dim-window.glsl
@@ +10,3 @@
+                      0.0, 0.0, 1.0 + c, 0.0,
+                      0.0, 0.0, 0.0, 1.0);
+vec4 off = vec4(-c * 0.633, - c * 0.633, -c * 0.633, 0);

Can you provide an URL to where these formulas are coming from?

@@ +20,3 @@
+
+  gl_FragColor = color + (gl_FragColor - color) * min(y / border_max_height, 1.0);
+  gl_FragColor = color + (gl_FragColor - color) * fraction;

I'd still like to see a comment for this code. Adapting my earlier suggestion:

 // We only fully dim at a distance of BORDER_MAX_HEIGHT from the edge and
 // when the fraction is 1.0. For other locations and fractions we linearly
 /// interpolate back to the original undimmed color.

::: js/ui/windowManager.js
@@ +27,3 @@
+            let file = Gio.file_new_for_path(global.datadir + '/theme/dim-window.glsl');
+            let is = file.read(null);
+            let dis = new Gio.DataInputStream({ 'base-stream': is });

All this complexity seems to be working around the inability to use g_input_stream_read_all() from GJS. Add shell_read_all_from_stream() to shell-global.c to go along with shell_write_string_to_stream().

@@ +42,3 @@
+        } catch (e) {
+            log(e.message);
+            dimShader = null;

The try{} catch should only be on the shader compilation. If reading the sources for the shader fails, that's just a bug that should throw an exception like any other bug in our code.

@@ +74,3 @@
+    if (texture._dimFraction === undefined) {
+        texture.__defineGetter__("dimFraction", Lang.bind(texture, _DimAbleTexture.getDimFraction));
+        texture.__defineSetter__("dimFraction", Lang.bind(texture, _DimAbleTexture.setDimFraction));

Too much magic, too hacky. You can tween any property on any JS object, it doesn't have to be a ClutterActor or GObject.

Write a standalone object class using our standard pattern and do:

function getWindowDimmer(actor) {
 if (actor._windowDimmer == undefined)
    actor._windowDimmer = new WindowDimmer(actor);

 return actor._windowDimmer;
}

Created attachment 165098 [details] [review]
lower a window and all its transients as a unit

> This comment got dropped and is still relevant
It is useless condition in most cases.
This condition check - does window have focus before call to meta_core_user_lower_and_unfocus.
window->has_focus should be replaced with meta_window_appears_focused.
But I think, It is faster and cleaner just always focus default window.

Created attachment 165115 [details] [review]
show animation of modal dialog appear/disappear process

> This shouldn't be in the theme/ subdirectory directory. I think I'd:
> - Put it in data/ in the source tree

I put it to data/shaders in source tree.
When run shell that compiled with jhbuild, it use ~/gnome-shell/source/gnome-shell/data as data dir (It ignore install dir)

> All this complexity seems to be working around the inability to use
> g_input_stream_read_all() from GJS. Add shell_read_all_from_stream() to
> shell-global.c to go along with shell_write_string_to_stream()

Since GDataInputStream implement GBufferedInputStream, this solution will have same speed.
shell_read_all_from_stream will have same complexity. (We don't know size of stream. So need concatenate buffers.)

> Can you provide an URL to where these formulas are coming from?
I got it from my lecture conspectus. I can't find it in Internet:(

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

Review of attachment 165098 [details] [review]:

OK to commit with the following comment added above the 
focus_default_window() line

 /* Rather than try to figure that out whether we just lowered
  * the focus window, assume that's always the case. (Typically,
  * this will be invoked via keyboard action or by a mouse action;
  * in either case the window or a modal child will have been focused.) */

And the following fixes to the subject:

 lower a window => Lower a window
 it's strange to => It's strange to
 lower and unfocus => lowered and unfocused

(In reply to comment #33)
> (In reply to comment #32)
> > Why do we need to call this in the case where the preference is turned off? 
> When go from 'on' to 'off', window change their size (header appear).
> Without this call, header appear, but it height is 0.
> So need recalculate size.

Then your comment should explain that. 

 /* Forcing a call to move_resize() does two things: first, it handles
  * resizing the dialog frame window to the correct size when we remove
  * or add the decorations. Second, it will take care of positioning the
  * dialog as "attached" to the parent when we turn the preference on
  * via the constrain_modal_dialog() constraint.

> > Are you just trying to write the code to be generic? To not depend on exactly
> > what the constraints do?
> No.
> 
> > You seemed to have missed my comment:
> Sorry
> 
> > I don't understand why you emit 'focus' when the parent gains the focus, but
> > not when the parent loses the focus. Needs a comment.
> 
> 'focus' signal mean window gain focus.
> It should not be emit when window loose focus.

If that is the meaning of the focus signal then emitting it on a window that isn't gaining the focus is inappropriate. You need a separate signal to handle the focus appearance changing. What code catching the ::focus signal needs the signal emitted here?

Review of attachment 165115 [details] [review]:

Hmm, it's a real hole in the GIO api that there's no equivalent to g_file_get_contents().

But you can just use g_file_get_contents() 

 let [source, length] = Glib.file.get_contents(global.datadir + '/shaders/dim-window.glsl');

(and then ignore length)

If that works, and the change below works, OK to commit with those changes, and the following commit message:

 Attach dialogs to windows with visual effects

 Override the new mutter preference /apps/mutter/general/attach_modal_dialogs
 to attach modal dialogs to their parent window. Animate the modal dialogs
 expanding from the top of the parent window. Slowly dim the parent window
 after the dialog comes up.

::: data/shaders/dim-window.glsl
@@ +10,3 @@
+                      0.0, 0.0, 1.0 + c, 0.0,
+                      0.0, 0.0, 0.0, 1.0);
+vec4 off = vec4(-c * 0.633, - c * 0.633, -c * 0.633, 0);

If we don't have an explanation for where this math is coming from in this form, let's do the (equivalent) math in a way that makes obvious sense:

 vec4 gray = vec4(0.633, 0.633, 0.633, 1.0);

 [...]

 // To reduce contrast, blend with a mid gray
 gl_FragColor= color * 0.8 + gray * 0.2;

If we want desaturation later, we can find the math for that and implement it at that point.

Created attachment 169932 [details] [review]
Optionally attach modal dialogs

> What code catching the ::focus signal needs the signal emitted here?
only repaint. I fix bug in meta_window_appears_focused. For now, It work witout emit signal.

(In reply to comment #40)
> Review of attachment 165115 [details] [review]:
> 
> Hmm, it's a real hole in the GIO api that there's no equivalent to
> g_file_get_contents().
> 
> But you can just use g_file_get_contents() 
> 
>  let [source, length] = Glib.file.get_contents(global.datadir +
> '/shaders/dim-window.glsl');
> 
> (and then ignore length)

It's actually:

let [success, source, length] = GLib.file_get_contents(global.datadir + '/shaders/dim-window.glsl');

Review of attachment 169932 [details] [review]:

>> What code catching the ::focus signal needs the signal emitted here?
> only repaint. I fix bug in meta_window_appears_focused. For now, It work witout
> emit signal.

OK, great. I think we can go ahead and land all this stuff now :-)

(In reply to comment #40)
> ...
> Hmm, it's a real hole in the GIO api that there's no equivalent to
> g_file_get_contents().

I just recently discovered g_file_load_contents()/g_file_load_contents_async() which is probably what you want?