From: Hyoyoung C. <hyo...@gm...> - 2012-03-01 13:52:32
|
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 > |