Created attachment 212474 [details] [review]
test-actor: Add test to make sure moving depths work properly

That is, that they don't inadvertently show the actor.

Created attachment 212475 [details] [review]
actor: Unconditionally set show_on_set_parent

Otherwise, doing something like adjusting the child's position
on a hidden actor will re-show it, which is not what we want.

the patch makes sense. I'm trying to remember why we chose to do a parent == NULL check, but apparently it was in the original implementation:

http://bugzilla.clutter-project.org/show_bug.cgi?id=791

and given that 4 years have passed I don't actually remember if we did discuss about the patch or not. side effects of working all in the same office, I guess.

I'm planning to remove the :show-on-set-parent property for Clutter 2.0, given that it's just a vestigial remnant of the 0.x series when we wanted to transition from a "hidden by default" to a "visible by default" setting without disturbing existing code too much.

I doubt people are listening in to the :show-on-set-parent property notification, so we can change it to emit notifications even when an actor has a parent.

alternatively, I think we can add a flag to clutter_actor_add_child_internal() that is checked before calling clutter_actor_show(); i.e., the check:

  if (child->priv->show_on_set_parent)
    clutter_actor_show (child);

would become:

  show_on_set_parent = (flags & ADD_CHILD_SHOW_ON_SET_PARENT) != 0;
  if (show_on_set_parent && child->priv->show_on_set_parent)
    clutter_actor_show (child);

with ADD_CHILD_SHOW_ON_SET_PARENT part of the default and legacy flags, while clutter_actor_set_child_above/below_sibling() would not pass the flag above.

I'm mainly outlining this alternative strategy in case we have to back out the change - as far as I'm concerned, attachment 212475 [details] [review] can go in master.

Review of attachment 212475 [details] [review]:

explicitly set the flag

Review of attachment 212474 [details] [review]:

test-actor.c is XIncluded in the API reference, so every change should also be geared towards it being an example. yes, we should split the examples from the interactive test suite, but for now I'd rather not commit this patch.

instead, we should have a conformance test unit to avoid regressions; checking the visible flag and the show-on-set-parent property inside the actor_raise_top/actor_lower_bottom units in actor-graph.c would be enough, I think.

Created attachment 212629 [details] [review]
confiorm: Make sure that raising/lowering children doesn't change state

Namely, visibility and show-on-set-parent.

Review of attachment 212629 [details] [review]:

::: tests/conform/actor-graph.c
@@ +283,3 @@
+  g_assert (!CLUTTER_ACTOR_IS_VISIBLE (iter));
+  g_object_get (iter, "show-on-set-parent", &show_on_set_parent);
+  g_assert (!show_on_set_parent);

mmh, no, this is not correct. this means that changing the stacking order will change the show-on-set-parent property, which is not something we document, nor want.

it means that you add an actor, move it around, remove it from a container and add it to another, and the actor is not shown on add_child().

show-on-set-parent should be set to FALSE only if the actor has been hidden (i.e. visible=false) prior to being added as a child of another actor.

@@ +302,1 @@
 

same as above: show-on-set-parent should be TRUE here.

Review of attachment 212475 [details] [review]:

given the result of the conformance test, I think this needs work.

Created attachment 212667 [details] [review]
conform: Make sure that raising/lowering children doesn't change state

Namely, visibility and show-on-set-parent.



Fix dumb bug. I never tested actor-lower-child, because I was dumb and just
decided to copy/paste half the code. Go me.

Review of attachment 212667 [details] [review]:

looks okay, now

Review of attachment 212475 [details] [review]:

let's try again.

Attachment 212475 [details] pushed as 81b19a7 - actor: Unconditionally set show_on_set_parent
Attachment 212667 [details] pushed as 85323f0 - conform: Make sure that raising/lowering children doesn't change state