GNOME Bugzilla – Bug 625662
Missing functions in the X11 bindings
Last modified: 2011-04-07 22:46:03 UTC
Created attachment 166839 [details] [review] Add missing bindings Originally reported in Ubuntu (https://bugs.launchpad.net/ubuntu/+source/vala/+bug/611729). Also valid in git HEAD. The X11 binding (x11.vapi) misses some functions, including (but maybe not limited to) XCloseDisplay and XMapRaised. The (trivial) attached patch binds these two functions. They have been successfully tested in a vala program.
What if XCloseDisplay will be used for freeing automatically the Display?
XOpenDisplay being mapped to the X.Display constructor, it would make sense to have XCloseDisplay mapped to the destructor. My experience and knowledge of Vala are very limited though, I'm not sure how to go about implementing that.
It's as simple as: [CCode (cname = "Display", free_function = "XCloseDisplay")]
Jiří, I tested your suggestion (with the latest development version of vala from git), and it doesn't seem to work as expected. However, the following does work: [CCode (cname = "Display", unref_function = "XCloseDisplay")] Is that correct?
Created attachment 166966 [details] test program For reference, I'm attaching my test program, which purpose is to achieve what `wmctrl -a chromium` does (expect for the desktop switch).
Actually, for me, your annotation does not work. unref_function only works in conjunction with ref_function. If you keep ref_function an empty string (as it was before?) then it will seem to work, but it will fail as soon as you assign the value of display into another variable. Vala will then, instead of issuing a proper error, "adds reference" by simple assignment and then "removes references" by closing the display twice. I think that is not what you want it to do. free_function really should work, just make sure to remove the superfluous ref_function and unref_function values.
Created attachment 166997 [details] [review] Updated patch You're absolutely right, I had forgotten to remove the empty ref_function and unref_function. Your solution works as expected. I'm attaching an updated patch, generated against git master.
Created attachment 167428 [details] [review] Updates to X11 vapi, against head of git master
Oh I was supposed to add comments there! Here they are anyway. So I took your patch and added some stuff. I did a few things: * Renamed the enumerations so they follow the vala naming conventions. * Removed the constant Success and replaced with the full set of error codes under the enumeration ErrorCode. * Moved the create_window function from namespace scope to a Display class member. * Renamed Screen.xxx_of_screen() methods to Screen.get_xxx() * Removed member variables from the Screen class, according to the XLib docs they should never be set manually and there are get_xxx methods for all the existing ones anyway. * Fixed the handling of the Visual struct. It's just a typedef of ID as with other handles like Pixmap and Window. * Fixed Screen.get_default_visual, XDefaultVisualOfScreen returns a pointer to a Visual, not a visual itself. The only way I could get this to actually work was to return a pointer from the get_default_visual function as well. Not sure if this is the best option.
Is anyone reviewing Joshua’s patch? Just asking, I’m not experienced enough myself (neither in X11 nor in Vala). It looks to me like valuable work that would benefit everyone. On the other hand it’s clearly a lot to review, it may be worth reviewing and merging my original unobtrusive patch in a first step?
commit b151d488d95f14f0c36181e9c5eae61df9283dde Author: Luca Bruno <lucabru@src.gnome.org> Date: Thu Apr 7 22:00:43 2011 +0200 x11: Use XCloseDisplay free function for Display Fixes bug 625662. commit 106c3313e7145e795dab06954b32493159b03e68 Author: Joshua Simmons <simmons.44@gmail.com> Date: Thu Apr 7 21:57:23 2011 +0200 x11: Add missing XMapRaised function, ErrorCode and WindowClass enums Partially fixes bug 625662. I split the patch in two commits for better bisecting future troubles against the XCloseDisplay free functions. I've cloned this bug to bug 647095 due to methods/fields renaming that do not belong to this bug. Thanks for the contribution.
Yeah sorry about that, a bit of a dump and run there. I think most of it is sane though. Thanks for taking the time to review. :)