First of all, thanks for detailed review
On Thu, Mar 1, 2012 at 3:01 PM, Daniel Juyung Seo <[email protected]> wrote:
> Thanks Hyoyoung, overall it looks good :)
> I have some comments.
>
> 1. diff option
> Create a diff with -x -up optoin.
> It shows c function name for each changes.
> This makes review so much easier and faster.
> $ svn diff -x -up *.c
>
> 2. remove default properties from edc.
> - mouse_events: 1
> - rel1.relative: 0 0
> - rel2.relative: 1 1
>
> 3. use elm_win_util_standard_add
> elm_win_add + elm_bg_add is ok but I encourage you to use
> elm_win_util_standard_add in normal cases.
> It does elm_win_add + elm_bg_add and reduces 6 lines of code.
I didn't know it. I'll apply
>
> 4. formatting
> There are some wrong formatted codes.
In or edc?
>
> 5. build warnings
> test_genlist.c: In function ‘_flip_icon_clicked_cb’:
> test_genlist.c:2495:82: warning: unused parameter ‘event_info’
> test_genlist.c: In function ‘gl16_content_get’:
> test_genlist.c:2516:17: warning: unused variable ‘ic’
> test_genlist.c: In function ‘test_genlist16’:
> test_genlist.c:2553:39: warning: unused variable ‘bt’
> test_genlist.c:2553:33: warning: unused variable ‘bx2’
Yeah, It should be removed
>
> 6. In _flip_icon_clicked_cb, you can just do:
> tit->checked = !tit->checked
> However, I would recommend to use elm_genlist_item_flip_get() instead
> of remembering tit->checked.
> This covers more API usage and it's useful example.
> And without checked, you don't even need to use tit array, you can
> just pass "(void *)(long) i" as an item data.
> If you don't prefer to use that casting trick, you can just use tit
> but without checked member.
U're right :)
>
> 7. const to getters.
> Yes, add "const" to getters.
> EAPI Eina_Bool elm_genlist_item_flip_get(const Elm_Object_Item *it);
Yeah i'll add it
>
> 8. in elm_genlist_item_flip_set
> Instead of
> _it->flipped = EINA_TRUE / EINA_FALSE;
> You can do
> _it->flipped = flip;
>
> 9. floating item bug
> a. flip many items
> b. scroll
> Then I can see floating items bug. I attached screenshot.
>
> This also happens easily in this case:
> a. flip many items
> b. drag flipped item and scroll it out of viewport.
>
> 10. if statement in elm_genlist_item_flip_set
> You already checked if _it->flipped equals to flip,
> so you do not need to check _it->flipped again.
>
> if (_it->flipped == flip) return; <---- you already checked it
> if (flip)
> else
> {
> if (_it->flipped) <---- do not need to check this
> else
> }
First checking is to verify changed.
Second checking is about previous status.
So It leaves at it is.
I don't think it's good or clean code either.
But checking is right. Maybe it should be re-written to make clean code.
>
> These are just trivial comments.
> Please apply them.
> Otherwise, this patch looks good :)
> Thanks.
you're always passionate.
Thanks very much for detail review.
>
> Daniel Juyung Seo (SeoZ)
>
> On Wed, Feb 29, 2012 at 9:17 PM, Hyoyoung Chang <[email protected]> wrote:
>> Dear all.
>>
>> I made a patch to introduce new genlist item mode.
>> Two public apis are added.
>> +EAPI void elm_genlist_item_flip_set(Elm_Object_Item *it, Eina_Bool flip);
>> +EAPI Eina_Bool elm_genlist_item_flip_get(Elm_Object_Item *it);
>>
>> It provides on-the-flying item change. It works like that a new item
>> added on existed item.
>> In elementary test, you can test it.
>> It's useful at adding widgets or show buttons in genlist item.
>>
>> Thanks.
>>
>> ------------------------------------------------------------------------------
>> Virtualization & Cloud Management Using Capacity Planning
>> Cloud computing makes use of virtualization - but cloud computing
>> also focuses on allowing computing to be delivered as a service.
>> http://www.accelacomm.com/jaw/sfnl/114/51521223/
>> _______________________________________________
>> enlightenment-devel mailing list
>> [email protected]
>> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>>
>
> ------------------------------------------------------------------------------
> Virtualization & Cloud Management Using Capacity Planning
> Cloud computing makes use of virtualization - but cloud computing
> also focuses on allowing computing to be delivered as a service.
> http://www.accelacomm.com/jaw/sfnl/114/51521223/
> _______________________________________________
> enlightenment-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
>
------------------------------------------------------------------------------
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
_______________________________________________
enlightenment-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
