Hi Jaehwan,
2011/9/15 Jaehwan Kim <jae.hwan....@samsung.com>:
> Thanks for your review. ^^
Thanks for your work :-)
>> In fact, you change to much thing that shouldn't in your patch. As an
>> example you are modifying
>> st_collections_group_parts_part_repeat_events when it's not related at
>> all to the current patch. Please only change the function that should.
>
> Indeed, It is necessary in order to change the part I want.
> In st_cellection_group_inherit, it copy all property of the group. And if I
> wirte the specific part or description in child group, its property is
> overwritten.
> To overwrite, the pointer of current part or current description is necessary
> and
> the properties is written in the current point. So I needed chage the code
> like below.
>
> static void
> st_collections_group_parts_part_repeat_events(void)
> {
> Edje_Part_Collection *pc;
> Edje_Part *ep;
>
> check_arg_count(1);
>
> pc = eina_list_data_get(eina_list_last(edje_collections));
> ep = current_part;
>
> ep->repeat_events = parse_bool(0);
> }
>
> But I think the below code is more simple.
>
> static void
> st_collections_group_parts_part_repeat_events(void)
> {
> check_arg_count(1);
>
> current_part->repeat_events = parse_bool(0);
> }
>
> All function have to change like this in order to change the property.
Hum, I will have to read your patch more carefully then, but if you
can, could you then split it in two. One for the previous change and
another for the inherit infrastructure.
>> 7. Do not increase the size of runtime structure, especially Edje_Part
>> and also Edje_program as much as possible. The reorder is maybe doable
>> when you find the keyword, that would avoid completly the reorder
>> structure inside Edje_Part.
>
> If I increase the size of runtime structure, previously built edj files may
> have
> a problem. You concerned about it, right? I didn't think about that. then how
> can I change the code? Do you have any idea? I saw the code you change
> in 62086. Should I change like that?
The issue is that with your change, we now have pointer that are not
used at runtime, but are allocated. This is a waste of memory in my
opinion. It will not break previous edje file, thanks to eet, but
still that's not a good thing in my opinion. Maybe instead of adding
this structure, you could try to put the part/program at the right
place when you find the keyword that tell you where to put it.
As for can_override, I think it should be part of a more generic
infrastructure to detect when setting a value override an existing
property. So lets forget about that one for the moment. You don't need
to change the way you handle it in your patch. When we improve edje
error, we could improve that at that time.
>> 8. Right now, your implementation is a bit like virtual in C++, the
>> group that inherit can change any part the way it want. Maybe, their
>> is two other cases that should be considered :
>> - program and part could be locked and impossible to change at all,
>> throwing a warning if the group try to change the content of this
>> items.
>> - pure virtual part/program, that would prevent the compilation of
>> a group if they are not implemented (like virtual = 0 in C++).
>
> I think there is not locked or virtual part in current edc. Is there anything
> I don't know?
> Or you just mean the C++ concept? I don't know C++ in detail. If I have to
> change,
> please give a explanations to me.
Yes, they don't exist yet. But it would have been a good addition or a
futur direction for this work. It really help to provide clean code.
There is no need to know C++ in detail. The idea is to provide part
that can't be changed and part that absolutly need to be implemented.
It's not as easy as it sounds and it's clearly a possible addition to
your work. So no need to provide to provide it yet. It was just a
question of what could be done in the futur once we have inheritance.
>>> Future work
>>>
>>> 1. Delete - The part, description, program can be deleted.
>
>> I don't know if this is really usefull as all the object model I know
>> don't provide this kinde of feature. Do you have a use case for that ?
>> Or maybe you know a language that provide this kind of feature, so I
>> could read about it.
>
> In our theme, genlist has a lot of style. Specific part can belong to some
> group or not. Let's assume I make new genlist style. I want to inherit a group
> and change some property and delete one part. If deletion is not supported,
> I cannot inherit a group. Let's think about this. ^^
Hum, in my opinion it may become a little bit hugly if we do follow
such path. In the exemple your are describing, I would have prefered
that a common genlist group was created and that both style inherit
from it. It would keep thing clearer, and if someone modify the first
style without looking at the second style, it will not break. If he
rename a part, add some that does have some unintended effect on the
second style.
In general, a clean hierarchie will prevent some breakage and
delete clearly provide a way to do things that will be difficult to
maintain. So I would prefer that's this feature doesn't go in.
>>> 2. Inherit from other edc file (is it possible?)
>
>> Hum, I would say that we don't want that. You don't know what an edc
>> could be. It could be just a macro, or many group, or just one part. I
>> don't think it would be doable.
>
> I think this feature would be doable but could make some problem like
> dependency problem. I'll think more about this feature.
>
>> Thanks for your patch and continue this good and usefull work,
>
> Thanks for your advice.
> I'll do it continuously. Please give a hint to me~^^
Good, have fun,
--
Cedric BAIL
------------------------------------------------------------------------------
Doing More with Less: The Next Generation Virtual Desktop
What are the key obstacles that have prevented many mid-market businesses
from deploying virtual desktops? How do next-generation virtual desktops
provide companies an easier-to-deploy, easier-to-manage and more affordable
virtual desktop model.http://www.accelacomm.com/jaw/sfnl/114/51426474/
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
