Dear Daniel,
Overall, the patch looks good, there are a couple of comments/questions
which by themselves aren't so bad, but because there are more than a
few, I'm sending it back to them to fix them.
Several comments/questions:
1. style { name: "seg_text_style_disabled";
Why the font_size here is 24 while in the others it's 16?
2. In the programs of the edc, why don't they use macros for the types?
using the numbers is fairly confusing... (for example line 1469 of the
patch).
3. Also, look at the style in the other parts of the theme, usually we
go down lines after brackets, for eaxmple:
BAD: rel2 {relative: 0.0 0.0; to_y: "padding_bottom"; }
GOOD:
rel2 {
relative: ...
...
}
4. in line 1360 of the patch they accidentally used double semicolons
instead of just one.
5. In the test they added: when selecting the rightmost one at the top
(seg5) I get a 1px line that just looks odd.
6. I took a look at _position_items(Widget_Data *wd)
why don't you just a box? I mean, unless I understand this code wrong,
this is exactly what a box does. So you don't really need to position
items by yourself.
7. in line 449 of the patch (and many other places) they are not
following the coding conventios, it's:
(index == wd->item_count-1) but it should be
(index == (wd->item_count - 1))
note the spaces and the brackets... There are other places as well,
please let them go through it and fix it.
Those are my main comments, please fix them and resend. I think changing
to box, and all of my other comments will simplify the code a lot, and
will also simplify ui-mirroring handling (for example in _update_list)
because in that case they'll just have to mirror the items and mirror
the box, everything else will happen automatically.
Thanks,
Tom.
On Thu, 2011-03-17 at 18:33 +0900, Daniel Juyung Seo wrote:
> Hello,
> Prince Kumar Dubey and Govindaraju SM sent me a revised version of
> elm_segment_control widget.
> They applied Tom's new TEXT part feature (fit and size_range).
> This supports for mirror mode.
> Please find email and attachment below.
> Thanks.
> Daniel Juyung Seo (SeoZ)
>
> ---------------------------------------------------
> Hi Mr. Seo,
> Please find in the attachment the fresh elm_segmentcontrol widget patch
> [elm_segmentcontrol.patch+Images+README] for SVN upstreaming.
> Now, elm_segmentcontrol using part TEXT instead of elm_label.
> I am also attaching segmentcontrol test RTL/LTR snapshots for reference.
> Can you please post it to EFL community?
>
> Patch: New elementary widget: elm_segment_control.
> ====================
> Authors: Govindaraju SM <[email protected]>, Prince Kumar Dubey
> <[email protected]>
> Change Log: New widget elm_segment_control. Segment Control Widget is
> a horizontal control made of multiple segment items together, each
> segment item functioning similar to discrete two state button. Only one
> Segment item can be at selected state.
> ====================
> ------------------------------------------------------------------------------
> Colocation vs. Managed Hosting
> A question and answer guide to determining the best fit
> for your organization - today and in the future.
> http://p.sf.net/sfu/internap-sfd2d
> _______________________________________________ enlightenment-devel mailing
> list [email protected]
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
------------------------------------------------------------------------------
Colocation vs. Managed Hosting
A question and answer guide to determining the best fit
for your organization - today and in the future.
http://p.sf.net/sfu/internap-sfd2d
_______________________________________________
enlightenment-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
