Added dependency on the NM bug.

Created attachment 229730 [details] [review]
Patch for the issue.

Please note that this patch should only be applied when libnm-gtk (in the network-manager-applet repository) includes the changes. When that is ready and the corresponding network-manager-applet released, we'll also need to bump the required version in configure.ac.

Review of attachment 229730 [details] [review]:

Yay, less code!

::: js/misc/modemManager.js
@@ +114,2 @@
         let table = _getProvidersTable();
+        let provider = NMGtk.mobile_providers_find_for_mcc_mnc(table, needle);

Is table a boxed/GObject or a GHashTable?
In the latter case, gjs might not like it a lot, and might end up iterating it fully multiple times to copy and then free the contents.

It is a GHashTable, with country code string as key and a GObject as value... What would be the way to avoid this?

Just wondering from my complete lack of knowledge on this; but maybe, instead of a _getProvidersTable(), we could have a _ensureProvidersTable() call which just makes sure that the table is created once, and then we directly use a global variable from within the code... would that help?

Personally, I'd create a NMMobileProvidersDatabase with a lookup() method. Maybe  async initialized. It is fine to use GHashTable internally, but I'd avoid exposing that to the JS layer.

Replacing _getProvidersTable with _ensureProvidersTable would be slightly better, but you'd still be iterating the table when passing it to find_for_sid() and find_for_mcc_mnc().

The issue here is that GJS maps GHashTable to native JS objects, so it must convert every key and every value to JS. And then, when you call the method, you're given a native JS object, so you iterate it again to build the GHashTable you need.

That actually makes sense... I think I'll make a new GObject and make it async-initable, so that it tries to load the contents of the country-code and mobile-provider files during init.

Dan, what do you think?

Sounds fine to me.

Created attachment 230519 [details] [review]
Updated patch using the new NMGtk.MobileProvidersDatabase object

This patch uses the new 'NMGtk.MobileProvidersDatabase' to handle the database of mobile providers.

The corresponding libnm-gtk is available in the 'aleksander/mobile-providers' branch of the network-manager-applet repository.

Review of attachment 230519 [details] [review]:

::: js/misc/modemManager.js
@@ +48,3 @@
+        _mpd = new NMGtk.MobileProvidersDatabase();
+        try {
+            _mpd.init(null);

We have that now, we should really do init_async().

@@ +50,3 @@
+            _mpd.init(null);
+        } catch (e) {
+            logError (e, 'Error while reading mobile broadband providers database');

Can this happen in practice?
If yes, logError() is wrong (it includes a backtrace). If no, ignore the error and let it crash the shell. Devs and testers will then fix it.

It may happen if the either the mobile broadband providers database or the country code list is not found. I guess just log() is fine in this case then?

(In reply to comment #10)
> It may happen if the either the mobile broadband providers database or the
> country code list is not found. I guess just log() is fine in this case then?

Yes, that would be appropriate.

Well, it depends. If neither of those are found, would you consider that a broken system?

(In reply to comment #12)
> Well, it depends. If neither of those are found, would you consider that a
> broken system?

Not really; the worst (and only) thing that can happen in that case is that we don't expose the operator name in the UI.

erm, I mean, would you expect a user's system to be shipped without those files, ever?

If not, those files being missing are a broken system. Something or somebody removed those files, and we shouldn't just carry on and log to a file nobody looks at until it's too late.

I really don't have a strong feeling on that issue. A system without those files is still fully functional w.r.t mobile broadband.

Created attachment 235223 [details] [review]
Rebased patch

So, rebased patch on top of git master.

Also changed the logError() to just log() when the mobile providers database cannot be loaded. Doing the MCCMNC/SID to operator name translation is really a nice to have thing; a system not able to do so shouldn't be considered a broken system IMO.

The changes required in the network manager applet are already released in version 0.9.7.995 (0.9.8-beta1), but I didn't update configure.ac to require this NM version.

Review of attachment 235223 [details] [review]:

::: js/misc/modemManager.js
@@ +15,3 @@
+    if (_mpd == null) {
+        _mpd = new NMGtk.MobileProvidersDatabase();
+        try {

If you put the constructor inside the try, it will allow running on an older libnm-gtk, which is generally desirable.

Created attachment 235230 [details] [review]
Added constructor inside the try{}

Review of attachment 235230 [details] [review]:

LGTM

This problem has been fixed in our software repository. The fix will go into the next software release. Thank you for your bug report.