Well... so this code in gnome-shell was originally coming from network-manager-applet... can we then include and export it in libnma (as done with the gnome-bluetooth stuff) or libnm-gtk?

Assigning to me; already have a branch doing this. Will test it with gnome-shell and the control-center now.

Branch 'aleksander/mobile-providers' now pushed to upstream git for review.

I've tested the code both with the wizard and in a patched gnome-shell.

> libnm-gtk,mobile-providers: import changes from gnome-shell

The "libnm-gtk,mobile-providers" prefix on all the commits is too long; just say "libnm-gtk"


> + *   list of #NMAGsmMccMnc this provider exposes

gtk-doc actually understands plurals, so you can say "#NMAGsmMccMncs" if you want.


> + * nma_mobile_provider_get_cdma_sid:
> + * @provider: a #NMAMobileProvider
> + *
> + * Returns: (element-type guint32) (transfer none): the

Hm... I'm not sure that using a non-pointer element-type in GSList is legit.

The fact that this works in the existing gnome-shell code is just a coincidence of how the gjs argument marshalling was written (and it *doesn't* work on big-endian 64bit platforms).

Returning a GArray, or a guint32* (with an (out) length argument) would be better.


Is there some reason you used G_DEFINE_BOXED_TYPE() for NMAGsmMccMnc but not for NMACountryInfo? Oh, I see, you fixed them all up later... maybe it would make more sense to move the "use G_DEFINE_BOXED_TYPE" commit first, rather than adding some types with explicit get_type() functions and then fixing them later...


> +                    g_warning ("%s: adding providers for unknown country '%s'", __func__, country_code);
> +                    country_info = country_info_new (country_code, NULL);

you need to g_strdup(country_code)


> +        country_info = g_hash_table_lookup (parser->table, parser->current_country);
> +        if (country_info)
> +            /* Store providers for this country */
> +            country_info->providers = parser->current_providers;
> +
>          parser->current_country = NULL;

it ought to be impossible for country_info to be NULL, given the "unknown country" handling code above, right?

Also, this leaks parser->current_country.


> libnm-gtk,mobile-providers: make all types opaque

You could also just add a "/*< private >*/" above the fields in the .h files, which would tell gobject-introspection to ignore them, but you could still access them directly from within libnm-gtk rather than having to use the accessors everywhere... (Or not. Your call.)


> libnm-gtk,mobile-providers: make refcounting thread-safe in the defined types

is this actually needed?

Hey Dan,

Thanks for the review, I'll rework the branch a bit more. Note that I also have some other additional issues to fix after dcbw's review and some other comments from gcampax in bug 688943. I'll possibly also add some unit tests for all this.

(In reply to comment #4)
> > libnm-gtk,mobile-providers: import changes from gnome-shell
> 
> The "libnm-gtk,mobile-providers" prefix on all the commits is too long; just
> say "libnm-gtk"
> 

Ok, will change that.

> 
> > + *   list of #NMAGsmMccMnc this provider exposes
> 
> gtk-doc actually understands plurals, so you can say "#NMAGsmMccMncs" if you
> want.
> 

Ah good, didn't know that. Actually, there's another branch where that whole type goes away and we just concatenate MCC and MNC in the same string, which seemed easier to handle.

> 
> > + * nma_mobile_provider_get_cdma_sid:
> > + * @provider: a #NMAMobileProvider
> > + *
> > + * Returns: (element-type guint32) (transfer none): the
> 
> Hm... I'm not sure that using a non-pointer element-type in GSList is legit.
> 
> The fact that this works in the existing gnome-shell code is just a coincidence
> of how the gjs argument marshalling was written (and it *doesn't* work on
> big-endian 64bit platforms).
> 
> Returning a GArray, or a guint32* (with an (out) length argument) would be
> better.
> 

Ah, good point. I think I'll go for the guint32* output. And, actually, will also do the same for get_3gpp_mcc_mnc() to return a (const gchar **), hope introspection can handle that as well.

> 
> Is there some reason you used G_DEFINE_BOXED_TYPE() for NMAGsmMccMnc but not
> for NMACountryInfo? Oh, I see, you fixed them all up later... maybe it would
> make more sense to move the "use G_DEFINE_BOXED_TYPE" commit first, rather than
> adding some types with explicit get_type() functions and then fixing them
> later...
> 

The idea was to have a commit porting all the code changes from gnome-shell exactly as they were, and then modify on top of that. But it's true that for that commit it doesn't make much sense, I'll fix it up.

> 
> > +                    g_warning ("%s: adding providers for unknown country '%s'", __func__, country_code);
> > +                    country_info = country_info_new (country_code, NULL);
> 
> you need to g_strdup(country_code)
> 

The dup() is done within country_info_new() here, don't think it's needed.

> 
> > +        country_info = g_hash_table_lookup (parser->table, parser->current_country);
> > +        if (country_info)
> > +            /* Store providers for this country */
> > +            country_info->providers = parser->current_providers;
> > +
> >          parser->current_country = NULL;
> 
> it ought to be impossible for country_info to be NULL, given the "unknown
> country" handling code above, right?

Yeah, will change that, maybe adding a g_assert().

> 
> Also, this leaks parser->current_country.
> 

True, will fix it.

> 
> > libnm-gtk,mobile-providers: make all types opaque
> 
> You could also just add a "/*< private >*/" above the fields in the .h files,
> which would tell gobject-introspection to ignore them, but you could still
> access them directly from within libnm-gtk rather than having to use the
> accessors everywhere... (Or not. Your call.)
> 

Well, given that we're now exposing it as API, I just wanted to make sure that we can change the structs cleanly.

> 
> > libnm-gtk,mobile-providers: make refcounting thread-safe in the defined types
> 
> is this actually needed?

Same reasoning as above, if we're now exposing this logic to clients of the library, just wanted to give some thread safety logic.

I updated the 'aleksander/mobile-providers' branch with the suggested fixes plus some additional things. Now the main interface to the mobile providers database is a new 'NMAMobileProvidersDatabase' GObject, which is a much better solution for clients using introspection. The other big change w.r.t the last review is the removal of the special NMAMccMnc type: now the interface always relies on 'mccmnc' strings instead.

Basically looks good.


You should use GPtrArray for NMAMobileAccessMethod->dns and NMAMobileProvider->mcc_mnc; it's a little simpler to use than GArray when the element type is a pointer. Also, you can use g_ptr_array_new_with_free_func() to tell it to use g_free to free the elements automatically, and then you don't need to free them yourself.


You don't need to explicitly implement init_async() and init_finish() on NMAMobileProvidersDatabase, because the default implementation runs the sync init() in a thread, which is basically exactly the same thing as your implementation does.


Squash the "po: update location of the mobile providers parsing code" commit into "libnm-gtk: expose the mobile providers database parser in the API" before landing this (so that "make check" isn't broken in between).


should be good to commit with those changes

> You should use GPtrArray for NMAMobileAccessMethod->dns and
> NMAMobileProvider->mcc_mnc; it's a little simpler to use than GArray when the
> element type is a pointer. Also, you can use g_ptr_array_new_with_free_func()
> to tell it to use g_free to free the elements automatically, and then you don't
> need to free them yourself.
> 

Ok.

> 
> You don't need to explicitly implement init_async() and init_finish() on
> NMAMobileProvidersDatabase, because the default implementation runs the sync
> init() in a thread, which is basically exactly the same thing as your
> implementation does.

I just learned about this. Good to know :-)

> 
> 
> Squash the "po: update location of the mobile providers parsing code" commit
> into "libnm-gtk: expose the mobile providers database parser in the API" before
> landing this (so that "make check" isn't broken in between).
> 

Ok.

(In reply to comment #7)
> 
> should be good to commit with those changes

Branch now merged, once the previous suggested fixes were done.


This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.