On Fri, Aug 29, 2014 at 7:25 PM, Carsten Haitzler <[email protected]> wrote: > On Fri, 29 Aug 2014 13:18:37 -0700 Jason Gerecke <[email protected]> said: > > review below. btw. it'd be really awesome if you could use arc to land these > patches for review. this is part of our "getting organized" and using tools to > make sure patches don't vanish in an inbox. :) there is a special limited > sized > review list for patches which allows inline code comments etc. etc. see > http://phab.enlightenment.org http://phab.enlightenment.org/w/arcanist/ > > 0001-Break-_ecore_x_input_handler-into-task-specific-sub-.patch: > > it'd be nice if it didn't have so many whitespace changes. it'd be REALLY nice > if you could "fix" the whitespace to match what was there before. i'm sifting > through it and almost all of this patch is whitespace changes (that are not > needed as no extra if, whole, for etc. clauses). > My editor seems to have gotten a little indent happy... I'll work on cleaning up that mess up :)
> 0002-Select-XI2-events-whenever-possible.patch: > > i am not sure here, but this may break multitouch support mpx-style that > checks > for floating slaves vs slaves. (reading the diff is not fun :) it is removing > all the detection code and that is worrying, especially as it's hard to test > this support (but has been tested in the past when written). > Basically this patch has the X server send Button, Motion, and Touch events without regard to if the device is floating. A driver with MPX multitouch that uses a floating device per finger shouldn't be sending XI2 touch events, so I doubt adding them to the mask would break anything. On the flip side, a driver with XI2 multitouch devices *will* send button/motion events (for the first finger) along with touch events, but the '_ecore_x_input_multi_handler' function only responds to touch events, so requesting more events from the server shouldn't break anything in this case either. > 0003-Define-and-implement-new-Ecore_Event_Axis_Update-eve.patch: > > you have some ome todo's and fixme's... :)). one thing - the #include of > xserver-properties.h ... that literally is not an existing installed file > anywhere on my xorg installation. so this is going to be a problem... it won't > compile. and it likely isn't going to compile for anyone else. what is this > doing there and why? (oh .. and we don;'t use egyptian brackets in count_bits > ()) also @since 1.12 in the doxy comments so we know what version of efl this > was introduced from :) oh and typedef enum _Ecore_Axis_Label like > Ecore_Event_Axis_Update was. and in fact like 0004 below - flatten out > Ecore_Axis that isnt even typedeffed. :) > The xserver-properties.h file contains the names that drivers are expected to use when naming their axes. It defines the constants I use later in that file (e.g. AXIS_LABEL_PROP_ABS_X). It looks like GTK at least just hard-codes the strings instead of depending on the header (which is part of the xorg server devel package). I'll see what I can do about the todos and fixmes :D I expected more critique on my design so only roughed out the implementation and punted on the really hard parts in case they needed to be changed anyway. > 0004-Implement-EVAS_CALLBACK_AXIS_UPDATE-event-and-friend.patch: > > might i suggest not using struct _Evas_Axis * as a parameter but actually > passing in the enum lable and double. this makes it easier for bindings to > other languages. so pass them in as 2 params. that prbably means removing this > struct entirely (also being a raw struct is inconsistent with our typedeffing > of all of these) so just break out enum (actually also typedef the enum > please) > and double value into the event update struct :) flatten the world! :) > The _Evas_Axis * is an array (whose length is given by 'naxes'), not a single object. There'll be maybe a half-dozen enum/double pairs depending on the device sending the event. Would using some kind of EFL-specific list datatype work better perhaps? > 0005-Wire-the-Ecore-and-Evas-implementations-of-axis-upda.patch: > > fine EXCEPT i see no patch to expose ecore_event_evas_axis_update in the > header > file :) it's EAPI (exposed api)... thus shoulld have something in > Ecore_Input.h :) don't forget @since 1.12 in a doxy comment :) > So _that's_ what EAPI means :D I should confess that pretty much all of my code was mechanically written; grepping through the tree for instances of e.g. "mouse_down" or "multi_down" and then creating similar-looking blocks that said "axis_update". I'm honestly surprised there aren't more little problems like this... Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... > 0006-DEBUG-Add-axis-update-logging-to-evas-multi-touch.c.patch: > > fine > > 0007-DEBUG-Make-evas-multi-touch-demo-be-pressure-sensiti.patch > > fine > >> Its been quite a while since my first patches were sent, so >> unsurprisingly they no longer apply to the master branch. I've >> attached an updated patchset which has been rebased and should work >> once more. Hopefully it makes reviewing the code a bit more attractive >> to those who've been standing on the sidelines... I'm not terribly >> familiar with the EFL codebase so I'm *sure* somebody out there could >> provide some feedback on the design :) >> >> Jason >> --- >> Now instead of four in the eights place / >> you’ve got three, ‘Cause you added one / >> (That is to say, eight) to the two, / >> But you can’t take seven from three, / >> So you look at the sixty-fours.... > > > -- > ------------- Codito, ergo sum - "I code, therefore I am" -------------- > The Rasterman (Carsten Haitzler) [email protected] > ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
