After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 625662 - Missing functions in the X11 bindings
Missing functions in the X11 bindings
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: Bindings
0.9.x
Other Linux
: Normal normal
: ---
Assigned To: Vala maintainers
Vala maintainers
Depends on:
Blocks:
 
 
Reported: 2010-07-30 14:48 UTC by Olivier Tilloy
Modified: 2011-04-07 22:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add missing bindings (716 bytes, patch)
2010-07-30 14:48 UTC, Olivier Tilloy
none Details | Review
test program (2.24 KB, text/x-vala)
2010-08-02 12:15 UTC, Olivier Tilloy
  Details
Updated patch (1.01 KB, patch)
2010-08-02 16:44 UTC, Olivier Tilloy
none Details | Review
Updates to X11 vapi, against head of git master (15.15 KB, patch)
2010-08-09 13:52 UTC, Joshua Simmons
none Details | Review

Description Olivier Tilloy 2010-07-30 14:48:34 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.
Comment 1 Luca Bruno 2010-07-31 18:52:25 UTC
What if XCloseDisplay will be used for freeing automatically the Display?
Comment 2 Olivier Tilloy 2010-07-31 19:23:50 UTC
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.
Comment 3 zarevucky.jiri 2010-08-01 08:37:38 UTC
It's as simple as:
[CCode (cname = "Display", free_function = "XCloseDisplay")]
Comment 4 Olivier Tilloy 2010-08-02 12:13:57 UTC
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?
Comment 5 Olivier Tilloy 2010-08-02 12:15:18 UTC
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).
Comment 6 zarevucky.jiri 2010-08-02 16:06:17 UTC
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.
Comment 7 Olivier Tilloy 2010-08-02 16:44:38 UTC
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.
Comment 8 Joshua Simmons 2010-08-09 13:52:01 UTC
Created attachment 167428 [details] [review]
Updates to X11 vapi, against head of git master
Comment 9 Joshua Simmons 2010-08-09 13:54:22 UTC
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.
Comment 10 Olivier Tilloy 2010-08-26 13:23:46 UTC
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?
Comment 11 Luca Bruno 2011-04-07 20:12:07 UTC
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.
Comment 12 Joshua Simmons 2011-04-07 22:46:03 UTC
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. :)