Bug 44581 - Add gobject-introspection support
Add gobject-introspection support
Status: RESOLVED FIXED
Product: libxklavier
Classification: Unclassified
Component: General
unspecified
Other All
: medium enhancement
Assigned To: Martin Pitt
Sergey V. Udaltsov
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-08 08:52 UTC by Martin Pitt
Modified: 2012-01-17 15:07 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
[1/6] Build introspection typelib (7.17 KB, patch)
2012-01-08 09:10 UTC, Martin Pitt
Details | Splinter Review
[2/6] Add GI annotations (6.44 KB, patch)
2012-01-08 09:13 UTC, Martin Pitt
Details | Splinter Review
[3/6] Add xkl_config_item_new_with_data() constructor (2.44 KB, patch)
2012-01-08 09:15 UTC, Martin Pitt
Details | Splinter Review
[4/6] Add Xkl prefix to callback types (11.77 KB, patch)
2012-01-08 09:15 UTC, Martin Pitt
Details | Splinter Review
[5/6] Add setters for XklConfigRec string arrays (3.15 KB, patch)
2012-01-08 09:16 UTC, Martin Pitt
Details | Splinter Review
[6/6] Add Python test script for GI binding (4.01 KB, patch)
2012-01-08 09:18 UTC, Martin Pitt
Details | Splinter Review
[3/6] Add XklConfigItem setters (3.73 KB, patch)
2012-01-08 11:49 UTC, Martin Pitt
Details | Splinter Review
[6/6] Add Python test script for GI binding (4.03 KB, patch)
2012-01-08 11:49 UTC, Martin Pitt
Details | Splinter Review
Mark unintrospectable methods as (skip) (1.24 KB, patch)
2012-01-11 00:54 UTC, Martin Pitt
Details | Splinter Review
Make xkl_engine_get_current_state() introspectable (2.36 KB, patch)
2012-01-11 00:55 UTC, Martin Pitt
Details | Splinter Review
Use local include files for building GIR (1.08 KB, patch)
2012-01-16 07:02 UTC, Martin Pitt
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Pitt 2012-01-08 08:52:29 UTC
There are static xklavier bindings for Python, but as it uses gobject, you can't use it in Python projects which use GI modules such as Gtk or Gobject. Thus it would be nice if xklavier would be introspectable.

I have worked on a set of patches for this and got it almost working (the train ride was half an hour too short :) ). I'm filing this for now to avoid duplicate work by other people. I expect to have it fully working within the next days.
Comment 1 Sergey V. Udaltsov 2012-01-08 09:08:01 UTC
There is one problem here. I would not want libxklavier to become too dependent on G* stack. It already depends on glib/gobject (that was a hard move to make - and it caused some tension when KDE used libxklavier). Introducing introspection would make libxklaver even more G-ish.

So, here is the question - would it be possible to make introspection optional, so people would be able to build libxklavier without it?
Comment 2 Martin Pitt 2012-01-08 09:10:36 UTC
Created attachment 55295 [details] [review]
[1/6] Build introspection typelib

Hah, turned out that there was only a tiny bit missing, it's working quite nicely now.

This patch adds the necessary autoconf magic to build a .gir/.typelib, as per https://live.gnome.org/GObjectIntrospection/AutotoolsIntegration

