I've finished making the requested updates, but forgot to check to see
what your thoughts are about the coordinate system used in the events.
Right now data is being passed through pretty much as-is, but this
might not be appropriate if a window has been rotated. I assume we
want to adjust the reported AZIMUTH and TWIST values so that the
application doesn't have to worry about it, but where can I find the
necessary information? Also, is there any sense in adjusting the other
axes (e.g. X, Y, {TOUCH,TOOL}_WIDTH_{MAJOR,MINOR}) to account for e.g.
scaling?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.... On Sat, Aug 30, 2014 at 7:32 PM, Carsten Haitzler <ras...@rasterman.com> wrote: > 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 > ------------------------------------------------------------------------------ Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
