This patch will allow drag n drop objects to groups in the outliner.
(this is part of more work on the outliner, I've split this up in smaller pieces for review now...)
Let me know what you think...
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Globally LGTM, aside from inlined comments.
| source/blender/blenkernel/intern/group.c | ||
|---|---|---|
| 222 | Would rather rename BKE_group_dependency_cycles_check() e.g. Also, I’m not happy at all with this code making assumption about a given state of LIB_DOIT (in other words, the fact that you have to call BKE_main_id_tag_listbase(&bmain->group, true); first outside of it). This should be done inside this func as well. | |
| 250–261 | This can be completely trashed, BKE_group_object_exists() already does exactly the same thing (and use it too in Editors code). ;) | |
| source/blender/editors/space_outliner/outliner_edit.c | ||
| 1844 | Please always let an empty line between vars declarations and actual code. | |
Comment Actions
adressed review by mont29 [except for the use of LIB_DOIT which I am still investigating... - this was existing code and I want to make sure I dont break something - ]
Comment Actions
In general functionality seems useful, since object is a member of a group (rather than settings as with other patches).
Would like to review this one.
Comment Actions
Just initialize LIB_DOIT before doing recursion check. This is because this flag is undefined state and it is up to you to re-set it when you're gonna to use it.
It is allowed to lave the flag dirty tho.
Apart from that seems fine.
Comment Actions
Initialization of LIB_DOIT is done wrong. In your implementation it's expected callee function of BKE_group_dependency_cycles_check() will clear the flags and at the same time you don't clear the flag in 2 cases out of 3.
Ideally it should be BKE_group_dependency_cycles_check() clearing the flags. For that you might need to have separate private function in BKE which would do actual objects traversal.