Python bindings

[Andrew Bird]

On Friday 16 November 2012 15:33:52 Aleksander Morgado wrote:
> >>>>> 1/ client.get_manufacturer() won't take None as the first argument,
> >>>>> but
> >>>>> looking at the gir I can't see why not. As a hack I passed in a
> >>>>> function
> >>>>> reference, knowing that its unused anyway.
> >>>> 
> >>>> Maybe some missing (allow-none) annotation for the 'input' variable in
> >>>> the async call?
> >>>> 
> >>>>> 2/ Where necessary, complex return values should be returned in a
> >>>>> tuple.
> >>>>> If
> >>>>> you look at get_manufacturer_ready() you'll see that supposedly the
> >>>>> value
> >>>>> is returned in the string passed in to arg1. For me that's always
> >>>>> empty,
> >>>>> but I checked with qmicli, the value is populated properly. The page
> >>>>> https://live.gnome.org/PyGObject/IntrospectionPorting suggests that it
> >>>>> should be returning tuples in this case, and its certainly more
> >>>>> pythonic.
> >>>> 
> >>>> Maybe some missing (out) annotation for each variable returned when
> >>>> reading the TLV from the output bundle?
> >>> 
> >>> Hi Aleksander,
> >>> 
> >>>     Given that GetManufacturer is in the generated stuff, where should I
> >>>     apply
> >>> 
> >>> my changes so that they appear in its output?
> >> 
> >> Directly in the code generator sources; under build-aux/qmi-codegen/*.py
> > 
> > Aleksander,
> > 
> >    I've found and made the changes and now get output now. One thing
> >    though
> > 
> > the unused parameter instead of 'allow-none', could be 'skip', which seems
> > nicer. On the other hand I'm not sure if you ever intend to use this
> > parameter?
> 
> That's the main problem. Messages defined without input TLVs need the
> NULL input bundle in the API, just in case some day the message is
> updated with a first TLV added. There are definitely cases where this
> will not happen; e.g. GetManufacturer will very likely never need an
> input TLV. But we cannot tell about *all* messages without input TLVs.
> Hence, I would personally leave it as (allow-none) instead of directly
> (skip).
yep, agreed

> 
> > Below are the patches for discussion:
> > 
> > ###############
> > 
> >>From 35b5bb2916b36c17350c3ded24d34b97d4592b2d Mon Sep 17 00:00:00 2001
> >>
> > From: Andrew Bird <ajb at spheresystems.co.uk>
> > Date: Fri, 16 Nov 2012 11:29:39 +0000
> > Subject: [PATCH 1/2] Add annotation to output strings in the generated
> > output
> > 
> > This patch ensures that the bindings recognise that these
> > variables are dealt with as output ones. On Python this
> > changes the calling convention to be more pythonic and enables
> > the value to be returned e.g.
> > 
> > before:
> >         s = ''
> >         v = m.get_manufacturer(s)
> >         # s is still empty
> > 
> > after:
> >         (v, s) = m.get_manufacturer()
> >         # s == "QUALCOMM INCORPORATED"
> > 
> > ---
> > 
> >  build-aux/qmi-codegen/VariableString.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/build-aux/qmi-codegen/VariableString.py b/build-aux/qmi-
> > codegen/VariableString.py
> > index a127eb7..96f49f4 100644
> > --- a/build-aux/qmi-codegen/VariableString.py
> > +++ b/build-aux/qmi-codegen/VariableString.py
> > 
> > @@ -258,7 +258,7 @@ class VariableString(Variable):
> >                           'name' : variable_name }
> >          
> >          template = (
> > 
> > -            '${lp}@${name}: a placeholder for the output constant string,
> > or %NULL if not required.\n')
> > +            '${lp}@${name}: (out): a placeholder for the output constant
> > string, or %NULL if not required.\n')
> > 
> >          return string.Template(template).substitute(translations)
> 
> So, this fixes it for strings. The other variable types would need the
> same update. Are you able to add those as well?
yes, can do. Just stumbled upon their usage in uim_get_pin_status

> 
> > --
> > 1.7.11.7
> > 
> > ###################
> > 
> >>From 0147e1fdcb4eeac7cb761ccf7069753f8848b45c Mon Sep 17 00:00:00 2001
> >>
> > From: Andrew Bird <ajb at spheresystems.co.uk>
> > Date: Fri, 16 Nov 2012 11:59:49 +0000
> > Subject: [PATCH 2/2] Allow unused parameters to have null passed in
> > 
> > ---
> > 
> >  build-aux/qmi-codegen/Client.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/build-aux/qmi-codegen/Client.py
> > b/build-aux/qmi-codegen/Client.py index 5fa4178..7a95d29 100644
> > --- a/build-aux/qmi-codegen/Client.py
> > +++ b/build-aux/qmi-codegen/Client.py
> > 
> > @@ -286,7 +286,7 @@ class Client:
> >              if message.input.fields is None:
> >                  input_arg_template = 'gpointer unused'
> >                  translations['input_var'] = 'NULL'
> > 
> > -                translations['input_doc'] = 'unused: %NULL. This message
> > doesn\'t have any input bundle.'
> > +                translations['input_doc'] = 'unused: (allow-none): %NULL.
> > This message doesn\'t have any input bundle.'
> > 
> >              else:
> >                  input_arg_template = '${input_camelcase} *input'
> >                  translations['input_var'] = 'input'
> 
> Nothing else to comment abuot this one.
> 
> 
> Some of the things I'm not sure about is which is the default 'transfer'
> mode for variables if none defined. Is it full? or none?
It depends upon the type, but a diff on the updated .gir file shows all outputs 
added have become 'full'
 
> 
> The input and output bundles have new(),ref() and unref(). In the case
> of new() and ref() they should be transfer full; I don't think I added
> those. Then, for all set/get operations in the bundles, the variables
> passed are all transfer-none. So seems to me that either one or the
> other also needs annotation tags to specify the correct transfer mode.
> Are you able to look at that as well?
Not sure about this yet, but I'll have a look.

> 
> 
> You're coming with a good set of patches here :-) If you have a public
> repo somewhere with all these in a branch let me know.

When I get through testing out various things I'll push them up to github.

Andrew