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>
------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
