On Thu, 16 Dec 2010 09:46:24 +0900 Hyoyoung Chang <hyoyoung.ch...@samsung.com> said:
ok. here's some feedback. 1. 0001 and 0002 are really the same patchset. so in future just send those as 1 patch. 2. it's an improvement on what was there, but the more i lok at it, the more this needs to move into edje. it's too inefficient to do it outside and too unclean. keeping that in mind - i'm not saying no to the patch, just that this should move into edje. multi-line ellipsis for textblock should be in edje for sure. the scrolling back and forth should be there too. i can go into reasons why, but i won't here unless you really want to know - but i think it should move to edje. this will have to be scheduled for post edje 1.0 so keep this in mind and add it to your "todo list" to move these features into edje for 1.1 (and i'd hope we can get 1.0 maybe in jan 2011). 3. with the above in mind i could comment on some small style things like linemode should be a Eina_Bool not an int. and should likely be "multiline" not linemode as thats what it does 4. and yes the FIXME's that you copies _str_append() from entry - and _mkup_to_text() really shouldnt have been copied. should have been moved to a utils file as you say :). i've done that for you here as part of applying your patch. 5. style-wise we dont declare vars c++-style in the middle of code. style-wise we try and keep them all at the top of sections (top of func, loop, if etc.). it means they are easier to track and spot as they all appear in the scrope together and go away with the matching } end scope. as opposed to appear half-way through. less chance of errors, so please in future keep vars at the top. at least keep the decls together without spacing them :) 6. fixme's about only caring about utf8 - no need. that is all efl cares about. utf8 is the strip format/api of choice for efl. i t does nothing else. :) 7. in _ellipsis_cut_chars_to_widget() you malloc a tmp string, but never check malloc failure. perhaps alloca would have been better here too as strings for labels should feasibly fit on the stack anyway. they wont be long - unlike things that may be in entries... though this stuff should go into edje. same in +_ellipsis_cut_lines_to_widget() 8. minshowcount - underscores would be nice, or shorter like mc, minc, min_count etc, but.. strlen() to set it to what is known to be 3 chars? should just be "3" :) or sizeof(ellipsis_string) - 1. 9. evas_textblock_cursor_range_text_get() returns a string u have to free() - i see no free() for it! leak leak leak. (fixed it for you). also you don't handle a NULL ret. 10. style. if (w < 0 && h < 0) should be if ((w < 0) && (h < 0)). similar instances of this throughout the patch. if (plaintxt != NULL) can be shorter -> if (plaintext) 11. ellen should be minshowcount or showcount or count - and an int, not Evas_Coord. 12. using h / linenum * 1.0 to "cast" things into doubles is.. well.. odd. use casts if that is what you want. 13. warnings: elm_label.c:766:4: warning: "/*" within comment elm_label.c: In function ‘_ellipsis_cut_chars_to_widget’: elm_label.c:399: warning: unused parameter ‘fontsize’ elm_label.c:399: warning: unused parameter ‘multiline’ elm_label.c: In function ‘_ellipsis_cut_lines_to_widget’: elm_label.c:462: warning: unused parameter ‘fontsize’ elm_label.c:462: warning: unused parameter ‘multiline’ those really are unused params. why they exist i dont know. i removed them. so i've put it in, but please heed the above for future patches. :) (this is just for ellipsis changes - and it is possible i missed 1 or 2 things - if someone notices - speak up/fix it in svn). > Ah, Sorry. It's filtered out. > > I re-attached patch files. > > ------------------------------------------- > Hyoyoung CHANG > Engineer > > SAMSUNG ELECTRONICS, Co., Ltd. > E-mail: hyoyoung.ch...@samsung.com > ------------------------------------------- > > -----Original Message----- > From: Hyoyoung Chang [mailto:hyoyoung.ch...@samsung.com] > Sent: Wednesday, December 15, 2010 8:45 PM > To: 'enlightenment-devel@lists.sourceforge.net' > Subject: elm_label patch(ellipsis, sliding) > > Dear Elementary developers. > > It's a elm_label patch. > My previous patch is too big to submit. > So I did split into patch files. > (Thanks for Gastavo and Rasterman) > > main changes are > 1. refine ellipsis algorithm > - improve to cut string to fit > 2. adding label text sliding feature > > Thank you. > > ------------------------------------------- > Hyoyoung CHANG > Engineer > > SAMSUNG ELECTRONICS, Co., Ltd. > E-mail: hyoyoung.ch...@samsung.com > ------------------------------------------- -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ Lotusphere 2011 Register now for Lotusphere 2011 and learn how to connect the dots, take your collaborative environment to the next level, and enter the era of Social Business. http://p.sf.net/sfu/lotusphere-d2d _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
