After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 688206 - Expose the common mobile providers database parser API in libnm-gtk
Expose the common mobile providers database parser API in libnm-gtk
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
0.9.x
Other Linux
: Normal normal
: ---
Assigned To: Aleksander Morgado
NetworkManager maintainer(s)
Depends on:
Blocks: 688943 688944
 
 
Reported: 2012-11-12 20:32 UTC by Aleksander Morgado
Modified: 2012-12-04 17:12 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Aleksander Morgado 2012-11-12 20:32:07 UTC
Currently gnome-shell has some generic code to parse both the mobile broadband providers database and the country codes list installed in the system. See:
http://git.gnome.org/browse/gnome-shell/tree/src/shell-mobile-providers.h
http://git.gnome.org/browse/gnome-shell/tree/src/shell-mobile-providers.c

This code is currently used by gnome-shell to translate MCCMNC and SID values to Operator Name strings, when ModemManager doesn't export itself the Operator Name.

This generic code could be re-used by any other application requiring that translation, like gnome-control-center, which currently doesn't do this and should probably do it.

A good place for this generic code could be either NetworkManager's libnm-util or ModemManager's libmm-glib...

What do you guys think?
Comment 1 Aleksander Morgado 2012-11-12 20:39:34 UTC
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?
Comment 2 Aleksander Morgado 2012-11-23 13:51:17 UTC
Assigning to me; already have a branch doing this. Will test it with gnome-shell and the control-center now.
Comment 3 Aleksander Morgado 2012-11-23 15:49:19 UTC
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.
Comment 4 Dan Winship 2012-11-26 22:21:40 UTC
> 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?
Comment 5 Aleksander Morgado 2012-11-27 10:55:04 UTC
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.
Comment 6 Aleksander Morgado 2012-12-03 11:51:00 UTC
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.
Comment 7 Dan Winship 2012-12-04 14:41:02 UTC
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
Comment 8 Aleksander Morgado 2012-12-04 16:45:33 UTC
> 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.
Comment 9 Aleksander Morgado 2012-12-04 17:12:53 UTC
(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.