The library needs some annotation fixes and API extensions to make this actually work, though. I'll send these in separate patches, to make it easier to review the individual ones.
Comment 3 Martin Pitt 2012-01-08 09:11:53 UTC
(In reply to comment #1)
> So, here is the question - would it be possible to make introspection optional,
> so people would be able to build libxklavier without it?

Yes, GI support is optional. configure detects whether gobject-introspection is available, and if not, just skips it. You can also explicitly disable it with --disable-introspection.
Comment 4 Martin Pitt 2012-01-08 09:13:18 UTC
Created attachment 55296 [details] [review]
[2/6] Add GI annotations

This just fixes missing annotations, pretty straightforward. This is also relevant for the gtk-doc documentation.
Comment 5 Martin Pitt 2012-01-08 09:15:26 UTC
Created attachment 55297 [details] [review]
[3/6] Add xkl_config_item_new_with_data() constructor

This and the next two patches add some API to provide methods for record member access, which can't be done through GI (at least not right now).

These might be debatable, I'm happy to discuss these with you. But I tried pretty hard to do without these, I'm fairly convinced that we need them.

The git log descriptions should have all the necessary details.
Comment 6 Martin Pitt 2012-01-08 09:15:57 UTC
Created attachment 55298 [details] [review]
[4/6] Add Xkl prefix to callback types
Comment 7 Martin Pitt 2012-01-08 09:16:43 UTC
Created attachment 55299 [details] [review]
[5/6] Add setters for XklConfigRec string arrays
Comment 8 Martin Pitt 2012-01-08 09:18:51 UTC
Created attachment 55300 [details] [review]
[6/6] Add Python test script for GI binding

And finally, this adds a tests/test_gi.py which demonstrates reading various objects (Engine, ConfigItem, ConfigRec, ConfigRegistry), appends a new layout, and removes it again:

$ LD_LIBRARY_PATH=libxklavier/.libs/ tests/test_gi.py == Engine ==
indicator names: ['Caps Lock', 'Num Lock', 'Scroll Lock', 'Compose', 'Kana', 'Sleep', 'Suspend', 'Mute', 'Misc', 'Mail', 'Charging', 'Shift Lock', 'Group 2', 'Mouse Keys', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '', '']
group names: ['English (US)', 'German (eliminate dead keys)']
default layout: German (eliminate dead keys)
features: 2F

== Available Layouts ==
[us] English (US), [ad] Catalan, [af] Afghani, [ara] Arabic, [al] Albanian, [am] Armenian, [at] German (Austria), [az] Azerbaijani, [by] Belarusian, [be] Belgian, [bd] Bengali, [in] Indian, [ba] Bosnian, [br] Portuguese (Brazil), [bg] Bulgarian, [ma] Arabic (Morocco), [cm] English (Cameroon), [mm] Burmese, [ca] French (Canada), [cd] French (Democratic Republic of the Congo), [cn] Chinese, [hr] Croatian, [cz] Czech, [dk] Danish, [nl] Dutch, [bt] Dzongkha, [ee] Estonian, [ir] Persian, [iq] Iraqi, [fo] Faroese, [fi] Finnish, [fr] French, [gh] English (Ghana), [gn] French (Guinea), [ge] Georgian, [de] German, [gr] Greek, [hu] Hungarian, [is] Icelandic, [il] Hebrew, [it] Italian, [jp] Japanese, [kg] Kyrgyz, [kh] Khmer (Cambodia), [kz] Kazakh, [la] Lao, [latam] Spanish (Latin American), [lt] Lithuanian, [lv] Latvian, [mao] Maori, [me] Montenegrin, [mk] Macedonian, [mt] Maltese, [mn] Mongolian, [no] Norwegian, [pl] Polish, [pt] Portuguese, [ro] Romanian, [ru] Russian, [rs] Serbian, [si] Slovenian, [sk] Slovak, [es] Spanish, [se] Swedish, [ch] German (Switzerland), [sy] Arabic (Syria), [tj] Tajik, [lk] Sinhala, [th] Thai, [tr] Turkish, [tw] Taiwanese, [ua] Ukrainian, [gb] English (UK), [uz] Uzbek, [vn] Vietnamese, [kr] Korean, [nec_vndr/jp] Japanese (PC-98xx Series), [ie] Irish, [pk] Urdu (Pakistan), [mv] Dhivehi, [za] English (South Africa), [epo] Esperanto, [np] Nepali, [ng] English (Nigeria), [et] Amharic, [sn] Wolof, [brai] Braille, [tm] Turkmen, [ml] Bambara, [tz] Swahili (Tanzania), [ke] Swahili (Kenya), [bw] Tswana, [ph] Filipino, 

== ConfigRec ==
Curent configuration:
  Model: pc105
  Layouts: ['us', 'de']
  Variants: ['', 'nodeadkeys']
  Options: ['terminate:ctrl_alt_bksp', 'grp:shifts_toggle']
Adding Danish layout...
Curent configuration:
  Model: pc105
  Layouts: ['us', 'de', 'dk']
  Variants: ['', 'nodeadkeys', '']
  Options: ['terminate:ctrl_alt_bksp', 'grp:shifts_toggle']
Removing Danish layout...
Curent configuration:
  Model: pc105
  Layouts: ['us', 'de']
  Variants: ['', 'nodeadkeys']
  Options: ['terminate:ctrl_alt_bksp', 'grp:shifts_toggle']
Comment 9 Martin Pitt 2012-01-08 09:30:34 UTC
Note that there are still a bunch of warnings from g-ir-scanner about constants without an XKL_* prefix, but I think they are relatively uninteresting to use from bindings.

There are still a few unintrospectable symbols which are harder to fix:

   xklavier.h:97: Warning: Xkl: xkl_set_log_appender: argument fun: Missing (scope) annotation for callback without GDestroyNotify (valid: call, async)

The actual scope of that argument would be "(forever)", or "until you call that function again", but that doesn't exist right now. A clean solution would be to provide a GDestroyNotify, but that would change API. I don't need that part of the API myself, so I'm not particularly interested in it, but if you are, I can work on this, too (perhaps with skipping this one and adding a new function with a GDestroyNotify).

  xkl_engine.h:329: Warning: Xkl: xkl_engine_get_current_state: return value: Invalid non-constant return of bare structure or union; register as boxed type or (skip)

Error message speaks for itself. Interpreters need to know how to copy/create/free objects, which you can't reliably do with bare structs.

  default_log_appender() (because GI cannot support varargs)

But as the test script shows, the majority of the API can be used, so I think it's "good enough" for now.
Comment 10 Sergey V. Udaltsov 2012-01-08 09:36:40 UTC
Ok, some comments:
1. You did not change VERSION_INFO in configure.ac. I guess it was just overlooked, right? The API is changed, even though in the compatible way.
2. The xkl_config_item_new_with_data - I do not have strong objection, but why are just plain setters/getters not sufficient?
3. Why is that in Makefile.am?
-xklavier_headers = xklavier.h xkl_config_registry.h xkl_engine.h \
-	xkl_config_rec.h xkl_config_item.h xkl_engine_marshal.h
+xklavier_headers = xkl_engine.h xkl_config_item.h xkl_config_registry.h \
+	xkl_config_rec.h xkl_engine_marshal.h xklavier.h
4. set_log_appender is mostly for debugging, so for now let's ignore it
5. I will check around get_current_state. Perhaps making it const would be affordable...
Comment 11 Martin Pitt 2012-01-08 11:20:36 UTC
(In reply to comment #10)
> Ok, some comments:
> 1. You did not change VERSION_INFO in configure.ac. I guess it was just
> overlooked, right? The API is changed, even though in the compatible way.

Right, sorry, I overlooked it. I should be bumped for the patches 2 to 5.

> 2. The xkl_config_item_new_with_data - I do not have strong objection, but why
> are just plain setters/getters not sufficient?

They would be sufficient. If you prefer individual setters, I can change the patches accordingly.

> 3. Why is that in Makefile.am?
> -xklavier_headers = xklavier.h xkl_config_registry.h xkl_engine.h \
> -    xkl_config_rec.h xkl_config_item.h xkl_engine_marshal.h
> +xklavier_headers = xkl_engine.h xkl_config_item.h xkl_config_registry.h \
> +    xkl_config_rec.h xkl_engine_marshal.h xklavier.h

With the original order, g-ir-scanner failed on some unknown symbols. It seems to have some left-to-right ordering. That's not very scientific, I know, I'm not a guru for the g-i implementation.

> 4. set_log_appender is mostly for debugging, so for now let's ignore it

Agreed.

> 5. I will check around get_current_state. Perhaps making it const would be
> affordable...

I'm not sure that const will help, as even with const the interpreter might still need to copy the value. Boxing the struct (i. e. providing a proper constructor and _copy() method) should help. Again, I wasn't terribly concerned about this particular method; once you are happy with the current patches and they can land, we can do the finishing touches.
Comment 12 Sergey V. Udaltsov 2012-01-08 11:29:48 UTC
> Right, sorry, I overlooked it. I should be bumped for the patches 2 to 5.
Would you update the patches?

> They would be sufficient. If you prefer individual setters, I can change the
> patches accordingly.
Let's do it with setters/getters, for flexibility sake.

> With the original order, g-ir-scanner failed on some unknown symbols. It seems
> to have some left-to-right ordering. That's not very scientific, I know, I'm
> not a guru for the g-i implementation.
Ok, let's forget about scientific approach here:)

> I'm not sure that const will help, as even with const the interpreter might
> still need to copy the value. Boxing the struct (i. e. providing a proper
> constructor and _copy() method) should help. Again, I wasn't terribly concerned
> about this particular method; once you are happy with the current patches and
> they can land, we can do the finishing touches.
Agreed.
Comment 13 Martin Pitt 2012-01-08 11:49:14 UTC
Created attachment 55308 [details] [review]
[3/6] Add XklConfigItem setters

This is now using setters instead of the new constructor. It's also more consistent with patch 5, and indeed more future proof.

This also bumps VERSION_INFO. (No need to do it again for the other two patches, assuming that all these patches go in at the same time without a release in between).
Comment 14 Martin Pitt 2012-01-08 11:49:55 UTC
Created attachment 55309 [details] [review]
[6/6] Add Python test script for GI binding

This updates the test script to use the setter instead of the new_with_data constructor.
Comment 15 Martin Pitt 2012-01-09 12:51:09 UTC
Are you generally happy about these now? If so, I can start working on e. g. the boxing for making get_current_state() introspectable, but I would not want to waste work on this if there's still something you'd like to see fixed in the basics.

Thanks in advance!
Comment 16 Sergey V. Udaltsov 2012-01-10 15:34:20 UTC
Thanks, that is all now pushed. The only thing is that version info is changed to 18:0:2. I guess your change was not really correct.
Comment 17 Martin Pitt 2012-01-10 23:06:31 UTC
Thanks for pushing!

I have a question about the VERSION_INFO bump: As far as I know I did not break any existing API. If I did, I'd like to rectify this, I didn't mean to. That's why I only bumped the revision (as this adds some new API) and age (as it's backwards compatible), but not current (as the new library will work fine with programs linked to .so.17). So what broke the ABI in my patches that required this bump?

Thanks, Martin
Comment 18 Martin Pitt 2012-01-10 23:08:20 UTC
Oh, _if_ you need to bump "current", age needs to be reset to 0, as in this case the library is not backwards compatible to any previous ABI.

Martin
Comment 19 Martin Pitt 2012-01-11 00:54:33 UTC
Created attachment 55408 [details] [review]
Mark unintrospectable methods as (skip)

I have two more patches to provide some fine tuning.

This one just marks unintrospectable methods as (skip) explicitly.
Comment 20 Martin Pitt 2012-01-11 00:55:44 UTC
Created attachment 55409 [details] [review]
Make xkl_engine_get_current_state() introspectable

This boxes XklState and fixes the return transfer annotation. With this, xkl_engine_get_current_state() is now also introspectable.

Thanks for considering!
Comment 21 Sergey V. Udaltsov 2012-01-11 14:13:44 UTC
Good! Pushed through!

Thanks (and for blogging as well:)
Comment 22 Martin Pitt 2012-01-11 23:00:34 UTC
Thanks! Can you please fix VERSION_INFO in trunk, too? (See comment 17). I. e. either revert back to 17, or if we need a SONAME bump, drop age to 0?

Thanks!
Comment 23 Sergey V. Udaltsov 2012-01-13 13:13:55 UTC
Look at the new .so name. It is now 16.2.0. It was 16.1.0. Exactly because you did not break the ABI, you only extended it. The crazy thing is that VERSION_INFO does not directly match the so number.
Comment 24 Martin Pitt 2012-01-15 01:35:27 UTC
Ah, indeed. I keep getting dazzled by VERSION_INFO, sorry. Thanks for pointing it out and fixing!

Do you plan to do a new release with this soon? Otherwise I'll just add the patches to our packages.
Comment 25 Martin Pitt 2012-01-16 06:57:48 UTC
I am sorry, seems I missed one thing: When the development files of libxklavier are not installed in the system, build fails with

  GISCAN Xkl-1.0.gir
In file included from <stdin>:6:0:
/home/martin/upstream/libxklavier/libxklavier/xkl_config_registry.h:24:36: fatal error: libxklavier/xkl_engine.h: No such file or directory

It should use the local include, not the system one.
Comment 26 Martin Pitt 2012-01-16 07:02:02 UTC
Created attachment 55638 [details] [review]
Use local include files for building GIR

This fixes the build by preferring includes from the local tree.

Thanks and sorry for the oversight!
Comment 27 Sergey V. Udaltsov 2012-01-17 15:07:22 UTC
Oops, I just released 5.2 :( I think that patch is not critical. Applied it anyway. Thanks!