GNOME Bugzilla – Bug 674499
app modal dialogs design update
Last modified: 2012-07-17 12:55:58 UTC
Now that we are using a lot of maximized windows with no titlebar it is a bit strange to have app modal dialogs attached to the titlebar. The designs have recently been using modals that are centered on the app window. For example, https://live.gnome.org/Design/SystemSettings/OnlineAccounts#Tentative_Design
1. Is this just for maximized windows? 2. For the titlebar ones we have the slight "roll down" effect ... how should we animate the centered ones? 3. In case the answer to 1. is no ... how to deal with windows that do not fit on the parent (can't be centered) ?
1. No 2. Quick fade in or similar to menu reveal (slight down and fade in) is what occurs to me. Will defer to jimmac here. 3. What do you mean by deal with them? It should work better for those cases I think. We can just consider the two windows to be one entity I think.
If I understand the concern correctly, it's about a modal dialog being larger than the parent window. Not to deny the existence of such occurrence, we should focus on the common case -- newly designed apps. Even in the current incarnation such situation doesn't exactly look appropriate as the attached modal seems to appear coming out of a slot that exceeds the boundary of the parent window, making the relation break here as well. I do agree this doesn't look very good for this case, but neither does out current way. I imagine the transition to be 1) fade out the parent window, followed by the modal scaling in X from the center of the parent window, going from 0 to the width of the final modal, at something like 5-10px height, followed by the Y scale to the final height (creating an old TV like poweron effect). All within 300ms range. I will mock it up. The transition alone creates the relation of the two windows.
(In reply to comment #3) > If I understand the concern correctly, it's about a modal dialog being larger > than the parent window. [...] Even in the current incarnation such situation > doesn't exactly look appropriate That's an understatement - I actually suspect that the new design will be less weird in those cases.
(In reply to comment #4) > (In reply to comment #3) > > If I understand the concern correctly, it's about a modal dialog being larger > > than the parent window. [...] Even in the current incarnation such situation > > doesn't exactly look appropriate > > That's an understatement - I actually suspect that the new design will be less > weird in those cases. Yeah my point was rather "we are going to change it so lets think about this case this time" ... but yeah I agree that it is better anyway.
Are we going to still use attached modal dialogs, only with the attachment in the center?
(In reply to comment #6) > Are we going to still use attached modal dialogs, only with the attachment in > the center? I don't understand this question. But, I think the answer is: yes. Any chance we can get this into 3.6?
(In reply to comment #7) > I don't understand this question. But, I think the answer is: yes. Any chance > we can get this into 3.6? If I have a window that has a modal dialog, and I move the window around, will the modal dialog move too? Do we want to have some sort of way to move the modal dialog out of the way in case it covers necessary information below?
(In reply to comment #8) > (In reply to comment #7) > > I don't understand this question. But, I think the answer is: yes. Any chance > > we can get this into 3.6? > > If I have a window that has a modal dialog, and I move the window around, will > the modal dialog move too? Yes. > Do we want to have some sort of way to move the modal dialog out of the way in > case it covers necessary information below? No. Modals are stateless. Just cancel and do it again if necessary.
OK. I'm working on this one, just to make sure Florian or Rui don't trample me again :). I should have patches in a few minutes (read: a few hours)
Created attachment 217958 [details] [review] constraints: Center modal dialogs on their parent ... rather than attaching them to the parent's title bar. Sorry, was already working on this one ;-)
Created attachment 217960 [details] [review] metacity-3: Update style of attached modal dialogs Modal dialogs are now centered rather than attached to the parent's titlebar. Use a flat titlebar style which fits the new position better. Rough gnome-themes-standard patch, definitively needs some Lapo-love
Created attachment 217961 [details] [review] windowManager: Update animation of attached modals With modal dialogs no longer being attached to their parents' titlebar, the current animation no longer works too well. Use a simple fade animation instead.
Review of attachment 217958 [details] [review]: You need to fix update_resize in window.c so that resizes offset vertically, too.
Review of attachment 217961 [details] [review]: So, first of all, I propose to remove the dimming effect. I have a patch that does it. It's subtle, and clearly no longer correct here, unless we want to dim the entire window (which could get misperceived as not responding). ::: js/ui/windowManager.js @@ -326,1 @@ if (actor.meta_window.is_attached_dialog()) { Can we just remove this entire branch, so we fall back to the stuff below?
Review of attachment 217960 [details] [review]: Sure, why not.
Created attachment 217962 [details] [review] constraints: Center modal dialogs on their parent ... rather than attaching them to the parent's title bar.
Created attachment 217963 [details] [review] shadow-factory: Don't use a top-fade on attached modals
Created attachment 217964 [details] [review] windowManager: Remove the dimming effect With attached modal dialogs no longer attached to the top anchor of the parent, we don't need to dim windows anymore.
Created attachment 217965 [details] [review] windowManager: Remove the special animation for modal dialogs
Review of attachment 217962 [details] [review]: Yes. (Equivalent to my local patch now)
Review of attachment 217963 [details] [review]: Sure.
Created attachment 217967 [details] [review] windowManager: Update animation of attached modals With modal dialogs no longer being attached to their parents' titlebar, the current animation no longer works too well. Use a simple fade animation instead.
We need to stop stepping on each other's toes here. Can we get designed input on the subtle dimming effect? It used to fade out at the top, to coincide with the attaching, but I don't think that works anymore.
(In reply to comment #15) > So, first of all, I propose to remove the dimming effect. Not sure that's better than fixing the effect - as I understand it, the main purpose is to indicate that the parent window can not be interacted with while the modal dialog is up. I don't see this changed with the updated position. Jon?
(In reply to comment #25) > (In reply to comment #15) > > So, first of all, I propose to remove the dimming effect. > > Not sure that's better than fixing the effect - as I understand it, the main > purpose is to indicate that the parent window can not be interacted with while > the modal dialog is up. I don't see this changed with the updated position. Yeah having the parent not responding to input without indicating that to the user (like we did with the dimming) would just be confusing and feel broken.
(In reply to comment #26) > Yeah having the parent not responding to input without indicating that to the > user (like we did with the dimming) would just be confusing and feel broken. I tried doing the fade-out, and it just seemed like the window was really unresponsive, which is the cue that Windows gives it.
I think the fade out / dimming is required. It serves a few purposes: distinguish the parent from the dialog, highlight that information is being requested, visually identify that the information is requested in a modal fashion, indicate that the parent cannot be interacted with, be somewhat consistent with unfocused window appearance, be somewhat consistent with system modal behaviour. So, we should think about how the effect here relates to or differs from unfocused windows and system modal dimming. With the first three of these patches applied I think the effect may be too subtle.
(In reply to comment #28) > With the first three of these > patches applied I think the effect may be too subtle. The first three ACN patches? They don't change the effect at all.
Created attachment 218200 [details] [review] dim-window: Dim the entire window While modal dialogs were attached to the parent's titlebar, it made sense to leave the top of the parent window at full color. With the new position of modal dialogs, it makes more sense to dim the entire parent window.
Created attachment 218201 [details] [review] dim-window: Make the effect a bit less subtle
Something like this?
Review of attachment 218200 [details] [review]: Wouldn't this be better as a ClutterGrayscaleEffect?
Review of attachment 217967 [details] [review]: ::: js/ui/windowManager.js @@ +327,3 @@ this._checkDimming(actor.get_meta_window().get_transient_for()); + + if (!this._shouldAnimate()) { Couldn't you merge this into the else if branch if, by removing the else?
(In reply to comment #34) > Review of attachment 217967 [details] [review]: > > ::: js/ui/windowManager.js > @@ +327,3 @@ > this._checkDimming(actor.get_meta_window().get_transient_for()); > + > + if (!this._shouldAnimate()) { > > Couldn't you merge this into the else if branch if, by removing the else? No - note the subtle difference between this._shouldAnimate() and this._shouldAnimate(actor). It's either that or modifying _shouldAnimate() like in your patch - I went with the conservative approach to avoid unwanted animations in case we start animating other events that affect modals.
Created attachment 218748 [details] [review] windowManager: Replace custom shader with a DesaturateEffect (In reply to comment #33) > Wouldn't this be better as a ClutterGrayscaleEffect? Yes. While modal dialogs were attached to the parent's titlebar, it made sense to leave the top of the parent window at full color. With the new position of modal dialogs, it makes more sense to dim the entire parent window, so we can use Clutter's DesaturateEffect instead of our own custom shader.
Created attachment 218749 [details] [review] magnifier: Fix grayscale effect Commit 8754b2767c1b54e added a grayscale effect to the magnifier, but missed actually adding it to the actor. Unrelated drive-by fix ;-)
Comment on attachment 217964 [details] [review] windowManager: Remove the dimming effect We still want dimming, so getting this off the review list
Review of attachment 218749 [details] [review]: D'oh!
Review of attachment 218748 [details] [review]: ::: js/ui/windowManager.js @@ +38,1 @@ if (!Meta.prefs_get_attach_modal_dialogs()) { It seems strange that this is in set dimFactor. I'm not sure why, TBH. We should just add/not add that effect when that happens, instead of doing extra work at 60fps. @@ -59,3 @@ - if (fraction > 0.01) { - Shell.shader_effect_set_double_uniform(this._effect, 'height', this.actor.get_height()); You can now remove shell_util_shader_effect_set_double_uniform as well @@ +259,3 @@ if (animate) Tweener.addTween(getWindowDimmer(actor), + { dimFactor: DIM_FACTOR, You can just tween the effect's 'factor' property directly. And with that, I don't think we need to keep the WindowDimmer class around directly anymore.
(In reply to comment #35) > No - note the subtle difference between this._shouldAnimate() and > this._shouldAnimate(actor). It's either that or modifying _shouldAnimate() like > in your patch - I went with the conservative approach to avoid unwanted > animations in case we start animating other events that affect modals. I'm just going to say that any comment that starts with "note the subtle difference" is probably too deep. I didn't even notice that. You should probably split that out into two methods, one calling the other.
Also, I'm going to make the comment that yes, I do know that blending with a partial, off gray is not the same as a "grayscale effect", or a "desaturate effect", as the former doesn't do proper weighting. I don't think it matters too much.
(In reply to comment #40) > Review of attachment 218748 [details] [review]: > > ::: js/ui/windowManager.js > @@ +38,1 @@ > if (!Meta.prefs_get_attach_modal_dialogs()) { > > It seems strange that this is in set dimFactor. I'm not sure why, TBH. We > should just add/not add that effect when that happens, instead of doing extra > work at 60fps. That means we need to handle preference changes and add/remove the effect appropriately (or probably better to always add the effect and enable/disable it). Not sure it's really worth the extra code though ...
I don't think it's worth the edge case scenario of disabling attached modal dialogs while one is up (or vice versa). I don't think mutter actually respects the change while a dialog is up, either.
Comment on attachment 218749 [details] [review] magnifier: Fix grayscale effect Attachment 218749 [details] pushed as b7018de - magnifier: Fix grayscale effect
(In reply to comment #42) > Also, I'm going to make the comment that yes, I do know that blending with a > partial, off gray is not the same as a "grayscale effect", or a "desaturate > effect", as the former doesn't do proper weighting. > > I don't think it matters too much. We might actually need to reconsider - I've just used nautilus for testing, and the DesaturateEffect is *extremely* subtle (unless the current folder happens to contain a lot of colorful thumbnails).
OK, then let's keep the current effect for now. (You could also play around with a brightness/contrast effect, but I'm not sure it's worth it)
(In reply to comment #47) > OK, then let's keep the current effect for now. (You could also play around > with a brightness/contrast effect, but I'm not sure it's worth it) Yeah, currently playing around with a combination of desaturation/brightness.
Created attachment 218779 [details] [review] windowManager: Split _shouldAnimate() into two functions ... instead of using an optional parameter.
Created attachment 218780 [details] [review] windowManager: Update animation of attached modals With modal dialogs no longer being attached to their parents' titlebar, the current animation no longer works too well. Use a simple fade animation instead.
Created attachment 218781 [details] [review] windowManager: Remove 'animate' parameter from (un)dimWindow() The parameter has become rather pointless since we always pass the same value.
Created attachment 218782 [details] [review] windowManager: Replace custom shader with builtin ClutterEffects While modal dialogs were attached to the parent's titlebar, it made sense to leave the top of the parent window at full color. With the new position of modal dialogs, it makes more sense to dim the entire parent window, so we can use a combination of Clutter's BrightnessContrast- and DesaturateEffect instead of our own custom shader.
(In reply to comment #41) > I'm just going to say that any comment that starts with "note the subtle > difference" is probably too deep. > > I didn't even notice that. > > You should probably split that out into two methods, one calling the other. This is getting completely unrelated to this patch series *sigh*
Review of attachment 218779 [details] [review]: Looks fine. Minor suggestion, but OK without it. ::: js/ui/windowManager.js @@ +166,3 @@ + }, + + _shouldAnimateActor: function(actor) { I'd be happier if this was shouldAnimateWindow, which took a MetaWindow.
(In reply to comment #44) > I don't think it's worth the edge case scenario of disabling attached modal > dialogs while one is up (or vice versa). There's currently a nasty bug that results in the parent window remaining dimmed after closing the modal dialog when the preference is disabled while the dialog is up. A slightly incorrect fix is to replace the check for window.is_attached_modal() with (window.get_window_type() == Meta.WindowType.MODAL_DIALOG) in _destroyWindow(). > I don't think mutter actually respects the change while a dialog is up, either. Right, filed as bug 679904 ...
(In reply to comment #55) > + _shouldAnimateActor: function(actor) { > > I'd be happier if this was shouldAnimateWindow, which took a MetaWindow. Why? We would always call it as _shouldAnimateWindow(actor.meta_window) ...
I just wasn't a fan of the name "shouldAnimateActor", which makes it sound like it's about any actor, and "shouldAnimateWindowActor" just sounded silly to me.
Review of attachment 218780 [details] [review]: ::: js/ui/windowManager.js @@ +394,2 @@ Tweener.addTween(actor, + { opacity: 0, I can't imagine this working very well -- the window is already gone. It's probably going to be junk or in the window texture before it goes. Same reason we can't fade out normal windows.
Review of attachment 218781 [details] [review]: Nice catch.
Review of attachment 218782 [details] [review]: One thing that I'm concerned about is if this will redirect off-screen twice. But IMO any inefficiency there should be handled by Clutter, as the same thing happens in the Shell with the magnifier.
Attachment 217962 [details] pushed as 0fe0534 - constraints: Center modal dialogs on their parent Attachment 217963 [details] pushed as 1a5132d - shadow-factory: Don't use a top-fade on attached modals
Comment on attachment 217960 [details] [review] metacity-3: Update style of attached modal dialogs Attachment 217960 [details] pushed as 3f67420 - metacity-3: Update style of attached modal dialogs
Attachment 218779 [details] pushed as a04350f - windowManager: Split _shouldAnimate() into two functions Attachment 218780 [details] pushed as 6ab25cd - windowManager: Update animation of attached modals Attachment 218781 [details] pushed as 04570ac - windowManager: Remove 'animate' parameter from (un)dimWindow() Attachment 218782 [details] pushed as c671ff7 - windowManager: Replace custom shader with builtin ClutterEffects Woopsie, the new _shouldAnimate() was actually returning the wrong value (the reviewed patch changed "if (condition) { return false; }" to "return condition;") - fixed before pushing. (In reply to comment #59) > ::: js/ui/windowManager.js > @@ +394,2 @@ > Tweener.addTween(actor, > + { opacity: 0, > > I can't imagine this working very well -- the window is already gone. It's > probably going to be junk or in the window texture before it goes. Yeah, but doing it for opacity is not really worse than doing it for scale_y ...
A little too late comes some guidance on the animation: http://jimmac.fedorapeople.org/gnome3/newmodals.webm The video is 50FPS, so if it's skipping in browser, try playing it back locally in totem.