GNOME Bugzilla – Bug 711645
clutter_actor_set_child_above_sibling() not working in ClutterStage
Last modified: 2021-06-10 11:33:24 UTC
Hello, clutter_actor_set_child_above_sibling() in ClutterStage actor doesn't work in Clutter 1.16 version but works in 1.14 version. #include <clutter/clutter.h> int main (int argc, char **argv) { ClutterActor *stage, *a1, *a2; if (!clutter_init (&argc, &argv)) return -1; stage = clutter_stage_new (); clutter_actor_show (stage); a1 = clutter_actor_new (); clutter_actor_add_child (stage, a1); clutter_actor_set_background_color (a1, CLUTTER_COLOR_Red); clutter_actor_set_size (a1, 50, 50); clutter_actor_set_position (a1, 0, 0); a2 = clutter_actor_new (); clutter_actor_add_child (stage, a2); clutter_actor_set_background_color (a2, CLUTTER_COLOR_Blue); clutter_actor_set_size (a2, 100, 100); clutter_actor_set_position (a2, 0, 0); clutter_actor_set_child_above_sibling (stage, a1, NULL); clutter_main (); return 0; } FYI, clutter_actor_set_child_above_sibling() works properly in non-stage actors.
FYI, it works again when I revert only `clutter-stage.c' in the commit `0b6498d65525661fa4dd7a94929b3c0aee0a129a'. https://git.gnome.org/browse/clutter/commit/?h=clutter-1.16&id=0b6498d65525661fa4dd7a94929b3c0aee0a129a
Created attachment 270168 [details] [review] Patch for clutter-stage painting against clutter-1.18
(Oops, I managed to lose this while adding the patch). I believe this is due to the removal of the clutter-stage paint overload in the above patch. This leads to the paint method from deprecated/clutter-group.c being called instead. That has it's own child list, populated by events and intended to be sorted. However clutter_stage_real_sort_depth_order() is empty. The result of all this is that clutter-stage child actors are painted in creation order, not the depth order. (Raise/lower change the depth order in the stage list, but this is ineffective.) Rather than revert the entire patch, I've replaced the paint overload with a minimal method. This fixes the above case (and a similar problem I had in clutter-1.16).
Review of attachment 270168 [details] [review]: thanks for the patch, and for the investigative work. it's very much appreciated. as a request: could you please use `git format-patch` or (better) `git bz` to attach patches to Bugzilla? it makes it so much easier to apply them with the right commit log and attribution. again, that would be very much appreciated. now, for your patch... in theory, this is a behavioural change: ClutterStage is a Group, which means that it should respect the ClutterActor:depth property when it's set. I did break this behaviour in Clutter 1.10, when I landed the new scene graph API following the First Apocalypse and removed the sort_depth_order() implementation from ClutterStage; and, again, I broke the broken behaviour when I removed the custom implementation of ClutterStage::paint. the correct fix would be to reimplement part of the ClutterGroup behaviour inside ClutterStage directly: implement sort_depth_order(), keep a list of children internally to ClutterStage, and use that one for allocating, painting, and picking. in practice, though, we always said that ClutterActor:depth was never going to reliably reorganise the sorting order, so people should not be using this property to change the allocation and painting of the children of a container, and we never specified the behaviour of ClutterGroup *and* of ClutterStage with regards to the ClutterActor:depth property. these days, ClutterGroup is deprecated, and so is the ClutterActor:depth property. the ClutterActor:z-position property that replaces ClutterActor:depth is specifically documented not to affect the allocation or paint order. your patch, while correct, is only enough to restore the behaviour of ClutterStage up to Clutter 1.10; to restore the original behaviour it would be necessary to remove most of the overrides of the ClutterContainer interface from ClutterStage, and fall back to the ClutterGroup implementation. it would be great to have some unit tests that verify that ClutterStage behaves like ClutterGroup, without breaking the ClutterActor API contract. it's a bit more complicated, so we could use your patch as a stop-gap to get back to a stable behaviour that does not break non-deprecated API, and keep this bug open until we have a patch to get back the old behaviour with deprecated API. again, if you could attach a patch using `git format-patch` or `git bz`, with a proper commit log and attribution, it would be fantastic. thanks again for the patch!
Thanks for the review - it was clear enough that things had changed, but I wasn't sure of the background, and I didn't want to swim against the tide. My initial thought was to promote the group behaviour into the stage, but it wasn't obvious to me that would be a useful plan, so I thought I'd try something simple first. That patch provided a tactical solution for me. I'll try and send it properly formatted (I hadn't committed it before); let me know if there are any other formatting problems.
Created attachment 270298 [details] [review] Patch for clutter stage painting (git format-patch)
Review of attachment 270298 [details] [review]: looks fantastic, thanks.
Comment on attachment 270298 [details] [review] Patch for clutter stage painting (git format-patch) Attachment 270298 [details] pushed to the clutter-1.18 branch. I'll leave the bug open for the rest of the issue.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of clutter, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a ticket at https://gitlab.gnome.org/GNOME/clutter/-/issues/ Thank you for your understanding and your help.