Add Connection Information to 3G (GSM) Modem Support

[Dcastelo]

In ticket #1622 the feature "3G Modem Support" was added. This is a improve to this feature adding the data sent /received and the connection time when the connection is established.

[Dcastelo]

The connection information (data sent/received and connection info) is shown as properties of the device icon (when the connection is established).

To get the data sent/received uses the PppStats ( u: in_bytes, u: out_bytes ) signal available in NetworkManager interface org.freedesktop.NetworkManager.Device.Serial ( http://projects.gnome.org/NetworkManager/developers/spec-07.html#org.freedesktop.NetworkManager.Device.Serial).

To show the connection time uses a gobject timeout signal. Detects that the connection was established by the StateChanged signal available in org.freedesktop.NetworkManager.Device.

Test Case:
1. Go to control panel, Modem Configuration
2. Set username, password, number, and apn
3. Save changes and restart
4. Connect the device and wait until the device icon appears
5. Click on connect item
6. Check that connection information is ok.
7. Click on disconnect item
8. Click on connect item
9. Check that the new connection information is ok.

[Dcastelo]

3G Support-Connection-Info.patch V1

[tomeu]

+import datetime, time

One import in each line.

+        def _padded(child, xalign=0, yalign=0.5):

Better make it a method, otherwise it's harder to read. And use a verb for the name, such as _add_padded_widget or _add_widget_with_padding.

+            padder = gtk.Alignment(xalign=xalign, yalign=yalign,

alignment instead of padder would be a more conventional name for the instance reference.

+        self._info = gtk.VBox()

No need to be a member of the class, because it's not used outside the constructor. "info" is a bit light in meaning, what about info_box?

+        self._info.show_all()

Better use individual show() calls for each widget, so effects of the code are more local.

@@ -770,8 +800,20 @@ class GsmDeviceView(TrayIcon):
 
         if state is network.DEVICE_STATE_ACTIVATED:
             gsm_state = _GSM_STATE_CONNECTED
-
-        elif state is network.DEVICE_STATE_DISCONNECTED:
+            connection = network.find_gsm_connection()
+            if (connection <> None):
+                connection.set_connected()
+                self.__connection_timestamp =  time.time() - connection.get_settings().connection.timestamp
+                self.__connection_time_handler = gobject.timeout_add(1000, self.connection_timecount_cb) 
+                self.__ppp_stats_changed_cb(0, 0)
+                self.connection_timecount_cb()                
+                self._palette._info.show_all() 
+        else: 
+            self.__connection_timestamp = 0
+            if (self.__connection_time_handler <> None):
+                gobject.source_remove(self.__connection_time_handler)
+            self._palette._info.hide_all() 
+        if state is network.DEVICE_STATE_DISCONNECTED:
             gsm_state = _GSM_STATE_DISCONNECTED
 
         elif state in [network.DEVICE_STATE_UNMANAGED,

I think I screwed this up when rebasing, can you check the ifs are ok?

+            if (connection <> None):

no parens and "is not" instead of "<>"

+                self.__connection_timestamp =  time.time() - connection.get_settings().connection.timestamp
+                self.__connection_time_handler = gobject.timeout_add(1000, self.connection_timecount_cb) 

Too long lines. Only callbacks should have two leading underscores.

+                self.__ppp_stats_changed_cb(0, 0)

Better not to call callbacks directly. Consider instead adding a method _update_stats() and calling it from ppp_stats_changed_cb.

+                self._palette._info.show_all() 

Shouldn't access a private member (_info), instead add a public method.

+        self._palette ._data_label.set_text("Data sent {0} kb / received {1} kb".format(out_bytes ,in_bytes))

Watch out private access, line length and we need to make that string translatable with gettext: _("blah blah"). Use the % operator instead of format().

+    def connection_timecount_cb(self):

callbacks should have two leading underscores (see PEP08 for the details)

+        self._palette._conn_time_label.set_text("Connection time " + connection_time.strftime('%H : %M : %S'))

Private access, string should be translatable.

+             self._settings.connection.timestamp = int(time.time())

Now I get what timestamp is. Can we call it connection_time or start_time instead?

[Dcastelo]

3G Support-Connection-Info.patch V2

[Dcastelo]

3G-Connection-Info.patch (mixed of previous patches)

[erikos]

diff with fixed issues

[erikos]

Thanks Daniel for following the review process that thorough.

 http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/f5daf6e6984badace857a37a3e399bf005a33614

[erikos]

Hi Daniel, I have a few suggestions from Tomeu to the current code we landed it git yesterday. Can you give us another patch with these fixed?

-        elif state is network.DEVICE_STATE_DISCONNECTED:
+        if state is network.DEVICE_STATE_DISCONNECTED:

Why replacing that elif for if?

+                self._connection_time_handler = gobject.timeout_add(1000, self.__connection_timecount_cb) 

All timeouts should be started with gobject.timeout_add_seconds, to increase the idle time between timeouts
Ideally, this timeout would be only active while the palette is visible and the palette would update its stats on popup (there's a signal for that)

+        self._palette._data_label.set_text(_("Data sent %d kb / received %d kb") % (out_kbytes ,in_kbytes))

kb doesn't mean kilobits? Maybe better KB? Watch out the spacing around the comma

+        self._palette._conn_time_label.set_text(_("Connection time ") + connection_time.strftime('%H : %M : %S'))

Guess that the time format should be translatable, to account for different formats across countries

[Dcastelo]

I could fix some points. But I had problems with two of them:

+ "Ideally, this timeout would be only active while the palette is visible and the palette would update its stats on popup (there's a signal for that)"

I remove the time handler at the same moment that hide the palette:

if self._connection_time_handler is not None:

gobject.source_remove(self._connection_time_handler)

self._palette.info_box.hide()

And I add the handler when I show it:

self._connection_time_handler = gobject.timeout_add(1000... self._palette.info_box.show()

+ self._palette._conn_time_label.set_text(_("Connection time ") + connection_time.strftime('%H : %M : %S'))
Guess that the time format should be translatable, to account for different formats across countries

I don't know the way to make this, but the connection time depends on the country? Because it isn't a "normal" date.

[tomeu]

Replying to Dcastelo:

I could fix some points. But I had problems with two of them:

+ "Ideally, this timeout would be only active while the palette is visible and the palette would update its stats on popup (there's a signal for that)"

I don't think this is so much of a problem, because affects only to people who are connected to the 3g network (that's why I said "ideally").

I remove the time handler at the same moment that hide the palette:
if self._connection_time_handler is not None:
gobject.source_remove(self._connection_time_handler)
self._palette.info_box.hide()

Best is to listen to the "popup" signal in the palette. For examples grep the code like this: "grep -R connect src | grep popdown"

And I add the handler when I show it:

self._connection_time_handler = gobject.timeout_add(1000... self._palette.info_box.show()

This could be better done by listening to the "popup" signal in the palette, similar to above.

+ self._palette._conn_time_label.set_text(_("Connection time ") + connection_time.strftime('%H : %M : %S'))
Guess that the time format should be translatable, to account for different formats across countries

I don't know the way to make this, but the connection time depends on the country? Because it isn't a "normal" date.

I also don't know, can you ask Sayamindu and/or the localization mailing lists? Maybe the time format is the same in all countries, not sure.

[tomeu]

Replying to tomeu:

Replying to Dcastelo:

I could fix some points. But I had problems with two of them:

+ "Ideally, this timeout would be only active while the palette is visible and the palette would update its stats on popup (there's a signal for that)"

I don't think this is so much of a problem, because affects only to people who are connected to the 3g network (that's why I said "ideally").

Tracking this one here, we'll deal with it at some point:

 http://dev.sugarlabs.org/ticket/1727

I think this one can be closed now, thanks!