GNOME Bugzilla – Bug 676052
Prerequisite patch stack for CSS window decorations
Last modified: 2012-05-25 22:46:50 UTC
A large number of cleanup patches. This patch stack is based on two patches: * frames: Remove expose_delayed (bug #671104) * frames: Remove frame pixel caching (bug #675111) The big end effect of this patch stack is that the MetaWindowActor is now painting the entirety of the frame mask, rather than passing a cairo path and a set of rectangles to MetaShapedTexture. This probably means that MetaShapedTexture should be renamed, or even that we should maybe merge the two actors, making MetaWindowActor paint directly with Cogl. The documentation for MetaShapedTexture also needs to be changed before landing. The rest of the patches are simple things that remove unused API or turn unused public API into private API. Some of these patches (the remove cairo overlay, punt mask generation, paint shape region, add back aa window corners stack, for example) may want to be squashed before pushing.
Created attachment 214037 [details] [review] screen: Remove more unused private API This one queued a redraw. We're trying to split the queue_redraw API into something a lot better.
Created attachment 214038 [details] [review] window-actor: Remove an unnecessary frame check meta_frame_calc_borders will zero out the borders if we don't have a frame.
Created attachment 214039 [details] [review] window-actor: Use cairo_region_create_rectangles Instead of the bunch of unions.
Created attachment 214040 [details] [review] shaped-texture: Remove the cairo overlay (and rounded corners) As we want GTK+ to paint the mask on an A8, we can't simply use a cairo path. A later commit will make this into a simple masked texture, and meta-window-actor will be in control of the mask.
Created attachment 214041 [details] [review] window-actor: Punt mask generation to MetaWindowActor This effectively makes MetaShapedTexture not a MetaShapedTexture, but a simple and dumb MetaMaskedTexture, with an optimization for clipped regions. XXX: Rename and give it documentation We're doing this as the mask may need to be more complicated than made of a cairo path -- we eventually want GTK+ to draw the entire frame background, which we'll then scan.
Created attachment 214042 [details] [review] window-actor: Paint the shape region with cairo
Created attachment 214043 [details] [review] window-actor: Add back antialiased window corners This simply adds fancy arcs to the mask texture. It's still not painted with GTK+ yet.
Created attachment 214044 [details] [review] window-actor: Add a debugging tool to write a region to a PNG Just a helper function that I keep rewriting all over the place.
Created attachment 214045 [details] [review] window-actor: Work around cairo bug Thank to Company and ickle, a cairo bug was identified and then fixed. They helped me verify I'm not going insane! http://cgit.freedesktop.org/cairo/commit/?id=ec400daf9ec3bbd8403324db7fcdaf175e185e7b
Created attachment 214046 [details] [review] preview-widget: Remove the unused meta_preview_get_clip_region Besides being unused, it used meta_theme_get_frame_style. Since we want to remove the static style layout structs, we need to remove usage of that. Removing unused usage is the way to go.
Created attachment 214047 [details] [review] theme: Make meta_frame_layout_calc_geometry private
Created attachment 214048 [details] [review] theme: Make meta_frame_style_draw_with_style private
(Some of the "unused" / "private" cleanup patches are at the start, and others at the end. This is unintentional, and wasn't supposed to imply dependencies or anything -- all of the cleanup patches in this stack can be landed now)
Review of attachment 214037 [details] [review]: Looks fine (Commit message is a bit obscure, maybe: meta_screen_queue_frame_redraws() will be problematical because we're trying to split the queue_redraw API into something a lot better )
Review of attachment 214038 [details] [review]: Sure
Review of attachment 214039 [details] [review]: Is this in the wrong place in the patch stack / a mismerge - the patch doesn't really make sense against the current code - the old code was modifying region, the new code overwrites region (leaks it, etc.)
Review of attachment 214040 [details] [review]: OK
Review of attachment 214041 [details] [review]: Doesn't have to be in this patch, but I think it's going to be useful to recycle the mask texture when possible. If we're getting alpha off of GTK+ drawing, then when the titlebar changes, we should be able to just update the redrawn areas without creating a new texture. I'd imagine that we should set things up so that we can also reuse the texture if check_reshape() gets called without a change to the window size. ::: src/compositor/meta-shaped-texture.c @@ -125,3 @@ priv->paint_tower = NULL; - meta_shaped_texture_dirty_mask (self); replace with meta_shaped_texture_set_mask_texture (self, COGL_INVALID_HANDLE) ? right now it looks like you are leaking every mask. ::: src/compositor/meta-window-actor.c @@ -2070,3 @@ meta_window_actor_update_shape_region (self, region); - - cairo_region_destroy (region); why was this removed? (might be more of the mis-rebase noted above for the region creation)
Review of attachment 214042 [details] [review]: Yep
Review of attachment 214043 [details] [review]: (marking needs-work though maybe I just don't understand the patch) ::: src/compositor/meta-window-actor.c @@ +2089,3 @@ + if (priv->window->frame != NULL) + install_corners (priv->window, borders, cr); I'm confused about how this works - aren't you just drawing the rounded rectangle on top of the shape? Don't you need to cut out (clear) the corners, and then draw the corners back in?
Review of attachment 214044 [details] [review]: Sure
Review of attachment 214045 [details] [review]: Hmm, basically, I don't like this - it's going to throw off any performance measurements, etc. Let's get it right in jhbuild either by checking out from master/a branch with the fix or by adding a patch to a tarball. If it looks like we might release before a cairo release with the fix, we can reconsider.
Review of attachment 214041 [details] [review]: ::: src/compositor/meta-window-actor.c @@ -2070,3 @@ meta_window_actor_update_shape_region (self, region); - - cairo_region_destroy (region); It's paired with the removal of a cairo_region_referenc in meta_window_actor_update_shape_region. Given that the only caller of the function is us, and it stores something into our private struct, I can move it inline.
Review of attachment 214046 [details] [review]: I don't think 'unused' is an appropriate name for a public function that was added (and used) by gnome-control-center from libmetacity-private. Something like: preview-widget: Remove meta_preview_get_clip_region() The concept of a clip region doesn't make sense now that we have anti-aliased corners and a full alpha channel. Once the theme transition is complete, creating a preview image with an alpha channel will be possible by passing an ARGB surface to gtk_widget_draw(preview_widget, ...); needs-work just for the commit message.
Review of attachment 214047 [details] [review]: you mean static not private in the commit message - it was already private
Review of attachment 214048 [details] [review]: Again you mean static not private in the commit message
Review of attachment 214041 [details] [review]: ::: src/compositor/meta-window-actor.c @@ -2070,3 @@ meta_window_actor_update_shape_region (self, region); - - cairo_region_destroy (region); I'm pretty sure that the reference in the callee and unreference in the caller isn't there just as a left-over, it there to prevent confusion and error-prone (transfer full) argument passing - I might have even asked for it in earlier patch review. I'd rather it was left unless you want to inline things.
Review of attachment 214039 [details] [review]: Yep, this one was wrong. I thought the code was closer than it was when I did this invalid rebase. Pushed back up.
(In reply to comment #22) > Review of attachment 214045 [details] [review]: > > Hmm, basically, I don't like this - it's going to throw off any performance > measurements, etc. Let's get it right in jhbuild either by checking out from > master/a branch with the fix or by adding a patch to a tarball. > > If it looks like we might release before a cairo release with the fix, we can > reconsider. Sure, I can just not push it. Should I apply the patch to cairo in jhbuild, or?
Created attachment 214604 [details] [review] screen: Remove more unused private API These queued redraws, which is a problem when we want to know exactly what changed when we redraw, so we do minimal effort. We're eventually going to replace the queue_redraw API with something a lot better, so let's just get these out of the way now.
Created attachment 214605 [details] [review] preview-widget: Remove meta_preview_get_clip_region The concept of a clip region doesn't make sense now that we have anti-aliased corners and a full alpha channel. Once the theme transition is complete, creating a preview image with an alpha channel will be possible by passing an ARGB surface to gtk_widget_draw(preview_widget, ...);
Created attachment 214606 [details] [review] theme: Make meta_frame_layout_calc_geometry static
Created attachment 214607 [details] [review] theme: Make meta_frame_style_draw_with_style static
Created attachment 214608 [details] [review] window-actor: Remove an unnecessary frame check meta_frame_calc_borders will zero out the borders if we don't have a frame.
Created attachment 214609 [details] [review] shaped-texture: Remove the cairo overlay (and rounded corners) As we want GTK+ to paint the mask on an A8, we can't simply use a cairo path. A later commit will make this into a simple masked texture, and meta-window-actor will be in control of the mask.
Created attachment 214610 [details] [review] window-actor: Punt mask generation to MetaWindowActor This effectively makes MetaShapedTexture not a MetaShapedTexture, but a simple and dumb MetaMaskedTexture, with an optimization for clipped regions. We're doing this as the mask may need to be more complicated than made of a cairo path -- we eventually want GTK+ to draw the entire frame background, which we'll then scan.
Created attachment 214611 [details] [review] window-actor: Paint the shape region with cairo
Created attachment 214612 [details] [review] window-actor: Add back antialiased window corners This simply adds fancy arcs to the mask texture. It's still not painted with GTK+ yet.
Created attachment 214613 [details] [review] window-actor: Add a debugging tool to write a region to a PNG Just a helper function that I keep rewriting all over the place.
Review of attachment 214604 [details] [review]: OK
Review of attachment 214605 [details] [review]: OK
Review of attachment 214606 [details] [review]: OK
Review of attachment 214607 [details] [review]: OK
Review of attachment 214610 [details] [review]: OK
Review of attachment 214612 [details] [review]: I may be misreading the patch stack, but I can't really see how this is going to work - isn't the shape region for a normal window - The window bounds - Minus the client area - With the shape of the client added back Which is a big rectangle the size of the window bounds. Even if we clip to the frame region, the effect of drawing a rounded rectangle *over* that should be a no-op. It seems that you want to do something start off with a region which is *just* the client shape and add what you are drawing here to that? - The shape we get from X includes the frame area - normally it's just going to be a big
Would you rather I move the scanning of the drawn stuff back here? I can do that, though I'm not sure how much it would help with performance.
Created attachment 214635 [details] [review] window-actor: Add back antialiased window corners This simply adds fancy arcs to the mask texture. It's still not painted with GTK+ yet. Here's the simple solution: scanning the frame mask. Discussed on IRC and not sure it's the right approach, but I already had the code from the upcoming GTK+ stack, so it may just be worth it to land it now, rather than build up a frankenstein that will be removed in two weeks. (We need to scan in GTK+ as there's no programmatic way to get access to the border-radius, and Company said he's not going to add a programmatic way to get the border-radius.)
Created attachment 214636 [details] [review] window-actor: Use MetaRegionBuilder when scanning the visible region This gives a pretty solid performance improvement when resizing windows.
Review of attachment 214635 [details] [review]: This isn't working properly.
Created attachment 214678 [details] [review] window-actor: Add back antialiased window corners This simply adds fancy arcs to the mask texture. It's still not painted with GTK+ yet.
Review of attachment 214678 [details] [review]: Looks OK to me, except for one unused variable ::: src/compositor/meta-window-actor.c @@ +1992,3 @@ } +#define TAU (2*M_PI) I assume this relates to http://tauday.com/tau-manifesto ....... @@ +2098,3 @@ + MetaFrameBorders *borders, + cairo_rectangle_int_t *client_area, + cairo_region_t *shape_region) I don't like how shape_region is both an in parameter for the client region and an out parameter for an underestimate of the shape, but since this is temporary code, it's fine. @@ +2187,3 @@ MetaDisplay *display = meta_screen_get_display (screen); MetaFrameBorders borders; + cairo_region_t *region, *frame_bounds_region; frame_bounds_region seems unused?
Review of attachment 214636 [details] [review]: OK
Review of attachment 214678 [details] [review]: ::: src/compositor/meta-window-actor.c @@ +1992,3 @@ } +#define TAU (2*M_PI) Yes. I had more trouble than I'd like to admit with the factor of 2 in this code that I just decided to do everything in terms of Tau. I think it makes the code below much clearer about quarters of a circle. @@ +2098,3 @@ + MetaFrameBorders *borders, + cairo_rectangle_int_t *client_area, + cairo_region_t *shape_region) It's not temporary code. The only thing we'll remove in the future when porting to GTK+ is the MetaFrameBorders parameter. Would you rather I return a cairo_region_t * that I union? Note that it's going to get even more convoluted with the MetaRegionBuilder patch. @@ +2187,3 @@ MetaDisplay *display = meta_screen_get_display (screen); MetaFrameBorders borders; + cairo_region_t *region, *frame_bounds_region; Ah, yep.
Attachment 214604 [details] pushed as 8cb7a45 - screen: Remove more unused private API Attachment 214605 [details] pushed as c2a0719 - preview-widget: Remove meta_preview_get_clip_region Attachment 214606 [details] pushed as 8b64a95 - theme: Make meta_frame_layout_calc_geometry static Attachment 214607 [details] pushed as b98e4e3 - theme: Make meta_frame_style_draw_with_style static Attachment 214608 [details] pushed as 9ca00d5 - window-actor: Remove an unnecessary frame check
Attachment 214609 [details] pushed as 4de492e - shaped-texture: Remove the cairo overlay (and rounded corners) Attachment 214610 [details] pushed as f1aada0 - window-actor: Punt mask generation to MetaWindowActor Attachment 214611 [details] pushed as c47de98 - window-actor: Paint the shape region with cairo Attachment 214613 [details] pushed as 30bc8bc - window-actor: Add a debugging tool to write a region to a PNG Attachment 214678 [details] pushed as 60c05a0 - window-actor: Add back antialiased window corners Pushed everything except the cairo hack. Redraw issues should be fixed. There should be no regressions except for a slight performance regression.