Dear Jaehwan, My comment are inlined
On 14/09/11 16:27, Jaehwan Kim wrote: > 1. Does it support multi level group inheritance? I.e group B inherits > from group A and group C inherits from group B. I.e A->B->C ? > > -> Yes. Technically, it is possible if the groups are written in order. > But I think if the group multi level inheritance is used, the readability > of the code become bad. I guess that's a matter of taste. I was just thinking of something like: "Button with Icon" inherits from "Button" And "disabled Button with Icon" inherits from "Button with Icon". > > 2. I see you use static variables as for context maintaining, why don't > you pass a context structure? > > -> in edje_cc_handlers.c, static variables is used already. > I.e current_de, current_program. If it is wrong, I'll fix later. Yeah, I noticed that, but I felt like yours are way more context related than those, the ones there looked more global and long-term, maybe I missed something, that's up to the reviewer to decide, I guess. > > 3. Have you tested this properly? What happens when you try to inherit a > non-existing group? > > -> I tested it by using example patch. But I didn't think the case of > non-existing > group. So I fixed about that. If there is any case which I have to consider, > please let me know. No problem. Other possibilities are: missing items with "insert_after/before" and etc. There are probably more cases I can't think about. > > 4. Please add a short explanation about it to the documentation in > src/bin/edje_cc_handlers.c > > -> I already added a short explanation about new feature. I.e inherit, > insert_before, > insert_after. If I have to add more explanation, I'll do it. Sorry, I guess I missed them, the patch is LONG. :) Thanks for taking my comments into account, and even more than that, taking them into account so quickly. Lets wait until someone reviews it from top to bottom (I see from his E-Mail that Cedric started to) and get it in, I'm really looking forward to having this. -- Tom. ------------------------------------------------------------------------------ BlackBerry® DevCon Americas, Oct. 18-20, San Francisco, CA Learn about the latest advances in developing for the BlackBerry® mobile platform with sessions, labs & more. See new tools and technologies. Register for BlackBerry® DevCon today! http://p.sf.net/sfu/rim-devcon-copy1 _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
