After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 674510 - Changing a hidden child's position with new API shows the actor
Changing a hidden child's position with new API shows the actor
Status: RESOLVED FIXED
Product: clutter
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: clutter-maint
clutter-maint
Depends on:
Blocks: 668195
 
 
Reported: 2012-04-20 23:55 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2012-04-24 15:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test-actor: Add test to make sure moving depths work properly (3.74 KB, patch)
2012-04-20 23:55 UTC, Jasper St. Pierre (not reading bugmail)
rejected Details | Review
actor: Unconditionally set show_on_set_parent (1.13 KB, patch)
2012-04-20 23:55 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
confiorm: Make sure that raising/lowering children doesn't change state (3.62 KB, patch)
2012-04-23 18:15 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
conform: Make sure that raising/lowering children doesn't change state (4.45 KB, patch)
2012-04-24 08:52 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2012-04-20 23:55:16 UTC
This was the main cause of mutter 2.0 breakage. See patches. Test-case
patch should be self-explanatory.
Comment 1 Jasper St. Pierre (not reading bugmail) 2012-04-20 23:55:18 UTC
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.
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-04-20 23:55:21 UTC
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.
Comment 3 Emmanuele Bassi (:ebassi) 2012-04-23 09:04:14 UTC
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.
Comment 4 Emmanuele Bassi (:ebassi) 2012-04-23 17:05:06 UTC
Review of attachment 212475 [details] [review]:

explicitly set the flag
Comment 5 Emmanuele Bassi (:ebassi) 2012-04-23 17:07:30 UTC
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.
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-04-23 18:15:17 UTC
Created attachment 212629 [details] [review]
confiorm: Make sure that raising/lowering children doesn't change state

Namely, visibility and show-on-set-parent.
Comment 7 Emmanuele Bassi (:ebassi) 2012-04-24 08:30:31 UTC
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.
Comment 8 Emmanuele Bassi (:ebassi) 2012-04-24 08:31:03 UTC
Review of attachment 212475 [details] [review]:

given the result of the conformance test, I think this needs work.
Comment 9 Jasper St. Pierre (not reading bugmail) 2012-04-24 08:52:07 UTC
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.
Comment 10 Emmanuele Bassi (:ebassi) 2012-04-24 09:37:57 UTC
Review of attachment 212667 [details] [review]:

looks okay, now
Comment 11 Emmanuele Bassi (:ebassi) 2012-04-24 09:38:48 UTC
Review of attachment 212475 [details] [review]:

let's try again.
Comment 12 Jasper St. Pierre (not reading bugmail) 2012-04-24 15:26:11 UTC
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