First of all, I appreciate for your detail answers. Do you suggest to move ellipsis feature from elm to edje textblock?
So except that, I'm going to make clean up patch for elm_label. Thanks a lot. > -----Original Message----- > From: Carsten Haitzler (The Rasterman) [mailto:ras...@rasterman.com] > Sent: Monday, December 20, 2010 4:17 PM > To: Hyoyoung Chang > Cc: enlightenment-devel@lists.sourceforge.net > Subject: Re: [E-devel] elm_label patch(ellipsis, sliding) > > 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
