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
