GNOME Bugzilla – Bug 678917
Fix Clutter deprecations
Last modified: 2014-12-29 03:38:43 UTC
See patches. This ports to the new Clutter API.
Created attachment 217321 [details] [review] Fix up for latest Clutter deprecations
Created attachment 217322 [details] [review] default: Fix up for latest Clutter deprecations
*** Bug 668195 has been marked as a duplicate of this bug. ***
Review of attachment 217322 [details] [review]: ::: src/compositor/plugins/default.c @@ +208,2 @@ { + clutter_actor_remove_child (actor, child); needs a ref() before remove_child(), or child will be disposed. @@ +281,3 @@ + (GDestroyNotify) g_object_unref); + + clutter_actor_remove_child (old_parent, actor); needs a ref() before remove_child(). @@ +388,3 @@ */ static void +on_maximize_effect_complete (ClutterActor *actor, you connect to these signals a lot, but I don't think it's correct - you could end up connecting multiple times. also, ::transitions-complete will be called when all transitions on an actor are complete. I'd detach the handler from within the callback, to avoid multiple invocations. @@ +572,3 @@ + clutter_actor_remove_all_transitions (priv->desktop1); + clutter_actor_remove_all_transitions (priv->desktop2); here would be a good place to also remove the signal handlers for the effect transitions. @@ +579,3 @@ MetaWindowActor *window_actor) { + clutter_actor_remove_all_transitions (CLUTTER_ACTOR (window_actor)); same as above.
Created attachment 217449 [details] [review] default: Fix up for latest Clutter deprecations
Review of attachment 217449 [details] [review]: Not doing a detailed review, as the default plugin is pretty low priority. If Emmanuele looked at an earlier version and it looked OK to him, I'm fine with the change. (master only) One nitpick about a comment. ::: src/compositor/plugins/default.c @@ +602,2 @@ + /* XXX: no good way to disconnect all handlers for a signal, + * so disconnect by data instead. */ This can't be an XXX - it's a deliberate design decision in GTK+ that a blanket disconnect on a signal is non-sensical - you can only disconnect signal handlers that you own, and generally speaking you don't know if you own all signal handlers for a signal. If we connect all our handlers with the same data, then this is fine. If we don't - then the alternative is to store the ID's somewhere.
*** Bug 689181 has been marked as a duplicate of this bug. ***
*** Bug 693128 has been marked as a duplicate of this bug. ***
Review of attachment 217321 [details] [review]: It is about time to get this in ;) Just some minor style fix (see below) otherwise fine. ::: src/compositor/meta-window-actor.c @@ +362,3 @@ meta_window_actor_update_shape (self); + + G_OBJECT_CLASS (meta_window_actor_parent_class)->constructed(object); Missing whitespace before ( .
Created attachment 237739 [details] [review] Fix up for latest Clutter deprecations Rebased to master
Created attachment 237740 [details] [review] default: Fix up for latest Clutter deprecations Rebased to master, but it doesn't actually work -- it crashes when switching workspaces. I don't know why, and I don't have the time to investigate right now. Reattaching in hopes that someone else like drago01 can pick this up.
Review of attachment 237740 [details] [review]: ::: src/compositor/plugins/default.c @@ +201,2 @@ { + ClutterActor *orig_parent = g_object_get_data (G_OBJECT (actor), ORIG_PARENT_KEY); ORIG_PARENT_KEY is set on child not actor. @@ +205,3 @@ + { + g_object_ref (child); + clutter_actor_remove_child (actor, child); You cannot do that. "An iterator is considered valid if it has been initialized, and if the ClutterActor that it refers to hasn't been modified after the initialization." ... You are modifying the actor here ... need to use get_children() and iterate over the list. @@ +206,3 @@ + g_object_ref (child); + clutter_actor_remove_child (actor, child); + clutter_actor_add_child (child, orig_parent); This is backwards ... you want to add the child to orig_parent not the other way round. @@ +207,3 @@ + clutter_actor_remove_child (actor, child); + clutter_actor_add_child (child, orig_parent); + g_object_unref (child); Why? remove_child() already unrefs it once. @@ +289,3 @@ + new_parent = win_workspace == to ? desktop2 : desktop1; + + clutter_actor_remove_child (old_parent, actor); You have to take a reference on actor here.
Review of attachment 237739 [details] [review]: Looks good, commit message is a bit vague though as you don't fix all depreciation. Maybe prepend "compositor:" ?
(In reply to comment #12) > @@ +205,3 @@ > + { > + g_object_ref (child); > + clutter_actor_remove_child (actor, child); > > You cannot do that. > "An iterator is considered valid if it has been initialized, and if the > ClutterActor that it refers to hasn't been modified after the initialization." > ... You are modifying the actor here ... need to use get_children() and iterate > over the list. no need to get a copy of the list of children: you can use clutter_actor_iter_remove(), which is meant for this exact case.
Comment on attachment 237739 [details] [review] Fix up for latest Clutter deprecations Attachment 237739 [details] pushed as d395d75 - Fix up for latest Clutter deprecations Going to push this one for now, and work on the default plugin fixes later.