Thanks for your patch.
Everything is fine but I still see the floating item bug.
And if the item is on dragging, it should not be unrealized.
As far as I know, when an item is on dragging, unrealizing the item
will cause unexpected problems.
I don't remember what it is exactly.
So I politely recommend you to solve this bug with another solution.
Thanks.
Daniel Juyung Seo (SeoZ)
On Fri, Mar 2, 2012 at 5:14 PM, Hyoyoung Chang <hyoyo...@gmail.com> wrote:
> I attached improved version of flip patch
>
> Thanks
>
> On Thu, Mar 1, 2012 at 10:52 PM, Hyoyoung Chang <hyoyo...@gmail.com> wrote:
>> First of all, thanks for detailed review
>>
>> On Thu, Mar 1, 2012 at 3:01 PM, Daniel Juyung Seo <seojuyu...@gmail.com>
>> 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 <hyoyo...@gmail.com> 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
>>>> enlightenment-devel@lists.sourceforge.net
>>>> 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
>>> enlightenment-devel@lists.sourceforge.net
>>> 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
> enlightenment-devel@lists.sourceforge.net
> 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
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
