Thanks for your review. I attached fixed source diff code. Since I uploaded first diff code, genlist item realize function and item edc had been changed by cedric. So I also fixed my code according to the latest genlist code. This version supports edit mode selection handling.
Please review about this. Thanks. -----Original Message----- From: Daniel Juyung Seo [mailto:seojuyu...@gmail.com] Sent: Thursday, April 21, 2011 5:31 PM To: Seunggyun Kim Cc: enlightenment-devel@lists.sourceforge.net Subject: Re: [E-devel] [Patch] elm_genlist - added new feature : genlist edit mode Dear Seunggyun, I have some comments. 1) Formatting. There are many wrong formatting: indentation, braces, using one line after if, alignment of function declaration, trailing whitespaces and etc. 2) Exception for selection. There are some exception handling code for selection. Is there any reason to do this? 3) It will be better to move _edit_mode_item_unrealize() above it->realized = EINA_FALSE; Because after unrealizing all instances of that item, we can say the item is unrealized. @@ -1917,6 +1934,7 @@ _item_unrealize(Elm_Genlist_Item *it, Eina_Bool ca it->states = NULL; it->realized = EINA_FALSE; it->want_unrealize = EINA_FALSE; + if (it->wd->edit_mode) _edit_mode_item_unrealize(it); } 4) Missing braces in if statement. Unnecessary {, } for one liner. + if (it->wd->edit_mode && it->itc->edit_item_style) + { + _edit_mode_item_controls(it, it->scrl_x, it->scrl_y); + } 5) How about 'position' or something else instead of 'controls'? Just a suggestion, if you mind just leave it :) >> _edit_mode_item_controls() 6) Why two braces? And it's not reasonable check wd->edit_mode inside _edit_mode_item_realize(). Because it's not possible to go that line if it's not in an edit mode. >> if ((it->wd->edit_mode)) 7) Just return wd->edit_mode. >> if (wd->edit_mode) return EINA_TRUE; >> else return EINA_FALSE; 8) Use !! for Eina_Bool type parameter in EAPIs. >> wd->edit_mode = edit_mode; wd->edit_mode = !!edit_mode; This workarounds application programmer's mistake. 9) How about using elm_genlist_realized_items_get() API? >> EINA_INLIST_FOREACH(wd->blocks, itb) >> { >> if (itb->realized) >> { >> EINA_LIST_FOREACH(itb->items, l, it) >> { >> if (it->flags != ELM_GENLIST_ITEM_GROUP && it->realized) 10) Missing braces. >> if (it->flags != ELM_GENLIST_ITEM_GROUP && it->realized) if ((it->flags != ELM_GENLIST_ITEM_GROUP) && (it->realized)) There are more of it. Please check all. 11) Use __UNUSED__ for not used parameters. 12) Space between if and (. >> if(!tit->checked) 13) Fix warnings. If you don't have enough warnings, add "-W -Wall -Wextra" to CFLAGS. 14) Seg fault. It's not working when I go to "Genlist Edit" menu. It's killed. Hmm I'm not sure if only I have problems with running genlist edit in elementary_test. It has a seg fault. Other e-developers, please test this. Thank you. Daniel Juyung Seo (SeoZ) On Tue, Apr 19, 2011 at 4:32 PM, Seunggyun Kim <sgyun....@samsung.com> wrote: Thanks for your review :) I attached fixed source diff code. The edit edc is like below table. ------------------------------------------------------------------------- | elm.edit.icon.1 | elm.swallow.edit.content | elm.edit.icon,2 | ------------------------------------------------------------------------- Now it is showing only edit mode set animation. If this is committed, I'll send another patch about edit mode unset animation. Thanks:) -----Original Message----- From: Carsten Haitzler (The Rasterman) [mailto:ras...@rasterman.com] Sent: Monday, April 18, 2011 7:41 PM To: Seunggyun Kim Cc: enlightenment-devel@lists.sourceforge.net Subject: Re: [E-devel] [Patch] elm_genlist - added new feature : genlist edit mode On Sat, 16 Apr 2011 17:30:13 +0900 Seunggyun Kim <sgyun....@samsung.com> said: i'm rather happy that i now get to grumble about much more esoteric things in patches :) ... why is ediit_mode not Eina_Bool and even better it's together with all the other Eina_Bool var:1 fields? this leads to much better struct packing. saves a byte :) double longpress_timeout; + int edit_mode; }; why are edit_icon_objs and edit_obj not higher up with pointers like Evas_Object *spacer etc.? better struct packing and alignment. :) + Eina_List *edit_icon_objs; + Evas_Object *edit_obj; bonus space at end before ")" :) + if (it->itc->edit_item_style ) this looks wrong. default case is item/edit_default but other styles are item/stylename. really shouldnt this be like "item/edit/default" and "item/edit/stylename".. in fact always item/edit/STYLENAME... ? simpler code and more consistent and style can provide a simpler stylename not needing edit_ in it... :) + if (it->itc->edit_item_style && strcmp(it->itc->edit_item_style, "default")) + { + strncat(buf, it->itc->edit_item_style, sizeof(buf) - strlen(buf)); + _elm_theme_object_set(it->base.widget, it->edit_obj, "genlist", buf, elm_widget_style_get(it->base.widget)); + } + else + { + _elm_theme_object_set(it->base.widget, it->edit_obj, "genlist", "item/edit_default", elm_widget_style_get(it->base.widget)); + } "original_edc" - you should namespace edc partnames "officially" required by elm. "elm.swallow.edit.content" for example + edje_object_part_swallow(it->edit_obj, "original_edc", it->base.view); decide on a consistent naming of signals here. "elm,state,edit,enabled" and "elm/state/edit/disabled" for example... BUT.. why emit a signal at the end.. just before you go DELETE the edit mode obj? pretty pointless signalling it then deleting it. if your plan is to allow an animation to "go out of edit mode" then keep the object around until that animation is done (some signal tells you tis done from the obj). + if (effect_on) edje_object_signal_emit(it->edit_obj, "elm,state,emode_enabled_effect", "elm"); + else edje_object_signal_emit(it->edit_obj, "elm,state,emode_enabled", "elm"); AND + edje_object_signal_emit(it->edit_obj, "elm,state,edit_end,disable", "elm"); AND + evas_object_del(it->edit_obj); base.view is already a sub object of it->base.widget when it was created. swallowing it into the edit obj didn't change that. it moved down 1 level in the evas object tree, but not in the elm one. :) no need for this. + elm_widget_sub_object_add(it->base.widget, it->base.view); that's it! :) not much. > Hello. All. > > I made genlist edit mode. > > The edit mode can be used when the application want to change existing item > edc into more expanded item edc. > The application can use this edit mode for genlist item editing. > For example, if the application want to show check box or plus/minus button > at the left or right of item in some case, > they can use this edit mode. > > This edit mode style includes existing item and will be extended according > to edit_item_style. > > edit_item_style "edit_default" EDC layout is like below. each part is > swallow part. > the existing item is swllowed to original_edc part. > -------------------------------------------------------------------------- > | elm.edit.icon.1 | original_edc | > elm.edit.icon,2 | > -------------------------------------------------------------------------- > > After applitciaon set edit mode, genlist loads > Elm_Genlist_Item_Class.edit_item_style if the style is not NULL. > Then genlist swallows a existing item to "original_edc" swallow part in > edit_item_style EDC.) > and the genlist will start to call application icon_get callback function to > swallow application objects(checkbox, radio button, plus/minus button. etc.) > > In time, the application returns checkbox or another object in icon_get > callback. edit mode EDC part name is "elm.edit.icon.1", "elm.edit.icon.2".. > > This API will change all realized items to edit mode style. In caes the list > is scrolled, when an item is realized, the item will be also changed to > edit mode style. > > For edit mode, I newly added elm_genlist_edit_mode_set / > elm_genlist_edit_mode_get api. > > I changed below files. > > a) elm_genlist.patch.txt > ----------------------------------- > elementary/src/lib/Elementary.h.in > elementary/src/lib/elm_genlist.c > > b) test_genlist.patch.txt > ----------------------------------- > elementary/src/bin/test.c > elementary/src/bin/test_genlist.c > > c) theme_default.patch.txt > ----------------------------------- > elementary/data/themes/default.edc > > Please review about this. > Thanks. -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ---------------------------------------------------------------------------- -- Benefiting from Server Virtualization: Beyond Initial Workload Consolidation -- Increasing the use of server virtualization is a top priority.Virtualization can reduce costs, simplify management, and improve application availability and disaster protection. Learn more about boosting the value of server virtualization. http://p.sf.net/sfu/vmware-sfdev2dev _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
genlist_edit_mode.diff
Description: Binary data
------------------------------------------------------------------------------ WhatsUp Gold - Download Free Network Management Software The most intuitive, comprehensive, and cost-effective network management toolset available today. Delivers lowest initial acquisition cost and overall TCO of any competing solution. http://p.sf.net/sfu/whatsupgold-sd
_______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
