GNOME Bugzilla – Bug 720225
XCB vapi file lacks a lot of basic types and methods
Last modified: 2018-05-22 15:01:56 UTC
Created attachment 263954 [details] Added several methods and types The XCB vapi file is extremely incomplete, to the point that is useless. I attach a new file much more complete (but still lacks some functions).
Can you please attach a diff instead of a new file? Thanks.
Created attachment 264006 [details] [review] A diff with new methods and definitions for XCB.VAPI Of course, here it is. This patch contains the new code and fixes needed to be able to create a bare minimal window manager in Vala.
Thanks. Why did you need to put values in EventMask? Why did you remove connect, create_window and map_window from the namespace?
The values in EventMask were needed because they are also put in xproto.h. If you see them, they are not correlative, but powers of two. About removing connect, create_window and map_window, they are removed because they are deprecated and already included in the Connect object, as they should. But, of course, if you considere that is better to keep them, go ahead.
(In reply to comment #4) > The values in EventMask were needed because they are also put in xproto.h. If > you see them, they are not correlative, but powers of two. I don't see the problem in not specifying the values if they are already in xproto.h. Can you explain more in detail? > About removing connect, create_window and map_window, they are removed because > they are deprecated and already included in the Connect object, as they should. > But, of course, if you considere that is better to keep them, go ahead. Deprecated stuff is kept until there are major maintenance troubles or something changes drastically. So yes please, keep them.
Created attachment 264021 [details] [review] New diff with changes asked Sorry, my fault, you are right. The values are not needed (tested with a simple printf). Also recovered the deprecated calls.
Review of attachment 264021 [details] [review]: Apart the line below being truncated (please test the change next time :-) ) the patch seems fine thanks. Do you have commit access? ::: xcb.vapi @@ +634,3 @@ public Connection connect (string? display = null, out int screen = null); [Deprecated (since = "vala-0.14", replacement = "Xcb.Connection.create_window")] + public VoidCookie create_window (Connection connection, uint8 depth, Window wid, Window parent, int16 x, int16 y, uint16 width, uint16 height, uint16 border_width, uint16 _class, VisualID visual, uint This line is truncated.
Ooooops! Next time I will not hurry O:) No, I don't have commit access, sorry.
Created attachment 264046 [details] [review] Final patch (I hope) The patch with the line fixed, and another little extra fix, removing other asignments in enums I forgot to remove.
Sorry, again uploaded a fully file instead of a diff. As soon as I arrive home I'll send the right file.
Created attachment 264097 [details] [review] Final patch This is a patch file with all the changes.
commit aef4702ccc93c8f10c80afd600e3410be304fc02 Author: Sergio Costas <raster@rastersoft.com> Date: Thu Dec 12 20:15:14 2013 +0100 xcb: Major changes and fixes. Fixes bug 720225. This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Created attachment 264216 [details] [review] Added "geometry" functions and started xcb-icccm.vapi Here is a new diff which adds the "geometry" call functions and starts to implement the xcb-icccm part. It must be added after the previous diffs (this is, it is done against the current GIT status, which already contains the previous patched of this thread). If you preffer, I can open a new bug, of course.
Review of attachment 264216 [details] [review]: Thanks, but does xcb-icccm.vapi get installed without modifying Makefile.am?
Created attachment 264233 [details] [review] Added "geometry" functions and started xcb-icccm.vapi (with makefile.am changes) You are right. I don't know what happened with the diff file. This one also has the makefile.am changes needed. Sorry.
Created a different format for ICCCM, more object oriented. I attach the diff.
Created attachment 264504 [details] [review] ICCCM now is more object-oriented Added all the changes in the previous patch, and a new design for XCB-ICCCM more object-oriented. Ready to be applied directly to the current master branch in git repo.
commit fdd6531f13303dc6adb53331f0e698357157aade Author: Sergio Costas <raster@rastersoft.com> Date: Sat Dec 21 10:34:29 2013 +0100 Add xcb-icccm bindings. Fixes bug 720225.
Is there a reason we should dist this with Vala instead of in the vala-extra-vapis repo? The only real argument I can see is that we already distribute xcb with Vala but, given how infrequently these are likely to be used (especially moving forward as people transition to Wayland or Mir), it seems like vala-extra-vapis might be a better home.
I agree with Evan. These days we have been talking about it and he convinced me. I attach a new patch with only the changes to xcb.diff.
Created attachment 264742 [details] [review] Only changes for xcb.vapi, without xcb-icccm.vapi.
BTW: I made a little survey at github, and found only one project writen in Vala that uses xcb.vapi: https://github.com/gandalfn/maia There is also what seems to be an automatic generator from the XML-based API of XCB to VAPI files, https://github.com/gandalfn/xcb-vala , but doesn't use xcb.vapi, and seems stalled. So maybe it would be possible to move the whole xcb vapis to vala-extra-vapis? (BTW, maybe it would be better to move this to a new bug...)
(In reply to comment #22) > So maybe it would be possible to move the whole xcb vapis to vala-extra-vapis? > (BTW, maybe it would be better to move this to a new bug...) Even if there is only one project on github, and even if that is the only published open-source project, we have no way of knowing what else is out there. There could be unpublished projects, proprietary software, etc. We would have to mark the one we distribute with Vala as deprecated and refer people to vala-extra-vapis (a bit like what we do now for librsvg) for a few years before removing it, but TBH I'm not convinced it's worth the effort. Luca, any objection to me moving xcb-icccm over, but keeping the xcb changes?
Yes, my objection is the same as yours: there's no reason to move vapis outside of the vala tree. I'm rather for moving stable vapis inside the vala tree rather than externalizing them. It makes it harder for people to find vapis and to use them just for the sake of having them separate to the vala tree.
Should I close this thread and start a new one to add new patches for XCB, or should I keep it and add here new patches? Also, I have a new patch that, unfortunately, breaks the source compatibility. The reason is that I fixed the use of "ref_function" and "unref_function", replacing them by "free_function" to ensure that the compiler can manage the memory in the proper way. The only change needed in the application source is to add "unowned" where needed, nothing else. Is it a problem?
No, leave it open. I plan to bring this up on the vala-devel mailing list on Monday. I disagree with Luca here, and I don't think either of us will convince the other, so it would be nice to get Jürg's opinion (as well as anyone else who might be interested).
Created attachment 265342 [details] [review] Tries to fix the memory management. This is a new patch that tries to fix the memory management in xcb.vapi. This patch slightly breaks the source compatibility: it would be necessary to add "unowned" in several assignments in the source code of any program that compiles against this VAPI, or the vala compiler will complain.
Review of attachment 265342 [details] [review]: ::: vapi/xcb.vapi @@ +119,3 @@ [CCode (cname = "xcb_get_geometry_cookie_t", has_type_id = false)] public struct GetGeometryCookie { + uint32 sequence; What's the problem with not threating GetGeometryCookie as an uint32? Some alignment/padding fails with the compiler?
All XCB cookies are an struct with a single integer inside, not an integer. Extracted from xproto.h: /** * @brief xcb_get_geometry_cookie_t **/ typedef struct xcb_get_geometry_cookie_t { unsigned int sequence; /**< */ } xcb_get_geometry_cookie_t;
Yes, which can be threaten as an integer. What problem did you have with the previous approach? It didn't work at all?
My reason is to keep in both places (.VAPI and .H files) the same formats. If in the .H is a structure, keep it as a structure in the VAPI. But if you think it is OK to move it to a single integer, I'll do.
It's to avoid breaking existing users. If it works, don't change.
Strictly speaking, it is an opaque value. You never use the "sequence" value. You just launch a question to the X server and store the cookie, returned to you by the "_query" function. Then, when you need to get the server response, you just call the "_reply" function passing the cookie "as is". To the user doesn't matter if it is a struct, an integer, a pointer... Also, the same source code works with both ways.
Does the current version work? If so, don't change.
Yes, the current version works. Ok, I'll keep it the old style.
Created attachment 265425 [details] [review] Reverted the cookies style to the old one Ok, here it is. I reverted the cookies code to the old format, and contains all the other changes in the last patch. This one can be applied in the current GIT master branch.
Review of attachment 265425 [details] [review]: Looks fine as long as it's tested, thanks.
Created attachment 265427 [details] [review] Better definition of Atom This is the same patch, but with an updated definition of Atom.
I changed the definition of Atom. When I wrote it, in the first patch, it was with the [IntegerType...] format.
BTW: I'm working on a compositor/window manager in Vala, that's why I'm working on these updates. Each new function added is because I need it for my code, so I'm testing them O:)
Review of attachment 265427 [details] [review]: I decided to reject my own patch because, after evaluating the use or not of inheritance for typedefs, I think it is really better to keep the Atom definition as in the attach 265425.
Hi again: I decided to reject my own patch 265427. Just use the 265426 as originally planed. Sorry for the inconvenience.
I just moved the xcb bindings to vala-extra-vapis. If you can rebase your patch on that I would be happy to push it. Robert Ancell has been doing a lot of work on xcb, so unfortunately there will probably be some conflicts :(
Review of attachment 265425 [details] [review]: Better revisit it or scratch it completely
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/422.