GNOME Bugzilla – Bug 665758
Window tiling could use animations
Last modified: 2014-02-22 23:37:46 UTC
Currently, when a window is dragged to the top/left/right edge, we just display a blue rectangle behind the window to show where the window would end up. It would be more intuitive if we animated a rectangle to grow from the window's position to the maximized state. - Ubuntu does this, with their rectangle on top of the window (ours is below) ; - Windows 7 does this with a glassy border.
Created attachment 210212 [details] [review] tile-preview: port to Clutter and animate Instead of a GTK+ window, use a ClutterActor to draw the tile preview area. This allows to animate the preview rectangle from the original window position to its final state. Also, the dependence on Xlib is replaced with a tighter dependence on the compositor internals.
Created attachment 210391 [details] [review] tile-preview: port to Clutter and animate Instead of a GTK+ window, use a ClutterActor to draw the tile preview area. This allows to animate the preview rectangle from the original window position to its final state. Also, the dependence on Xlib is replaced with a tighter dependence on the compositor internals. -- let's try with 2.0 API
Review of attachment 210391 [details] [review]: So, currently the split is compositor/core/ui. The idea for the core/ui split stems from Metacity: the idea is that the UI code would be able to be used without being tied to a window manager, so it could be used to draw frames in something like a theme previewer application. See: http://git.gnome.org/browse/mutter/tree/src/core/core.h#n55 (I'm sure there's a more high-level conceptual overview of the split somewhere, but I couldn't find it) The compositor has no interaction with the UI code, only with the core code -- it takes X windows and displays them in a Clutter scene graph, and that's it. So, the abstraction layers: Compositor gets information from Core and displays pixmaps in Clutter using TFP Core gets information from X and sets information to X, as well as creating frame windows UI paints in the frame windows, and also creates its own windows, like in tabpopup.c, tile-preview.c So there's an indirection here -- instead of painting directly inside the scene graph, UI paints on X windows, and compositor pulls from those X windows and displays them. These operations don't have to be in the same process at all (in Compiz, they're not). At one time, this split existed because we wanted to keep mutter as a branch of metacity, rather than a fork. Since the landing of invisible borders/AA corners, this is now impossible, but we chose to keep the split for now. So, there's two issues: * core/ui separation layer is being invalidated with knowledge about MetaWindow from inside UI. I don't think we want to break this split. Use meta_core_get, or pass everything that you need into the constructors. * compositor/ui separation layer is being invalidated with knowledge about the scene graph from inside UI. I don't think we want to break this split either, but it's certainly less of a concern. Either we can just not care about the split and go ahead and invalidate it (not a big fan), or we can move tile-preview.c to the compositor, or to the Shell. ::: src/ui/tile-preview.c @@ +37,3 @@ struct _MetaTilePreview { + ClutterActor *actor; + ClutterContent *canvas; This is just a rectangle with a border. If we wanted rounded rectangles, it may be worth using cairo, but if not, then we should just draw it directly with cogl. See the deprecated clutter-rectangle actor's source for how. (The rounded rectangles would be hard to do in mutter entirely - either we remove the border or we move the actor into the Shell, really) @@ +84,3 @@ + + clutter_actor_get_size (actor, &width, &height); + clutter_canvas_set_size (CLUTTER_CANVAS (preview->canvas), width, height); Use a constraint? @@ +132,3 @@ g_object_unref (context); + g_signal_connect (preview->actor, "paint", Going away in the 2.0 API. Just use a constraint. @@ +199,3 @@ + + window_actor = CLUTTER_ACTOR (meta_window_get_compositor_private (window)); + clutter_actor_lower (preview->actor, window_actor); Deprecated.
(In reply to comment #3) > Review of attachment 210391 [details] [review]: > > So, currently the split is compositor/core/ui. > [...] > * compositor/ui separation layer is being invalidated with knowledge about > the scene graph from inside UI. I don't think we want to break this split > either, but it's certainly less of a concern. Either we can just not care about > the split and go ahead and invalidate it (not a big fan), or we can move > tile-preview.c to the compositor, or to the Shell. > Ok, thanks for taking time to explain this non-trivial layering structure. So, what I'm trying in the next patches is: * Make the tile-preview a compositor plugin effect. I found that there is a lot of boilerplate code for the plugin interface which is maybe now obsolete. I never knew for example that in principle multiple plugins can be registered, even if to some extent this is implicit in the idea of "plugin" :) * Add a very basic effect to default plugin, just to have something simple to test (in principle it can be removed altogether) * Implement the desired effect in the Shell, with rounded corners etc. There are still some issues, I think, I just want to know if the general idea is good enough. > @@ +84,3 @@ > + > + clutter_actor_get_size (actor, &width, &height); > + clutter_canvas_set_size (CLUTTER_CANVAS (preview->canvas), width, height); > > Use a constraint? > Ok, right. > @@ +199,3 @@ > + > + window_actor = CLUTTER_ACTOR (meta_window_get_compositor_private (window)); > + clutter_actor_lower (preview->actor, window_actor); > > Deprecated. Yes, I know, but the new API set_child_below_sibling() doesn't work with window_group, instance of the (deprecated) ClutterGroup. Must wait until mutter is ported to new API
Created attachment 210560 [details] [review] Make tile preview a compositor plugin effect
Created attachment 210561 [details] [review] plugin: implement tile preview effect
Created attachment 210584 [details] [review] plugin: implement tile preview effect -- Split tile preview code into its own files.
Created attachment 211134 [details] [review] plugin: implement tile preview effect -- Since binding the "width" and "height" properties ends up in redrawing the canvas twice per allocation, I'm switching to use the "notify::allocation" signal.
Review of attachment 211134 [details] [review]: a couple of issues I've seen just by looking at the patch ::: src/shell-tile-preview.c @@ +108,3 @@ +get_screen_tile_preview (MetaScreen *screen) +{ + ScreenTilePreview *preview = g_object_get_qdata (G_OBJECT (screen), screen_tile_preview_data_quark); you should probably move this below the g_quark_from_static_string() call - at least, logically. @@ +223,3 @@ + + window_actor = CLUTTER_ACTOR (meta_window_get_compositor_private (window)); + clutter_actor_lower (preview->actor, window_actor); clutter_actor_lower() is deprecated - you should use clutter_actor_set_below_child()
Created attachment 253812 [details] [review] Make tile preview a compositor plugin effect I had meant to pick this up for a while now, finally found some time to do it ... First patch rebased to master.
Created attachment 253813 [details] [review] windowManager: Implement tile previews Mutter now delegates tile previews to compositor plugins, so add a simple implementation based on the UI previously provided by mutter. This is using a slightly different approach than the original patch - with previews in the shell, I don't see any reason not to use JS/CSS them.
Created attachment 253814 [details] [review] windowManager: Animate tile previews With tile previews being implemented as Clutter actors in the shell, we can now easily add fancy animations when showing/hiding the preview. Besides looking more polished, the animations also help understanding what will happen to the window when the drag is finished.
*** Bug 693499 has been marked as a duplicate of this bug. ***
Review of attachment 253814 [details] [review]: ::: js/ui/windowManager.js @@ +1092,3 @@ + width: monitor.width, + height: monitor.height }); + let [, rect] = window.get_outer_rect().intersect(monitorRect); Why this intersection?
Review of attachment 253813 [details] [review]: ::: data/theme/gnome-shell.css @@ +1872,3 @@ +} + +.tile-preview-left.tile-preview-right.on-primary { Do we not have border-radius-top-left / border-radius-top-right? Hm. ::: js/ui/windowManager.js @@ +1060,3 @@ + if (!this._tilePreview) { + this._tilePreview = new St.Bin({ style_class: 'tile-preview', + visible: false }); Any reason this is an St.Bin instead of an St.Widget? @@ +1063,3 @@ + global.window_group.add_actor(this._tilePreview); + } + global.window_group.set_child_below_sibling(this._tilePreview, actor); I'm worried about sync_actor_stacking getting confused and thrusting this to the top/bottom, but OK. @@ +1069,3 @@ + this._tilePreview.add_style_class_name('on-primary'); + else + this._tilePreview.remove_style_class_name('on-primary'); Can we just do style_class = ''; above so we don't have to play with a remove_style_class_name song and dance?
Review of attachment 253812 [details] [review]: ::: src/compositor/meta-plugin-manager.h @@ +39,3 @@ #define META_PLUGIN_SWITCH_WORKSPACE (1<<5) +#define META_PLUGIN_SHOW_TILE_PREVIEW (1<<6) +#define META_PLUGIN_HIDE_TILE_PREVIEW (1<<7) Are these used?
(In reply to comment #14) > + let [, rect] = window.get_outer_rect().intersect(monitorRect); > > Why this intersection? Because having the tile preview "fly" in from outside the monitor looks odd (in particular when the outside parts are actually visible on a second monitor). (In reply to comment #15) > Review of attachment 253813 [details] [review]: > > Do we not have border-radius-top-left / border-radius-top-right? Hm. I didn't see it when I looked for it. Should I have looked harder? > ::: js/ui/windowManager.js > @@ +1060,3 @@ > + if (!this._tilePreview) { > + this._tilePreview = new St.Bin({ style_class: 'tile-preview', > + visible: false }); > > Any reason this is an St.Bin instead of an St.Widget? Not really, no > I'm worried about sync_actor_stacking getting confused and thrusting this to > the top/bottom, but OK. Yeah, there seem to be some issues with that. Will take a look after the app-picker stuff has landed. > @@ +1069,3 @@ > + this._tilePreview.add_style_class_name('on-primary'); > + else > + this._tilePreview.remove_style_class_name('on-primary'); > > Can we just do style_class = ''; above so we don't have to play with a > remove_style_class_name song and dance? The actor is currently only created once. I'm ok with destroying it on hide, but we'd still need to update the style when the preview changes without being hidden (for instance move to the side and wait for the preview, then move along the top edge to have it change to 'maximize') (In reply to comment #16) > Review of attachment 253812 [details] [review]: > +#define META_PLUGIN_SHOW_TILE_PREVIEW (1<<6) > +#define META_PLUGIN_HIDE_TILE_PREVIEW (1<<7) > > Are these used? No, I'll remove them.
Comment on attachment 253812 [details] [review] Make tile preview a compositor plugin effect Attachment 253812 [details] pushed as 21e94ed - Make tile preview a compositor plugin effect
Comment on attachment 253812 [details] [review] Make tile preview a compositor plugin effect Attachment 253812 [details] pushed as 8c69f1b - Make tile preview a compositor plugin effect
Attachment 253813 [details] pushed as 6d93c8b - windowManager: Implement tile previews Attachment 253814 [details] pushed as fcd5f06 - windowManager: Animate tile previews
*** Bug 672425 has been marked as a duplicate of this bug. ***