On Thu, 22 Nov 2012 03:47:50 +0000 (GMT) Sung-Jin Park <sj76.p...@samsung.com> said:
appwoved! tnx muchly man! :) > Dear Carsten and developers, > > I modified my previous patch. > - correct format > - replace my own list implementation with eina_inlist stuff > - correct configure.ac > > Please kindly review my codes. :) > > Thanks and regards, > Sung-Jin Park > > ------- Original Message ------- > Sender : Sung-Jin Park<sj76.p...@samsung.com> S5/Senior Engineer/System S/W > Lab./Samsung Electronics Date : Nov 21, 2012 16:34 (GMT+09:00) > Title : Re: Re: [E-devel] Fwd: Review request : [Ecore XI2] Add codes for > selecting/retrieving XI2 touch events > > Dear Carsten, > Thank you for your quick and kind review. :) > My opinions are following. > > > 1. Xi2_2 -> is libXi2.2 really libXi2_2.so..... ? really? this has ADDED > > stuff from xi2... but should it have a whole new soname on real systems? > > Oh, does ECORE_CHECK_X_EXTENSION([Xi2], ...) mean that libXi2*.so* are > existing? Library file name for xi2 is libXi.so and it wasn't changed. > I used ECORE_CHECK_X_EXTENSION([Xi2_2], > [XInput2.h], > [Xi], > [XIGrabTouchBegin], > [$want_ecore_x_input]) > in the patch. As I see this macro, 3rd argument ([Xi]) is used for checking > the existence of the file (libXi.so). Please let me know if I didn't > understand properply what you said. > > > 2. XI_2_Major and XI_2_Minor -> i would guess that the headers of xinput2 > > define these.. are these new in 2.2 or were they there before? this smells > > fishy to me. > > Yes, they were in XI2.h in 2.0 version of inputproto. > > > 3. just style-wise... pleasde don't do: > > > > if (EINA_TRUE != find || !touchdev->slot) > > > > instead: > > > > if ((!find) || (!touchdev->slot)) > > > > notice i'm using the booleanness of find ANd using ()'s to group logic > > checks rather than precedence of operation. :) > Oh, I'm sorry, I'll follow the style. :) > > > just a small optimization - you use a while loop to walk thru > > _ecore_x_xi2_touch - you also made it a linked list as well.. why dont you > > use eina_list, or eina_inlist ? inlist inlines the lit node info into the > > struct... also something i tend to do with such lists that dont have any > > specific sort-order... when i do a successful lookup, i put the found item > > at the list start so the theory goes - commonly accessed items are found > > very quickly as they are at the font. unless real life usage is that you > > get pretty much random usage evenly distributed... then there is no > > benefit... for this i suspect it helps a little. especially once we get > > like 5 or 10 devices... and we look this up often.. which is what we do for > > every touch event there. :) > > Thank you :) I didn't have a time for thinking of using eina stuff. > > > more consistent formatting would be nice. ie > > > > static Ecore_X_Touch_Device_Info* _ecore_x_input_touch_info_get()... > > is all one line but others are on 2 lines... make it consistent so its > > easier to read :) > > > > formatting: if(touchdev) -> if (touchdev) ... (notice the space) > > I'll follow the style. :) > > > touchdev->slot can be made part of the calloc of touchdev - alloc 1 memory > > blob that is sizeof Ecore_X_Touch_Device_Info PLUS ((t->num_touches) * > > sizeof (int))... and make slot an array of 1 instead of int *. ie int slot > > [1]; instead of int *slot; > > "slot" is a container which contains each finger status in it. > The number of items of slot will be changed with the maximum number of fingers > being recognized by a touch screen device. > If the touch screen device can recognizes ten fingers at the same time > and it can makes ten finger events, touchdev->max_touch will be ten > and slot will be allocated with malloc(sizeof(int)*10). > This is why I declare slot as a pointer. > > > otherwise looks ok to me. can you fix the above and re-submit? :) tnx > > muchly! > Absolutely. :) > > Thanks and regards, > Sung-Jin Park > > ------- Original Message ------- > Sender : Carsten Haitzler<ras...@rasterman.com> > Date : 2012-11-21 13:37 (GMT+09:00) > Title : Re: [E-devel] Fwd: Review request : [Ecore XI2] Add codes for > selecting/retrieving XI2 touch events > > On Wed, 21 Nov 2012 04:05:19 +0000 (GMT) 박성진 <sj76.p...@samsung.com> said: > > ok - i didn't get started with your first one... so i'll start here. note - i > havent dug in depth and looked all over libxinput2.2 itself... > > 1. Xi2_2 -> is libXi2.2 really libXi2_2.so..... ? really? this has ADDED stuff > from xi2... but should it have a whole new soname on real systems? > 2. XI_2_Major and XI_2_Minor -> i would guess that the headers of xinput2 > define these.. are these new in 2.2 or were they there before? this smells > fishy to me. > 3. just style-wise... pleasde don't do: > > if (EINA_TRUE != find || !touchdev->slot) > > instead: > > if ((!find) || (!touchdev->slot)) > > notice i'm using the booleanness of find ANd using ()'s to group logic checks > rather than precedence of operation. :) > > just a small optimization - you use a while loop to walk thru > _ecore_x_xi2_touch - you also made it a linked list as well.. why dont you use > eina_list, or eina_inlist ? inlist inlines the lit node info into the > struct... also something i tend to do with such lists that dont have any > specific sort-order... when i do a successful lookup, i put the found item at > the list start so the theory goes - commonly accessed items are found very > quickly as they are at the font. unless real life usage is that you get > pretty much random usage evenly distributed... then there is no benefit... > for this i suspect it helps a little. especially once we get like 5 or 10 > devices... and we look this up often.. which is what we do for every touch > event there. :) > > more consistent formatting would be nice. ie > > static Ecore_X_Touch_Device_Info* _ecore_x_input_touch_info_get()... > is all one line but others are on 2 lines... make it consistent so its easier > to read :) > > formatting: if(touchdev) -> if (touchdev) ... (notice the space) > > touchdev->slot can be made part of the calloc of touchdev - alloc 1 memory > blob that is sizeof Ecore_X_Touch_Device_Info PLUS ((t->num_touches) * sizeof > (int))... and make slot an array of 1 instead of int *. ie int slot[1]; > instead of int *slot; > > otherwise looks ok to me. can you fix the above and re-submit? :) tnx muchly! > > > Dear developers, > > > > I found some of my debug routines are existing in my patch and I removed > > them and added exception codes. I attached my newer patch. > > > > Thanks and regards, > > Sung-Jin Park > > > > ------- Original Message ------- > > Sender : 박성진<sj76.p...@samsung.com> S5(책임)/책임/System S/W Lab(S/W센터 > > )/ 삼성전자 Date : 2012-11-19 20:33 (GMT+09:00) > > Title : Review request : [Ecore XI2] Add codes for selecting/retrieving XI2 > > touch events > > > > Dear developers, > > I added codes for selecting XI2 touch events(cf. XI_TouchBegin, > > XI_TouchUpdate and XI_TouchEnd) and codes for retrieving touched finger(s) > > index from corresponding events. > > > > Please kindly review my codes. > > All comments will be welcomed. :) > > > > Thanks and regards, > > Sung-Jin Park<p> </p><p> </p> > > -- > ------------- Codito, ergo sum - "I code, therefore I am" -------------- > The Rasterman (Carsten Haitzler) ras...@rasterman.com > > <p> </p><p> </p><p> </p><p> </p> -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
