You mean, a modal dialog attached to a parent window? In that case, the fact that the dialog is hidden is by design.

Yes. It looks weird when e.g. About dialog suddenly disappears, with no animation or such.

Maybe some kind of animation would make it nicer... Let's see what designers think about that.

It isn't really by design I think. Seems reasonable to me to show the apps in the overview the way they actually look.

work on it)

Created attachment 190716 [details] [review]
workspace: show attached modal dialog in overview

(depend on patch from Bug 650254)

Comment on attachment 190716 [details] [review]
workspace: show attached modal dialog in overview

>+        this._shouldShowChilds = Meta.prefs_get_attach_modal_dialogs();

I was going to say "you should use meta_window_is_attached_dialog()", but I guess that's not committed yet. So, you should review bug 646761, and then use meta_window_is_attached_dialog().

also, "children", not "childs"

I noticed recently that Nautilus progress dialog, which is not related to any Nautilus window, is also hidden from the overview. As it is not attached to any Nautilus window, there simply is no way to access it anymore if hidden behind another application window...

(In reply to comment #8)
> I noticed recently that Nautilus progress dialog, which is not related to any
> Nautilus window, is also hidden from the overview. As it is not attached to any
> Nautilus window, there simply is no way to access it anymore if hidden behind
> another application window...

True, very inconvenient. I'm copying files from one location to another, switch to another application, and then I can't switch back to the progress dialog. The only way how to see it is to close all windows.

However, it does not happen for all Nautilus progress dialogs, just sometimes. I'm not sure what the difference is.

(In reply to comment #9)
> (In reply to comment #8)
> > I noticed recently that Nautilus progress dialog, which is not related to any
> > Nautilus window, is also hidden from the overview. As it is not attached to any
> > Nautilus window, there simply is no way to access it anymore if hidden behind
> > another application window...
> 
> True, very inconvenient. I'm copying files from one location to another, switch
> to another application, and then I can't switch back to the progress dialog.
> The only way how to see it is to close all windows.
> 
> However, it does not happen for all Nautilus progress dialogs, just sometimes.
> I'm not sure what the difference is.

It seems like it was an Ubuntu bug, and it is fixed now: https://bugs.launchpad.net/ubuntu/+source/nautilus/+bug/868032

Review of attachment 190716 [details] [review]:

Marking needs-work based on Dan's comment

I hope nobody is offended if take over this bug, as no work has been done since june last year.

Created attachment 222892 [details] [review]
Workspace: show attached modal dialog

Windows in the overview should be like they appear in the workspace,
including modal dialogs that are attached above them.
In addition, hiding the dialogs in the overview causes a flash as
dialog appears at the end of the transition.

Based on a patch by Maxim Ermilov <zaspire@rambler.ru>

Very good initiative! ;-)

Review of attachment 222892 [details] [review]:

Looks good and seems to work fine.

Just have some nitpicks re variable names.

::: js/ui/workspace.js
@@ +141,3 @@
             Lang.bind(this, this._disconnectRealWindowSignals));
 
+	this._childs = [];

childs sounds odd ... "kids" (wouldn't use that though) "children" or "childWindows"

@@ +142,3 @@
 
+	this._childs = [];
+	this.updateChilds();

Same as above.

@@ +189,3 @@
+	    childs.push({ clone: new Clutter.Clone({ source: actor }),
+			  window: actor,
+			  originalPlacing: 1.0,

Placement

Review of attachment 222892 [details] [review]:

::: js/ui/workspace.js
@@ +189,3 @@
+	    childs.push({ clone: new Clutter.Clone({ source: actor }),
+			  window: actor,
+			  originalPlacing: 1.0,

Actually you don't even use that, so just remove it.

What happens to the close button in cases like:
    _______
   |       |
  _|_______|_
 |           |
 |___________|
   |       |
   |_______|

and

    _______
   |       |
  _|       |_
 | |       | |
 |_|       |_|
   |       |
   |_______|

Where the modal dialog is on top in both cases?

(In reply to comment #17)
> What happens to the close button in cases like:
>     _______
>    |       |
>   _|_______|_
>  |           |
>  |___________|
>    |       |
>    |_______|
> 
> and
> 
>     _______
>    |       |
>   _|       |_
>  | |       | |
>  |_|       |_|
>    |       |
>    |_______|
> 
> Where the modal dialog is on top in both cases?

I didn't think about that but from a quick testing with a resized gnome-terminal and the about dialog on top the close button is just a nop ...

Giovanni, are you going to finish this one off?

Uhm, I can integrate the code review comments, but I can't answer the design issues you pointed out in comment #17 offhand.

(In reply to comment #17)
> What happens to the close button in cases like:
>     _______
>    |       |
>   _|_______|_
>  |           |
>  |___________|
>    |       |
>    |_______|
> 
> and
> 
>     _______
>    |       |
>   _|       |_
>  | |       | |
>  |_|       |_|
>    |       |
>    |_______|
> 
> Where the modal dialog is on top in both cases?

Are we able to make it so that the close button closes both the dialog and the parent window? If yes, then the only solution that I can think of would be to place the close button in the upper right of the combined area of both windows. eg:

     _______ |x|
    |       |
   _|       |_
  | |       | |
  |_|       |_|
    |       |
    |_______|

> Are we able to make it so that the close button closes both the dialog and the
> parent window? If yes, then the only solution that I can think of would be to
> place the close button in the upper right of the combined area of both windows.
> eg:
> 
>      _______ |x|
>     |       |
>    _|       |_
>   | |       | |
>   |_|       |_|
>     |       |
>     |_______|

If it's not possible to close both at once, maybe the best solution is simply not to show the close button.

(In reply to comment #21)
> Are we able to make it so that the close button closes both the dialog and the
> parent window?

We can try, of course, but we have the same guarantees as a regular close button: it may not close the window.

(In reply to comment #21)
> Are we able to make it so that the close button closes both the dialog and the
> parent window?

We can try, of course, but we have the same guarantees as a regular close button: it may not close the window.

The other thing is that it might be a bit awkward unless we make that entire box area reactive. If so, what happens when you simply click on the blank space in the combined area? We zoom to the window?

(In reply to comment #24)
> (In reply to comment #21)
> > Are we able to make it so that the close button closes both the dialog and the
> > parent window?
> 
> We can try, of course, but we have the same guarantees as a regular close
> button: it may not close the window.

OK.

> The other thing is that it might be a bit awkward unless we make that entire
> box area reactive. If so, what happens when you simply click on the blank space
> in the combined area? We zoom to the window?

That sounds right. It would help if we fixed bug 665310.

Created attachment 226670 [details] [review]
Workspace: show attached modal dialog

Windows in the overview should be like they appear in the workspace,
including modal dialogs that are attached above them.
In addition, hiding the dialogs in the overview causes a flash as
dialog appears at the end of the transition.

Based on a patch by Maxim Ermilov <zaspire@rambler.ru>

Heavily reworked to handle correctly dialogs that are outside of their
parent. Now the bounding box of the window + children is taken and scaled
as a unit.

Created attachment 226696 [details] [review]
Workspace: show attached modal dialog

Windows in the overview should be like they appear in the workspace,
including modal dialogs that are attached above them.
In addition, hiding the dialogs in the overview causes a flash as
dialog appears at the end of the transition.

Based on a patch by Maxim Ermilov <zaspire@rambler.ru>

Fixed the scaling of non-maximized windows (so that interaction
with the highlight patch is now working) and fixed crash with scroll-to-zoom.

Created attachment 226698 [details] [review]
Workspace: show attached modal dialog

Windows in the overview should be like they appear in the workspace,
including modal dialogs that are attached above them.
In addition, hiding the dialogs in the overview causes a flash as
dialog appears at the end of the transition.

Based on a patch by Maxim Ermilov <zaspire@rambler.ru>

Created attachment 227181 [details] [review]
Workspace: show attached modal dialog

Windows in the overview should be like they appear in the workspace,
including modal dialogs that are attached above them.
In addition, hiding the dialogs in the overview causes a flash as
dialog appears at the end of the transition.

Based on a patch by Maxim Ermilov <zaspire@rambler.ru>

Rebased on the new window layout code.

Review of attachment 227181 [details] [review]:

.oO ( At some point, I think we're going to have to do something about the stex including the padding -- I had my proposal for multiple MetaWindowActors, which I still want to persue, but it just needs completing. Let's not block this on that, it's just some internal notes I have )

::: js/ui/workspace.js
@@ +137,3 @@
+        let clone = container._delegate;
+
+        clone._windowClone.allocate(this._makeBoxForWindow(clone.metaWindow),

Given that you pass over all the children of the container eventually AFAICT, why not just iterate over the children here?

@@ +172,3 @@
+        // As usual, we cannot use a ShellGenericContainer or StWidget here,
+        // because Workspace plays dirty tricks with reparenting to do DNDs
+        // and scroll-to-zoom.

We're going to need to fix this reparenting garbage sooner or later.

@@ +174,3 @@
+        // and scroll-to-zoom.
+        this.actor = new Clutter.Actor({ reactive: true,
+                                         layout_manager: new WindowCloneLayout });

()

@@ +268,3 @@
+                child.clone.destroy();
+
+                this._children = this._children.filter(function(a) {

this._children.splice(this._children.indexOf(child), 1);

(In reply to comment #30)
> Review of attachment 227181 [details] [review]:
> [...]
>
> ::: js/ui/workspace.js
> @@ +137,3 @@
> +        let clone = container._delegate;
> +
> +        clone._windowClone.allocate(this._makeBoxForWindow(clone.metaWindow),
> 
> Given that you pass over all the children of the container eventually AFAICT,
> why not just iterate over the children here?

Because the way to access metaWindow is different (clone is not an actor here, is a WindowClone, and child is a JS object holding the actor and the window)

Created attachment 228319 [details] [review]
Workspace: show attached modal dialog

Windows in the overview should be like they appear in the workspace,
including modal dialogs that are attached above them.
In addition, hiding the dialogs in the overview causes a flash as
dialog appears at the end of the transition.

Based on a patch by Maxim Ermilov <zaspire@rambler.ru>

Just noticed: there is a weird effect when dragging a window with
modal: dragging to the workspace switcher, the window has modals, but
they disappear once you relase. Opposite behavior when dragging from.
My opinion is that we should show modals in the workspace thumbnails
too.

(In reply to comment #32)
> Just noticed: there is a weird effect when dragging a window with
> modal: dragging to the workspace switcher, the window has modals, but
> they disappear once you relase. Opposite behavior when dragging from.
> My opinion is that we should show modals in the workspace thumbnails
> too.

I agree.

(In reply to comment #32)
> Just noticed: there is a weird effect when dragging a window with
> modal: dragging to the workspace switcher, the window has modals, but
> they disappear once you relase. Opposite behavior when dragging from.
> My opinion is that we should show modals in the workspace thumbnails
> too.

Is this considered blocker for merging the patch? Are you still working on this bug, or could I help?

I don't have code to implement the second part right now, so I guess you could build a second patch on top of mine. Also, I don't think it is a blocker, as this is an improvement on its own.

Ok, I started to work on this now. Hope I didn't step on anyone toes...

Created attachment 229776 [details] [review]
Workspace: show attached modal dialog

Windows in the overview should be like they appear in the workspace,
including modal dialogs that are attached above them.
In addition, hiding the dialogs in the overview causes a flash as
dialog appears at the end of the transition.

Based on a patch by Maxim Ermilov <zaspire@rambler.ru>

Heavily refactored, should now handle modal dialogs that appear while
the overview is open.

Created attachment 229777 [details] [review]
Worskpace: miscellaneuous cleanups

Use a single GSettings object for all layout changes of window overlays,
and avoid quadratic behavior for window positioning.

Created attachment 229778 [details] [review]
WorkspaceThumbnails: show attached modal dialogs togheter with their parents

The window clones in the central part of the overview are showing modal
dialogs now, and this creates an inconsistency if the thumbnail doesn't too.
Code is intentionally similar in the two places.

Review of attachment 229777 [details] [review]:

::: js/ui/workspace.js
@@ +1325,3 @@
             let metaWindow = clone.metaWindow;
+            let overlay = clone.overlay;
+            clone.slotId = i;

This is not part of the cleanup AFAICT.

@@ +1730,2 @@
     _addWindowClone : function(win) {
         let clone = new WindowClone(win, this);

WindowClone needs this.overlay = null; in the constructor.

Created attachment 231741 [details] [review]
Workspace: miscellaneuous cleanups

Use a single GSettings object for all layout changes of window overlays,
and avoid quadratic behavior for window positioning.

Review of attachment 231741 [details] [review]:

OK.

Comment on attachment 231741 [details] [review]
Workspace: miscellaneuous cleanups

Attachment 231741 [details] pushed as d479c93 - Workspace: miscellaneuous cleanups

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

One month review ping!
This is an EDM bug.

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

Will we get this in, before UI freeze?

(In reply to comment #47)
> Will we get this in, before UI freeze?

We missed the freeze so we'd need an exception ... can you rebase the patches?

Created attachment 238007 [details] [review]
Workspace: show attached modal dialog

Windows in the overview should be like they appear in the workspace,
including modal dialogs that are attached above them.
In addition, hiding the dialogs in the overview causes a flash as
dialog appears at the end of the transition.

Based on a patch by Maxim Ermilov <zaspire@rambler.ru>

Created attachment 238008 [details] [review]
WorkspaceThumbnails: show attached modal dialogs togheter with their parents

The window clones in the central part of the overview are showing modal
dialogs now, and this creates an inconsistency if the thumbnail doesn't too.
Code is intentionally similar in the two places.

Review of attachment 238007 [details] [review]:

Still does not apply so only reviewed the code which looks good modulo minor c&p issue.

::: js/ui/workspace.js
@@ +94,3 @@
+            let realWindow;
+
+            let realWindow;

One "let realWindow" is enough.

Review of attachment 238008 [details] [review]:

Code looks good, but didn't test it.

(In reply to comment #51)
> Review of attachment 238007 [details] [review]:
> 
> Still does not apply so only reviewed the code which looks good modulo minor
> c&p issue.

That's weird, they're straight on master. Well, grab wip/gcampax/modal-dialogs if you want to test them.

The first patch seems to work fine, but in the workspace switcher I can *only* see the attached modal ...
(Also the code looks like it could be simplified very much if dnd between workspace thumbnails is dropped as requested in 686984 ...)

Weird, it works fine here...

Review of attachment 238008 [details] [review]:

OK now I have tested this ...

Two issues:

1) I can drag the modal dialog away from its parent (when dragging the window from the thumbnails).
2) During DND the clone is not positioned at the cursor actually the thumbnail from the window picker gets moved instead which is very weird ...

2) is not a regression, I can reproduce the same weirdness on master too. And it will be fixed when we stop dragging thumbnails.

On the other hand, the clipping problem (mixing clones and effects) resurfaced, causing the issue Florian mentions in comment #54, so I'm wary of pushing this to 3.8.
We'll prepare the right fix for 3.10.

Not sure this is 3.8.1 material.

(In reply to comment #58)
> Not sure this is 3.8.1 material.

Its not. Changes are too invasive for that.

(In reply to comment #59)
> (In reply to comment #58)
> > Not sure this is 3.8.1 material.
> 
> Its not. Changes are too invasive for that.

I agree. We might even miss 3.10, if we don't fix the ClutterEffect bug, and apparently noone has a clue...

I forgot to add: not using a ClutterClone (MetaWindowActor everywhere) or not using a ClutterOffscreenEffect (and shading the window texture directly) would sidestep the problem.

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

Reassigning. Sorry for the noise.

Pushed, because this bug has been open for too long. No real progress is
possible here, so I pushed also a patch that removes the dimming in the
overview  (as a separate, revertable, commit).
Code has been reviewed, and has been tested by me for several releases now.

Attachment 238007 [details] pushed as 587655f - Workspace: show attached modal dialog
Attachment 238008 [details] pushed as 8b99617 - WorkspaceThumbnails: show attached modal dialogs togheter with their parents