Created attachment 319557 [details]
a python test program to set gdoups and levels

The attached program is how I imagine the groups and levels could be set from python, but it does not work even with my change proposed above.

BTW I test the changes with this command line:

LD_LIBRARY_PATH=$PWD/../libgnomekbd/build/lib/ \
GI_TYPELIB_PATH=$PWD/../libgnomekbd/build/lib/girepository-1.0/ \
  ./gkbd_test_set_groups_levels.py

where build/ is a subdirectory containing a local installation of the library which includes the modifications.

Thanks,
   Antonio

Created attachment 322274 [details]
a python test program to set groups and levels

An updated python test program which sets the group and levels to 1,1.

Along with the following change to libgnomekbd the example program shows that the C code receives the actual structs where it would expect a pointer to the struct:

  diff --git a/libgnomekbd/gkbd-keyboard-drawing.c b/libgnomekbd/gkbd-keyboard-drawing.c
  index 8cb55d3..9e74a98 100644
  --- a/libgnomekbd/gkbd-keyboard-drawing.c
  +++ b/libgnomekbd/gkbd-keyboard-drawing.c
  @@ -1016,6 +1016,8 @@ draw_key_label (GkbdKeyboardDrawingRenderContext * context,
               glp < GKBD_KEYBOARD_DRAWING_POS_TOTAL; glp++) {
                  if (drawing->groupLevels[glp] == NULL)
                          continue;
  +               fprintf(stderr, "drawing->groupLevels[glp] should be a pointer, instead it's the struct itself\n");
  +               fprintf(stderr, "%016p\n", drawing->groupLevels[glp]);
                  g = drawing->groupLevels[glp]->group;
                  l = drawing->groupLevels[glp]->level;

  @@ -2416,6 +2418,11 @@ gkbd_keyboard_drawing_set_track_config (GkbdKeyboardDrawing * drawing,
                  drawing->track_config = 0;
   }

  +/**
  + * gkbd_keyboard_drawing_set_groups_levels:
  + * @kbdrawing: a #GkbdKeyboardDrawing
  + * @groupLevels: (array fixed-size=4) (transfer container): a list of #GkbdKeyboardDrawingGroupLevel
  + */
   void
   gkbd_keyboard_drawing_set_groups_levels (GkbdKeyboardDrawing * drawing,
                                           GkbdKeyboardDrawingGroupLevel *


In the output we get:

  drawing->groupLevels[glp] should be a pointer, instead it's the struct itself
  0x00000100000001

So maybe there is something going on in the pyhton side of gobject-introspection?

Created attachment 322275 [details]
a javascript example which seems to work

Attaching a javascript example which instead seems to work as intended, when run this way after adding the annotations to libgnomekbd:

LD_LIBRARY_PATH=$PWD/../libgnomekbd/build/lib/ \
GI_TYPELIB_PATH=$PWD/../libgnomekbd/build/lib/girepository-1.0/ \
  gjs gkbd_test_set_groups_levels.js

In this case the C code receives pointers to structs.

So calling set_group_levels from gjs seems to work fine, but in all the other languages I tried (python, perl, ruby) it does not.

Can it be gobject introspection that is not mapping an array of pointers to struct correctly in these languages, while it is doing the right thing for javascript?

Adding the gobject-introspection maintainers to eventually help out.

Created attachment 363501 [details] [review]
Fix calling gkbd_keyboard_drawing_set_groups_levels from other languages

So I looked at this issue again.

It looks like g-ir-scanner (1.54.1-3 from Debian unstable) cannot recognize that the argument is an array of pointer to structs.

In:

void
gkbd_keyboard_drawing_set_groups_levels (GkbdKeyboardDrawing * drawing,
					 GkbdKeyboardDrawingGroupLevel *
					 groupLevels[])

The second parameter gets represented as:

          <parameter name="groupLevels" transfer-ownership="none">
            <type name="KeyboardDrawingGroupLevel"
                  c:type="GkbdKeyboardDrawingGroupLevel*"/>
          </parameter>

Passing annotations does not seem to be enough either:

/**
 * gkbd_keyboard_drawing_set_groups_levels:
 * @groupLevels: (array) (transfer container) (type GkbdKeyboardDrawingGroupLevel**) (element-type GkbdKeyboardDrawingGroupLevel *)
 */
void
gkbd_keyboard_drawing_set_groups_levels (GkbdKeyboardDrawing * drawing,
					 GkbdKeyboardDrawingGroupLevel *
					 groupLevels[])
Results in:

          <parameter name="groupLevels" transfer-ownership="container">
            <array zero-terminated="0" c:type="GkbdKeyboardDrawingGroupLevel*">
              <type name="KeyboardDrawingGroupLevel"/>
            </array>
          </parameter>

Only by changing the function signature I was able to make the gir definition looks as expected and call the method reliably from other languages:

/**
 * gkbd_keyboard_drawing_set_groups_levels:
 * @groupLevels: (array) (transfer container)
 */
void
gkbd_keyboard_drawing_set_groups_levels (GkbdKeyboardDrawing * drawing,
					 GkbdKeyboardDrawingGroupLevel **
					 groupLevels)

which gives:

          <parameter name="groupLevels" transfer-ownership="container">
            <array zero-terminated="0"
                   c:type="GkbdKeyboardDrawingGroupLevel**">
              <type name="KeyboardDrawingGroupLevel"
                    c:type="GkbdKeyboardDrawingGroupLevel*"/>
            </array>
          </parameter>

I still have to make sure that the array elements are in scope when used and that they are in contiguous memory but for a small array this is not asking too much.

If changing the signature is the only way to fix the issue I would take the chance to use a fixed array of four elements for goupLevels as that's what the code assumes in many places anyway.

Any comments? I consider the attached patch still RFC.

Ciao,
   Antonio

Note that the second example produces the same result even after fixing the typo (there was a space before the asterisk):

(element-type GkbdKeyboardDrawingGroupLevel*)

gobject-introspection doesn't do anything by itself; it just provides an API to dynamically load a shared library, and and API to call symbols into it. The responsibility for marshalling data to and from the C API is left to the language binding calling libgirepository's API.

Assuming that the annotations are correct, if gjs works and pygobject doesn't, then it's likely a pygobject bug.

The annotation:

(array) (transfer container) (type GkbdKeyboardDrawingGroupLevel**) (element-type GkbdKeyboardDrawingGroupLevel *)

is incorrect. The `type` and `element-type` annotations are unnecessary, but GkbdKeyboardDrawingGroupLevel must be a known GType so that language bindings can correctly know how to marshal it.

The (transfer container) annotation is puzzling: does the C API take ownership of the C array but not of its data?

Since the C API expects an array of pointers, then:

  (array)

should be enough. The C API does not provide a length for the C array, so the array must be either NULL-terminated:

  (array null-terminated=1)

 or must be of a fixed size N:

  (array fixed-size=N)

(In reply to Emmanuele Bassi (:ebassi) from comment #7)
> gobject-introspection doesn't do anything by itself; it just provides an API
> to dynamically load a shared library, and and API to call symbols into it.
> The responsibility for marshalling data to and from the C API is left to the
> language binding calling libgirepository's API.
>
> Assuming that the annotations are correct, if gjs works and pygobject
> doesn't, then it's likely a pygobject bug.
>

gjs was the only binding I tried which worked without changing the function signature, so I concluded this was because of some "memory bingo": it worked even if the gir file didn't seem to represent the function correctly.

> The annotation:
> 
> (array) (transfer container) (type GkbdKeyboardDrawingGroupLevel**)
> (element-type GkbdKeyboardDrawingGroupLevel *)
> 
> is incorrect. The `type` and `element-type` annotations are unnecessary, but
> GkbdKeyboardDrawingGroupLevel must be a known GType so that language
> bindings can correctly know how to marshal it.
>

An what about plain C types?

GkbdKeyboardDrawingGroupLevel is a C structure with scalar fields, and it looks like it is recognized by g-ir-scanner just fine.

The problems seems to be in the declaration of an array of pointers to it: GkbdKeyboardDrawingGroupLevel *groupLevels[]
Like if the square brackets are not recognized by g-ir-scanner in this case.

That's why I (blindly) tried to overcome this with the "type" and "element-type" annotations. But this opened _another_ problem: the annotations were ignored even if they were there.

But let's not insist on this point, I think the main issue is: having the function signature correctly represented in the gir file.  

As soon as the function signature uses GkbdKeyboardDrawingGroupLevel** instead of  GkbdKeyboardDrawingGroupLevel*[] things seem to start working.

> The (transfer container) annotation is puzzling: does the C API take
> ownership of the C array but not of its data?
>

I'd say so, yes; since it's an array of pointers to structs, I think it makes sense (despite the ugliness of the API) that the array belongs to the function but the elements do not.

> Since the C API expects an array of pointers, then:
> 
>   (array)
> 
> should be enough. The C API does not provide a length for the C array, so
> the array must be either NULL-terminated:
> 
>   (array null-terminated=1)
> 
>  or must be of a fixed size N:
> 
>   (array fixed-size=N)

I agree, and (array fixed-size=4) will work in this case.

The main question is:
Is the current syntax with square brackets supposed to work with g-ir-scanner? Or should the function signature be changed?

I double checked and:
  - using only "(array fixed-size=4)" does not work anywhere

  - using "(array fixed-size=4) (transfer container)" works in gjs (even though the gir file looks still incorrect) but not in the other languages 

  - using "(array fixed-size=4) (transfer container)" and using GkbdKeyboardDrawingGroupLevel** instead of GkbdKeyboardDrawingGroupLevel*[] in the function signature works for gjs and python, but not in perl (but that may be because I don't really know perl...)

GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/libgnomekbd/-/issues/

Thank you for your understanding and your help.