Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xinput: implemented XIChangeHierarchy #57

Closed

Conversation

@fkohlgrueber
Copy link

@fkohlgrueber fkohlgrueber commented Aug 11, 2016

Hi,

First of all, thanks very much for this library and all the work you have invested in this project!

I'm getting more into xinput at the moment and I saw that the implementation of this extension isn't complete in python-xlib. I saw the chance to start contributing and so I implemented xlib's XIChangeHierarchy function in xinput.py ;)

New functions:

dpy.xinput_add_master(name, send_core=True, enable=True)
dpy.xinput_remove_master(deviceid, return_mode=2, return_pointer=0, return_keyboard=0)
dpy.xinput_attach_slave(deviceid, new_master)
dpy.xinput_detach_slave(deviceid)

It's also possible to send multiple changes in a single request:

from Xlib.ext.xinput import ChangeHierarchyContext

with ChangeHierarchyContext(dpy) as c:
    c.add_master(...)
    c.remove_master(...)
    c.attach_slave(...)
    c.detach_slave(...)

You can use the xinput command line tool to see the device hierarchy and verify changes made by the above functions.

Unfortunately, I had to modify rq.String8 because of an issue with the X Server expecting an zero-terminated string for AddMaster. The padding implementation didn't insert a termination character when entering a string that doesn't need padding (e.g. "test") which caused the X Server to crash. I made a minimal change (pad=2) to fix that. This change doesn't affect existing code, but I'm not sure if this problem can also occur with other functions.

Ok, that's it. What do you think?

@codecov-io
Copy link

@codecov-io codecov-io commented Aug 11, 2016

Current coverage is 80.96% (diff: 59.18%)

Merging #57 into master will decrease coverage by 0.24%

@@             master        #57   diff @@
==========================================
  Files            39         39          
  Lines          4396       4445    +49   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3570       3599    +29   
- Misses          826        846    +20   
  Partials          0          0          

Powered by Codecov. Last update eb3f4b7...02b217d

Loading

@vasily-v-ryabov
Copy link
Collaborator

@vasily-v-ryabov vasily-v-ryabov commented Aug 12, 2016

Good. I'm just thinking about separate parameter for String8 constructor: String8('name', null_term=True) that is more readable. @benoit-pierre ?

Loading

@benoit-pierre
Copy link
Member

@benoit-pierre benoit-pierre commented Aug 13, 2016

I honestly would prefer to wait for my changes for generating the extensions code, to avoid non-backward compatible API changes.

Regarding this particular implementation, I'm against changing the protocol for a bug in a version of the X11 server: the code works fine with name="test" on my machine without the rq.String8('name', pad=2) hack (Arch Linux, xorg-server 1.18.4-1).

Loading

@fkohlgrueber
Copy link
Author

@fkohlgrueber fkohlgrueber commented Aug 13, 2016

Ok, I just tried the sample again and it also worked on my machine when using "test". I also tried earlier versions of the code and found out that the crash was in fact caused by something else that I fixed in a later version. I'm sorry for the confusion. I will roll back the changes I did in rq.py in the next commit.

@benoit-pierre what do you mean by "generating the extensions code"? I'm just curious ;)

Loading

@benoit-pierre
Copy link
Member

@benoit-pierre benoit-pierre commented Aug 15, 2016

Generating the extension code: using the xcb-proto definitions and xcbgen.

Loading

self.events.append((DetachSlave, deviceid))

def __exit__(self, *exc):
_change_hierarchy(self.display, *self.events)
Copy link
Member

@benoit-pierre benoit-pierre Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be called if a exception did occur:

diff --git i/Xlib/ext/xinput.py w/Xlib/ext/xinput.py
index 348f19f..fc2efdb 100644
--- i/Xlib/ext/xinput.py
+++ w/Xlib/ext/xinput.py
@@ -752,8 +752,9 @@ class ChangeHierarchyContext(object):
     def detach_slave(self, deviceid):
         self.events.append((DetachSlave, deviceid))

-    def __exit__(self, *exc):
-        _change_hierarchy(self.display, *self.events)
+    def __exit__(self, *exc_info):
+        if exc_info == (None, None, None):
+            _change_hierarchy(self.display, *self.events)


 def init(disp, info):

Loading

@drzraf
Copy link

@drzraf drzraf commented Oct 27, 2018

@benoit-pierre could you please provide the method/template/script you used to generate the bindings from xcb-proto XML files in #42?

Loading

@benoit-pierre
Copy link
Member

@benoit-pierre benoit-pierre commented Nov 12, 2018

@drzraf: script here: https://gist.github.com/de5beb2cfb1a60abe78f83619796edea.

I have not updated it in some time, so it won't work with the latest version of xcb-proto, you'll need to use version 1.11:

$ git clone git://anongit.freedesktop.org/git/xcb/proto
$ git -C proto checkout 1.11
$ PYTHONPATH="$PWD/proto" python3 ./gen_xlib.py proto/src/xinput.xml

This will output the generated code on stdout. Note that this might generate some code that is not compatible with the current python-xlib master: I have an (old) branch with a lot of changes to support things like unions, switches, variable length fields with complex length calculation, etc...

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants