Bug 18939 - Useless xcb_lookup_t in xcb_keysyms.h
Summary: Useless xcb_lookup_t in xcb_keysyms.h
Status: RESOLVED FIXED
Alias: None
Product: XCB
Classification: Unclassified
Component: Utils (show other bugs)
Version: unspecified
Hardware: All All
: low trivial
Assignee: xcb mailing list dummy
QA Contact:
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2008-12-07 19:02 UTC by Sean Bartell
Modified: 2008-12-09 00:27 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Remove xcb_lookup_t (566 bytes, patch)
2008-12-07 19:02 UTC, Sean Bartell
Details | Splinter Review

Description Sean Bartell 2008-12-07 19:02:53 UTC
Created attachment 20880 [details] [review]
Remove xcb_lookup_t

The enum xcb_lookup_t in xcb_keysyms.h is not used by any code or documented anywhere. It seems to parallel some Xlib defines (XLookupNone, etc.) that are used by XLookupString, but XCB has no version of that function.

Patch included.
Comment 1 Sean Bartell 2008-12-07 21:15:30 UTC
Even if it can't be removed, it needs to be changed from enum{...}xcb_lookup_t to enum xcb_lookup_t{...} to avoid problems when included from a C++ file.
Comment 2 Julien Danjou 2008-12-08 05:30:19 UTC
I've pushed a patch that use this for the col attribute via a typedef.
Comment 3 Sean Bartell 2008-12-08 06:51:56 UTC
That's not quite right... The col argument is actually passed as 0, 1, 2, or 3 depending on modifiers (see http://tronche.com/gui/x/xlib/input/keyboard-encoding.html and an implementation at http://git.naquadah.org/?p=awesome.git;a=blob;f=keybinding.c;h=d17018d42046ae4d69d61d9e470cadb1633a06f0;hb=HEAD#l180).

I figure someone was trying to figure out the col parameter and thought it was related to those Xlib constants, but it's not, it's used in the Xlib-only function XLookupString. The enum needs to be removed entirely, unless someone wants to add an xcb_lookup_string to go with it:).
Comment 4 Julien Danjou 2008-12-08 07:21:39 UTC
I've renamed it to xcb_key_lookup_t. I don't see why we can't use this name for typedef a parameter in xcb-keysym, even if the word lookup is used. It's not a trademark deposed by Xlib, is it? :)
Comment 5 Sean Bartell 2008-12-08 14:31:06 UTC
The problem is that it isn't actually a parameter for anything in xcb-keysym. The col parameters are supposed to be ints, they correspond to the last parameters of XKeycodeToKeysym and XLookupKeysym and they indicate which column of the keysym table to use.

The xcb_lookup_t/xcb_key_lookup_t is just some dead, useless code originally added by someone who couldn't figure out what the col parameter was for (I don't blame them, I couldn't figure it out for a while either). They just copied over some relevant-looking constants from Xlib in the hope that they would be useful, but the constants were actually for something else (XLookupString).

Sorry if I'm being completely confusing:).
Comment 6 Julien Danjou 2008-12-09 00:27:39 UTC
> The problem is that it isn't actually a parameter for anything in xcb-keysym.

It is now, read my patch. :)

For the rest, I really don't see a problem. xcb-keysyms can do whatever he wants, it's not mean to be a straight portage of Xlib anyway, and it lacks suppport of many stuff anyway.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.