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 ------------------------------------------------------------------------------ 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
