search

bendahl / uinput

Go wrapper for uinput on LINUX
MIT License
95 stars 30 forks source link

Add touch screen support #9

Closed sashko closed 6 years ago

sashko commented 6 years ago

I would like to make clear that my aim is to clean this up and refactor some of the existing code to make it more clean and generic.

At the moment I would simply like to get an idea whether you would be interested in this contribution at all, so I know if I should spend any more time on this.

Thank you.

bendahl commented 6 years ago

Hi sashko,

I really appreciate the effort! Thanks for the contribution! There are a few things that I'm currently pondering, however:

  1. What can be done with the virtual touch screen that cannot be done using the virtual touch pad? Using the touch pad one can move the cursor on the screen to any position, right/left click it and simulate a longer press on a certain area. It would be great if you could shed some light on this. I haven't yet investigated the possibilities of touch screens in uinput, so if you could clarify this point, that would be awesome. As far as I understand the API of the current device, it will let you touch (or click, for that matter) on a certain point on the screen. Don't get me wrong I like the idea of supporting all possible devices that can be created using uinput, but it would be bad from a usability perspective to introduce a new device type that doesn't add any new functionality. This might simply confuse potential users of the lib.
  2. Moving the constants to uinputdefs might be a good idea, as it would mean that all constants are in one place. I initially moved them out of this file, since the idea was that only the exported definitions (the ones that users of this lib will be using in their code) should stay in uinputdefs and all the internal definitions should go in the wrapper, as they should not concern the user and only be used in the low level syscall wrapper. I'm still not 100% sure which option would be better.
  3. It would be preferable not to use any hex numbers or other numeric values directly in uinput, as it is not immediately clear what their purpose is. A few constants would be better for readability (see comments in review).
  4. A simple test would also be great, since it does at least provide a quick check to see if all syscalls go through as expected and (more importantly - as the basic uinput stuff is already tested) it serves as an additional documentation for developers using this lib.

I think this sums up the questions that I've got so far. Let me know what your thoughts are, so we can discuss.

Cheers, Ben

sashko commented 6 years ago

Hi Benjamin,

Thank you for the comment. I do not have any issues discussing technical issues as long as it is a productive and respectful discussion.

  1. I could mirror your question by asking: what can be done by keyboard, mouse and touchpad that cannot be done using a touchscreen with a virtual keyboard? Makes no sense, right? But is probably true (dropping numerous details of course). Touchscreen and touchpads in the real world are different devices that serve different functions and operate differently, e.g touchscreen usually serves as a screen (with appropriate dimensions) and a touchpad is just another pointing device. On many platforms, e.g. Android UI widgets respond differently to touch and click.

  2. I like clean code, but see this rather as a personal preference than an issue.

  3. As I wrote above, this is to be cleaned up.

  4. As I wrote above, the aim is to just "get an idea whether you would be interested in this contribution at all". This code should, of course, be covered by unit tests.

bendahl commented 6 years ago

Hi sashko,

absolutely. I think you got me wrong here. My first question really wasn't about the difference of real world devices, nor was the intention to spur any kind of dispute. I did not get the exact intention of this device with regards to the uinput lib. I am well aware that we're talking about different devices that behave differently from a physical perspective. What I wasn't aware of was the fact that, as you mentioned, the Android UI reacts differently to a click vs a touch event, as I assumed this was virtually the same thing. That's exactly what my question was all about ("What can be done with a virtual touch screen that cannot be done with a virtual touch pad?"). I guess the question just came across wrong.

Regarding your second point, to a certain extent this surely is personal preference, but it also makes things easier to reason about. If I see this code for the first time and have never done any work on the lib, it's easier to grasp a named constant than a numeric value, just like a good variable or parameter name can make all the difference in other places.

I am interested in any contribution and value the time spent on this code very much. It would be great to get some additional functionality in here. So go ahead! I canceled the review for now, as you wanted to continue with the implementation and do some cleanup afterwards, correct? For the sake of readability, please do make use of constants instead of straight numeric values though, as it really helps to understand what kind of events you're issuing. That would be appreciated.

Anyway, I hope this clarifies things.

Cheers, Ben

sashko commented 6 years ago

Hi,

I am not exactly sure what you expect me to do from now on. What is the minimum contribution you'd accept? I prefer to do this in few steps, so I don't end up spending 6 hours on implementing and testing the whole functionality and getting my patch rejected.

bendahl commented 6 years ago

Hi sashko,

there are currently three minimum requirements from my perspective:

  1. Add constants (preferably to uinputdefs, as the rest of the contants reside there, too) for the numeric values used in the sendEvent calls and use those instead of numeric values (hex and others). The reasoning being that this will help readers of the code to understand what this code does. I added a comment to the review.
  2. Fix the error message in sendEvent. It currently mentions a rel event, but as I see it, only abs events are processed in the sendEvent function (see comment).
  3. Provide a (short) test. Basically, the pattern would be the same as for the other, already existing tests, so it shouldn't be a lot of effort to provide this for the TouchPad and its Touch function.

Please integrate those three changes , so I can merge the patch.

Cheers, Ben