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