GNOME Bugzilla – Bug 669730
actor: Fix and improve add_child_at_index()
Last modified: 2012-02-13 23:00:07 UTC
See patches - it might make sense to use index_ >= self->priv->n_children in the last patch, though requesting an index of e.g. 4 and ending up with 3 might be confusing.
Created attachment 207162 [details] [review] actor: Fix add_child_at_index() for negative index There is a typo in the check for a negative index: the index variable should be index_, not index - unfortunately, the latter can still be resolved to index(3), so compiler and linker are perfectly happy.
Created attachment 207163 [details] [review] actor: Improve appending actors in add_child_at_index() Currently, add_child_at_index() allows to pass an index of 0 to prepend, a negative index to append, or an existing position to insert. Using an index of "last position + 1" (a.k.a. number of elements) to append seems natural, so allow that as well.
Review of attachment 207162 [details] [review]: how on earth did that even manage to sneak in? damn and blast.
Review of attachment 207163 [details] [review]: I'm not overly convinced about this; it may mask off-by-one errors, and I haven't seen many cases of API actually allow this. plus, it needs: - a check on the input, to ensure that only len, and not len + arbitrary number can be used - a note inside the documentation on the acceptable range and probably set_child_at_index() should be modified as well to accept n_children as a valid value for appending.
Comment on attachment 207162 [details] [review] actor: Fix add_child_at_index() for negative index Attachment 207162 [details] pushed as a023bb3 - actor: Fix add_child_at_index() for negative index (In reply to comment #3) > how on earth did that even manage to sneak in? damn and blast. I guess there's type safety and C type safety (when in doubt, everything is an int) ...
(In reply to comment #4) > Review of attachment 207163 [details] [review]: > > I'm not overly convinced about this; it may mask off-by-one errors, and I > haven't seen many cases of API actually allow this. We do in StBoxLayout :-) (To be fair, our method is implemented as "add_child(); move_child();", so it's a lot more natural than this patch). Also note that so far I only found a single place where we actually rely on that behavior, so it wouldn't be a big deal to treat that case in the shell. So feel free to reject the patch - it's not like I spent hours writing it ... > plus, it needs: > > - a check on the input, to ensure that only len, and not len + arbitrary > number can be used I don't understand that comment - how could "input == len" match len + arbitrary number? > - a note inside the documentation on the acceptable range Fair point. > and probably set_child_at_index() should be modified as well to accept > n_children as a valid value for appending. Dito.
I noticed that ClutterBox uses g_list_insert() directly, which allows appending by simply using an index >= than the list length - so at least there's prior art. if you updated the documentation and set_child_at_index(), I'd accept the patch.
after discussing about it, set_child_at_index() can stay as it is. the patch in attachment 207163 [details] [review] is just missing the documentation update.
Created attachment 207499 [details] [review] actor: Improve appending actors in add_child_at_index() Sorry, I thought I had attached the fpasted version from IRC ...
Sigh. Sorry, we're both working on removing deprecation errors from Clutter. I just pushed http://git.gnome.org/browse/clutter/commit/?id=e3151228958e66fc8a0271c6cffd26234aa9ab69 after IRC review from ebassi. ( For reference, my branch is at http://github.com/magcius/gnome-shell/tree/cb2 )