On Sat, 30 Aug 2014 18:37:13 -0700 Jason Gerecke <killert...@gmail.com> said:
> On Fri, Aug 29, 2014 at 7:25 PM, Carsten Haitzler <ras...@rasterman.com> > wrote: > > On Fri, 29 Aug 2014 13:18:37 -0700 Jason Gerecke <killert...@gmail.com> > > 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. hmmm - for now, ok. > > 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'd recommend hardcoding them here ala gtk. :) there is nothing in these patches to check for this header, and since it's part of server devel 'd probably avoid relying on it. :) > 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. design doesn't seem much of a problem :) > > 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? oh. array. dang. then definitely typedef the axis struct. always. dont leave it a raw struct (same with enum as above) > > 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... yup. EAPI is everywhere indicating it's exposed to external entities (outside the lib/module.binary etc.). i though it would be obvious after reading a public header and seeing every single function prefixed with EAPI. it's actually important. it's a macro that sets attribute of visibility, so if you strip -x the file these symbols remain. also on windows it does a cdecl indicating to the linker to export this symbol. if you don't, on windows, these functions have no symbols and are inaccessible. it isn't needed for cross-file usage, or using any symbol within a single .so or single executable binary, but it is needed for access from outside that destination file. > 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) ras...@rasterman.com > > > > ------------------------------------------------------------------------------ > Slashdot TV. > Video for Nerds. Stuff that matters. > http://tv.slashdot.org/ > _______________________________________________ > enlightenment-devel mailing list > enlightenment-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
