Bug 44581

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?

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.

(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.

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.

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.

Created attachment 55298 [details] [review]
[4/6] Add Xkl prefix to callback types

Created attachment 55299 [details] [review]
[5/6] Add setters for XklConfigRec string arrays

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']

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.

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...

(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.

> 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.

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).

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.

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!

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.

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

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

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.

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!

Good! Pushed through!

Thanks (and for blogging as well:)

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!

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.

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.

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.

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!

Oops, I just released 5.2 :( I think that patch is not critical. Applied it anyway. Thanks!