Re: [E-devel] [patch] elm_genlist - add item flip set/get

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

Hello, I modifed fliP_set api to use ecore_job.
It resolved version of mouse_{move, up} bugs.

On Sun, Mar 4, 2012 at 2:47 PM, Hyoyoung Chang <hyo...@gm...> wrote:
> As discussed in irc, i modified and removed draggable in flip mode.
>
> @@ -975,6 +975,8 @@ _mouse_move(void        *data,
>    if (ady < 0) ady = -dy;
>    minw /= 2;
>    minh /= 2;
> +   if (it->flipped)
> +     return;
>    if ((adx > minw) || (ady > minh))
>      {
>         it->dragging = EINA_TRUE;
>
> What do you think about it?
> If it's not a good idea, i think flip mode should have only one item.
>
> On Sat, Mar 3, 2012 at 1:39 PM, Hyoyoung Chang <hyo...@gm...> wrote:
>> On Fri, Mar 2, 2012 at 7:39 PM, Daniel Juyung Seo <seo...@gm...> wrote:
>>> Thanks for your patch.
>>> Everything is fine but I still see the floating item bug.
>> Could you tell me how to reproduce?
>> i tried several times but i can't see it
>>>
>>> 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.
>> dragging is enabled by mouse_move (except group item)
>> If then how about to define flipped item would not draggable?
>> Or as i think we should restrict to only one flipped mode item.
>> What do you think so?
>>>
>>> Thanks.
>>>
>>> Daniel Juyung Seo (SeoZ)
>>>
>>> On Fri, Mar 2, 2012 at 5:14 PM, Hyoyoung Chang <hyo...@gm...> wrote:
>>>> I attached improved version of flip patch
>>>>
>>>> Thanks
>>>>
>>>> On Thu, Mar 1, 2012 at 10:52 PM, Hyoyoung Chang <hyo...@gm...> wrote:
>>>>> First of all, thanks for detailed review
>>>>>
>>>>> On Thu, Mar 1, 2012 at 3:01 PM, Daniel Juyung Seo <seo...@gm...> 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 <hyo...@gm...> 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
>>>>>>> enl...@li...
>>>>>>> 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
>>>>>> enl...@li...
>>>>>> 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
>>>> enl...@li...
>>>> 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
>>> enl...@li...
>>> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel