Dear Mr. Kim. As we discussed, can you changed the API name to elm_genlist_items_mode_all_set()? Btw, do this after reorder mode patch is committed to avoid the conflict :)
Raster, I propose elm_genlist_items_mode_all_set() not elm_genlist_items_all_mode_set() because elm_xxx_all_set() is already there :) Thanks. Daniel Juyung Seo (SeoZ) On Fri, May 13, 2011 at 4:58 PM, Carsten Haitzler <ras...@rasterman.com> wrote: > On Fri, 13 May 2011 10:15:34 +0900 Daniel Juyung Seo <seojuyu...@gmail.com> > said: > > none - i had my say. :) commitsors > >> Any comments on this patch? >> >> Daniel Juyung Seo (SeoZ) >> >> On Tue, May 3, 2011 at 8:00 PM, Seunggyun Kim <sgyun....@samsung.com> wrote: >> > 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 >> > >> > >> > ------------------------------------------------------------------------------ >> > 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 >> > >> > >> >> ------------------------------------------------------------------------------ >> Achieve unprecedented app performance and reliability >> What every C/C++ and Fortran developer should know. >> Learn how Intel has extended the reach of its next-generation tools >> to help boost performance applications - inlcuding clusters. >> http://p.sf.net/sfu/intel-dev2devmay >> _______________________________________________ >> enlightenment-devel mailing list >> enlightenment-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel >> > > > -- > ------------- Codito, ergo sum - "I code, therefore I am" -------------- > The Rasterman (Carsten Haitzler) ras...@rasterman.com > > ------------------------------------------------------------------------------ What Every C/C++ and Fortran developer Should Know! Read this article and learn how Intel has extended the reach of its next-generation tools to help Windows* and Linux* C/C++ and Fortran developers boost performance applications - including clusters. http://p.sf.net/sfu/intel-dev2devmay _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
