On Mon, 20 Dec 2010 19:35:09 +0900 Hyoyoung Chang <hyoyoung.ch...@samsung.com> said:
> First of all, I appreciate for your detail answers. that's what code review is all about. :) patch submission is not just a "please just apply this" it's definitely a review process, so it can often mean rejecting of patches. right now i'm being "nice" and am doing lots of fixes for you rather than rejecting it. but this also means it takes quite a while - that is why we ask for patches to be small and dont "dump" them in large batches - spread them out over time. :) > Do you suggest to move ellipsis feature from elm to edje textblock? absolutely - yes!. > So except that, I'm going to make clean up patch for elm_label. > Thanks a lot. well i already fixed it all up for you and put it into svn :) you don't have to do anything to elm_label right now - you need to add the above "move it to edje" to your todo list and think about it for a bit, and when you're ready, do that, and send patches removing it from elm and putting it in edje. :) > > -----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 > -- ------------- 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
