From: Jaehwan K. <jae...@sa...> - 2011-09-15 07:05:39
|
Dear Cedric Thanks for your review. ^^ > 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. > 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? > 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. >> 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. ^^ >> 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~^^ Jaehwan Kim. |
From: Jaehwan K. <jae...@sa...> - 2011-09-16 02:09:36
|
Dear Cedric Hello^^ >> 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. The working to split it in two takes long time because I have to change all function again. The change is simple and easy to understand. If you cannot understand my code, I'll give explanation to you. >> 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. ok, I understand your opinion. I'll try to change the reorder structure. >> 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. Thanks for explanation. I'll consider it later. >>> 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. Then I'll let it be. And if there is nessarity later, let think about it at that time. I'll change the part of reorder structure and upload again. Thank you. Jaehwan Kim. |
From: Jaehwan K. <jae...@sa...> - 2011-09-17 08:55:21
|
> Reading this ringed a BIG warning in my mind. You need to be careful > with the way Edje is done. I did not review the code, but let's > remember that: > - All references are done by integer identifier. Order will affect > that! When you provide insert_before and insert_after, do you > recalculate all your ids and reassign them? I recall it was a painful > work to do in Edje_Edit The part id is assigned in the function "compile" and then the value of the part id is assigned to the variable which refer to it in the function "data_process_lookups". (compile -> data_process_lookups) I insert the function "reorder" between "compile" and "data_process_lookups". (compile -> reorder -> data_process_lookups) So the part id is refered after reordering. Because the part id is changed according the order of the part, we don't need to concern about that. > - remember about PART:"name" and PROGRAM:"name"... what to do when > part is deleted? Do you remember to create a new program and recompile > it with the new references on insert_before/after? > - what to do with dependencies? One part is relative to another... > it is deleted... then? If a part or program is deleted and any other part or program has the reference for it, edje_cc have to print ERROR and quit the compile. Currently edje_cc print ERROR and quit for non-exist reference. I think deleted part or program is same with that. It's error. > Please consider all these issues, create tests and prove it is right > before merging it. I smell lots of possible problems with this thing > :-/ I tested the reorder about many case. and it works well. If there is any error case, please let me know. Thanks. Jaehwan Kim. |
From: Jaehwan K. <jae...@sa...> - 2011-09-22 09:43:16
Attachments:
edje_group_inherit.patch
|
Dear Cedric I changed the part of the reorder structure as your advice. >> 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. I separated the structure of reorder from Edje_Part. And after reordering, the structure is deleted. Becuase Edje_Part just has the reorder pointer, I think there isn't the waste of the memory. Please check attached patch file. Thanks. Jaehwan Kim. |
From: Cedric B. <ced...@fr...> - 2011-09-22 12:27:51
|
Dear Jaehwan, 2011/9/22 Jaehwan Kim <jae...@sa...>: > I changed the part of the reorder structure as your advice. >>> 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. > > I separated the structure of reorder from Edje_Part. And after reordering, > the structure is deleted. Becuase Edje_Part just has the reorder pointer, > I think there isn't the waste of the memory. > Please check attached patch file. This change sounds good to me, just one question why didn't you put the "can_override" member inside the reorder member ? And could you please split your patch in two part, one with just the change to use "current_part" and another with the inherit change. Thanks, -- Cedric BAIL |
From: Jaehwan K. <jae...@sa...> - 2011-09-24 07:33:20
|
Dear Cedric >>> 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. > > I separated the structure of reorder from Edje_Part. And after reordering, > the structure is deleted. Becuase Edje_Part just has the reorder pointer, > I think there isn't the waste of the memory. > Please check attached patch file. > This change sounds good to me, just one question why didn't you put > the "can_override" member inside the reorder member ? "can_override" is not related to reorder feature. So I think it is not proper. And "can_override" is in three structure but reorder is just in _Edje_Part structure. I think it's not proper in unity aspect, too. > And could you please split your patch in two part, one with just the > change to use "current_part" and another with the inherit change. I will upload again ,after split the patch code. Thank you for your review. Jaehwan Kim. |
From: Cedric B. <ced...@fr...> - 2011-09-25 23:24:36
|
Hi Jaehwan, 2011/9/24 Jaehwan Kim <jae...@sa...>: >>>> 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. >> >> I separated the structure of reorder from Edje_Part. And after reordering, >> the structure is deleted. Becuase Edje_Part just has the reorder pointer, >> I think there isn't the waste of the memory. >> Please check attached patch file. > >> This change sounds good to me, just one question why didn't you put >> the "can_override" member inside the reorder member ? > > "can_override" is not related to reorder feature. So I think it is not proper. > And "can_override" is in three structure but reorder is just in _Edje_Part > structure. I think it's not proper in unity aspect, too. Yes, but can_override is a data only used by edje_cc. So puting it in Edje_Part make it increase the size of Edje at runtime. In fact, in my opinion, it would make sense to create an Edje_Part_Parser or something like that, that would hold this can_override and the reorder field to. >> And could you please split your patch in two part, one with just the >> change to use "current_part" and another with the inherit change. > > I will upload again ,after split the patch code. I will look at them. Thanks, -- Cedric BAIL |
From: Jaehwan K. <jae...@sa...> - 2011-09-25 07:53:54
|
Dear Cedric I split the group inherit patch file into two patch files as you said. First one is edje_group_inherit_first.patch. Second one is edje_group_inherit_final.patch. I wanted second one to be the diff between first one and final version. But I can not make the diff because "svn diff -x -up --new edje_first --old edje_final" didn't work. So I made a second one final-version-patch which includes first patch content. If you know how to make a diff between directorys by using "svn diff", please let me know. Then I'll upload again. If it is enough these patch files, please check my work. ^^ >>> 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. > > I separated the structure of reorder from Edje_Part. And after reordering, > the structure is deleted. Becuase Edje_Part just has the reorder pointer, > I think there isn't the waste of the memory. > Please check attached patch file. > This change sounds good to me, just one question why didn't you put > the "can_override" member inside the reorder member ? > "can_override" is not related to reorder feature. So I think it is not proper. > And "can_override" is in three structure but reorder is just in _Edje_Part > structure. I think it's not proper in unity aspect, too. > And could you please split your patch in two part, one with just the > change to use "current_part" and another with the inherit change. > I will upload again ,after split the patch code. Thanks. Jaehwan Kim. |
From: Jaehwan K. <jae...@sa...> - 2011-09-28 04:30:33
Attachments:
edje_group_inherit.patch
|
Dear Cedric >>>> 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. >> >> I separated the structure of reorder from Edje_Part. And after reordering, >> the structure is deleted. Becuase Edje_Part just has the reorder pointer, >> I think there isn't the waste of the memory. >> Please check attached patch file. > >> This change sounds good to me, just one question why didn't you put >> the "can_override" member inside the reorder member ? > > "can_override" is not related to reorder feature. So I think it is not proper. > And "can_override" is in three structure but reorder is just in _Edje_Part > structure. I think it's not proper in unity aspect, too. > Yes, but can_override is a data only used by edje_cc. So puting it in > Edje_Part make it increase the size of Edje at runtime. In fact, in my > opinion, it would make sense to create an Edje_Part_Parser or > something like that, that would hold this can_override and the reorder > field to. I changed the structure of three struct (Edje_Program, Edje_Pack_Element, Edje_Part) as you said. I made three struct (Edje_Program_Parser, Edje_Pack_Element_Parser, Edje_Part_Parser). can_override and reorder are used in that struct. So Edje_Program, Edje_Pack_Element and Edje_Part are not changed. We don't worry about the increasing the size of Edje at runtime. I tested it and it works well. Please check my patch. Thanks. Jaehwan Kim. |
From: Cedric B. <ced...@fr...> - 2011-09-29 01:31:07
|
Here we go ! 2011/9/28 Jaehwan Kim <jae...@sa...>: >>>>> 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. >>> >>> I separated the structure of reorder from Edje_Part. And after reordering, >>> the structure is deleted. Becuase Edje_Part just has the reorder pointer, >>> I think there isn't the waste of the memory. >>> Please check attached patch file. >> >>> This change sounds good to me, just one question why didn't you put >>> the "can_override" member inside the reorder member ? >> >> "can_override" is not related to reorder feature. So I think it is not proper. >> And "can_override" is in three structure but reorder is just in _Edje_Part >> structure. I think it's not proper in unity aspect, too. > >> Yes, but can_override is a data only used by edje_cc. So puting it in >> Edje_Part make it increase the size of Edje at runtime. In fact, in my >> opinion, it would make sense to create an Edje_Part_Parser or >> something like that, that would hold this can_override and the reorder >> field to. > > I changed the structure of three struct (Edje_Program, Edje_Pack_Element, > Edje_Part) as you said. I made three struct (Edje_Program_Parser, > Edje_Pack_Element_Parser, Edje_Part_Parser). can_override and reorder > are used in that struct. So Edje_Program, Edje_Pack_Element and Edje_Part > are not changed. We don't worry about the increasing the size of Edje at runtime. > I tested it and it works well. Please check my patch. I did just move the various _Parser structure inside edje_cc.h instead of edje_private.h and fixed some obvious source of bug. As the rest looked fine, I pushed it in svn, so every one can now try and use it. Thanks for your work. Have fun ! -- Cedric BAIL |
From: Tom H. <to...@st...> - 2011-09-29 09:05:40
|
On 29/09/11 04:31, Cedric BAIL wrote: > I did just move the various _Parser structure inside edje_cc.h instead > of edje_private.h and fixed some obvious source of bug. As the rest > looked fine, I pushed it in svn, so every one can now try and use it. > Thanks for your work. > > Have fun ! Thanks a lot to the both of you, this was a long awaited feature. -- Tom. |
From: Cedric B. <ced...@fr...> - 2011-09-29 09:09:03
|
On Thu, Sep 29, 2011 at 11:05 AM, Tom Hacohen <to...@st...> wrote: > On 29/09/11 04:31, Cedric BAIL wrote: >> I did just move the various _Parser structure inside edje_cc.h instead >> of edje_private.h and fixed some obvious source of bug. As the rest >> looked fine, I pushed it in svn, so every one can now try and use it. >> Thanks for your work. >> >> Have fun ! > > Thanks a lot to the both of you, this was a long awaited feature. I am still surprised you didn't commit something that was using it so far ! :-) -- Cedric BAIL |
From: Jaehwan K. <jae...@sa...> - 2011-09-29 09:24:43
|
Dear Cedric and Tom Thank you very much~ ^^ I'm pleased it is pushed in svn. Your advice is so helpful. -- Jaehwan Kim. ------- Original Message ------- Sender : Tom Hacohen<to...@st...> Date : 2011-09-29 18:05 (GMT+09:00) Title : Re: [E-devel] [Patch] edje_cc : group_inherit feature On 29/09/11 04:31, Cedric BAIL wrote: > I did just move the various _Parser structure inside edje_cc.h instead > of edje_private.h and fixed some obvious source of bug. As the rest > looked fine, I pushed it in svn, so every one can now try and use it. > Thanks for your work. > > Have fun ! Thanks a lot to the both of you, this was a long awaited feature. -- Tom. |
From: <han...@gm...> - 2011-09-30 03:04:24
|
Hi, On Thu, Sep 29, 2011 at 11:24 AM, Jaehwan Kim <jae...@sa...> wrote: > > Dear Cedric and Tom > > Thank you very much~ ^^ I'm pleased it is pushed in svn. > Your advice is so helpful. > > -- > Jaehwan Kim. > Thank You very much for implementing this long awaited feature! one thing I came across when using inheritance is that there is no way to unset rel.to from an inherited part. sure one can add a part that is relative to 'base' and make the inherited part relative to it. just wanted to let you know in case you know a better solution for this. Regards, Hannes > ------- Original Message ------- > Sender : Tom Hacohen<to...@st...> > Date : 2011-09-29 18:05 (GMT+09:00) > Title : Re: [E-devel] [Patch] edje_cc : group_inherit feature > > On 29/09/11 04:31, Cedric BAIL wrote: >> I did just move the various _Parser structure inside edje_cc.h instead >> of edje_private.h and fixed some obvious source of bug. As the rest >> looked fine, I pushed it in svn, so every one can now try and use it. >> Thanks for your work. >> >> Have fun ! > > Thanks a lot to the both of you, this was a long awaited feature. > > -- > Tom. > ------------------------------------------------------------------------------ > All the data continuously generated in your IT infrastructure contains a > definitive record of customers, application performance, security > threats, fraudulent activity and more. Splunk takes this data and makes > sense of it. Business sense. IT sense. Common sense. > http://p.sf.net/sfu/splunk-d2dcopy1 > _______________________________________________ > enlightenment-devel mailing list > enl...@li... > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > |
From: Cedric B. <ced...@fr...> - 2011-09-15 08:01:31
|
Hi Jaehwan, 2011/9/15 Jaehwan Kim <jae...@sa...>: > 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 |
From: Gustavo S. B. <bar...@pr...> - 2011-09-15 17:55:50
|
On Thu, Sep 15, 2011 at 5:01 AM, Cedric BAIL <ced...@fr...> wrote: > >>> 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. Reading this ringed a BIG warning in my mind. You need to be careful with the way Edje is done. I did not review the code, but let's remember that: - All references are done by integer identifier. Order will affect that! When you provide insert_before and insert_after, do you recalculate all your ids and reassign them? I recall it was a painful work to do in Edje_Edit - remember about PART:"name" and PROGRAM:"name"... what to do when part is deleted? Do you remember to create a new program and recompile it with the new references on insert_before/after? - what to do with dependencies? One part is relative to another... it is deleted... then? Please consider all these issues, create tests and prove it is right before merging it. I smell lots of possible problems with this thing :-/ -- Gustavo Sverzut Barbieri http://profusion.mobi embedded systems -------------------------------------- MSN: bar...@gm... Skype: gsbarbieri Mobile: +55 (19) 9225-2202 |