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 687356 - Built-in MCCMNC to Operator Name conversion doesn't work
Built-in MCCMNC to Operator Name conversion doesn't work
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: network-indicator
3.7.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-11-01 16:56 UTC by Aleksander Morgado
Modified: 2012-11-05 21:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
This patch fixes the issue (1.12 KB, patch)
2012-11-01 16:58 UTC, Aleksander Morgado
reviewed Details | Review
Patch for the issue (13.72 KB, patch)
2012-11-02 20:14 UTC, Aleksander Morgado
reviewed Details | Review
Patch for the issue, plus unit tests (26.76 KB, patch)
2012-11-05 13:21 UTC, Aleksander Morgado
accepted-commit_now Details | Review

Description Aleksander Morgado 2012-11-01 16:56:44 UTC
The shell has a built-in logic to convert a given 3GPP/MCCMNC string or 3GPP2/SID integer into a proper Operator Name, which is used when the modem doesn't report the operator name itself.

This logic seems to be currently broken, as the 'providers' list retrieved with _getProvidersTable() in modemManager.js contains undefined objects, so the conversion is never done.
Comment 1 Aleksander Morgado 2012-11-01 16:58:11 UTC
Created attachment 227823 [details] [review]
This patch fixes the issue
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-11-01 17:48:10 UTC
Review of attachment 227823 [details] [review]:

The unfortunate thing is that this breaks gtk-doc.
Comment 3 Aleksander Morgado 2012-11-01 18:11:24 UTC
Any suggestion in how to fix it?
Comment 4 Aleksander Morgado 2012-11-01 18:35:04 UTC
I'm assuming the issue of gtk-doc is that it doesn't know how to handle the additional element type for the list items inside the hash table. Is there already a bugreport in gtk-doc for this?

Anyway, the only consumer of the values reported by the parser seems to be modemManager.js, and in this case we completely ignore the 'key' of the hash table.

So, would it be better if instead of a HashTable with:
    (element-type utf8 GList<ShellMobileProvider>)
we modify the method to return just a GList of these?:
    (element-type ShellMobileProvider)
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-11-01 18:35:43 UTC
Fine with me.
Comment 6 Giovanni Campagna 2012-11-02 14:52:22 UTC
Note though that gobject-introspection has code to support
(element-type utf8 GList(ShellMobileProvider)), but using that syntax breaks in a different way, so you may want to fix g-i instead.
Comment 7 Aleksander Morgado 2012-11-02 20:14:10 UTC
Created attachment 227924 [details] [review]
Patch for the issue

Patch with a new approach to solve the issue. Modified the parse() method to return a hashtable where the key is the countrycode and the value is a new 'ShellCountryMobileProvider' type. This new type covers both the providers list and the country name information, so no need to return a second optional hash table any more.

How does this look?
Comment 8 Giovanni Campagna 2012-11-02 20:29:54 UTC
Review of attachment 227924 [details] [review]:

Seems fine, and builds fine.
Needs testing with a 3G device (or, even better, some unit tests)

::: src/shell-mobile-providers.c
@@ +712,2 @@
  *
+ * Returns: (element-type utf8 Shell.CountryMobileProvider) (transfer container): a

Not your bug, but this really is (transfer full), as nothing else retains the hash table contents.
Comment 9 Aleksander Morgado 2012-11-02 20:43:13 UTC
I tested it with some of my modems here. I guess the best way to ensure the code is used is to hack it so that the operator name reported by the modem is ignored. This should work:

-            this.operator_name = this._findOperatorName(name, code);
+            this.operator_name = this._findOperatorName('', code);

Anyway, I'll write some unit tests for all this as well.
Comment 10 Aleksander Morgado 2012-11-05 13:21:44 UTC
Created attachment 228107 [details] [review]
Patch for the issue, plus unit tests

So, this patch is an update of the previous one, fixing the transfer container vs full issue, plus including unit tests. Instead of relying on the system-installed "iso3166.tab" and "serviceproviders.xml" files, I also included some shorter test files with minimum contents.

If you want to fully test this patch from within jhbuild you'll need to:
$> cp -r /usr/share/mobile-broadband-provider-info $HOME/gnome/install/share
$> mkdir -p $HOME/gnome/install/share/zoneinfo
$> cp  /usr/share/zoneinfo/iso3166.tab $HOME/gnome/install/share/zoneinfo

I've tested it with my mobile provider, which if in 3G reports both MCCMNC and operator name itself (so the code is never triggered), but when in 2G only gives me MCCMNC (of another operator with which they have a deal), which triggers the code to search operator name from MCCMNC.
Comment 11 Giovanni Campagna 2012-11-05 17:03:52 UTC
Review of attachment 228107 [details] [review]:

Very nice!
Comment 12 Aleksander Morgado 2012-11-05 21:21:14 UTC
Pushed to git master then.

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.