Opened 9 years ago

Closed 9 years ago

Last modified 6 years ago

#1652 closed enhancement (fixed)

Add Connection Information to 3G (GSM) Modem Support

Reported by: Dcastelo Owned by: Dcastelo
Priority: Urgent Milestone:
Component: Sugar Version: Git as of bugdate
Severity: Major Keywords: 3G GSM modem usb r+
Cc: tomeu, aa, tch Distribution/OS: Fedora
Bug Status: Assigned

Description

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.

Attachments (6)

3G Support-Connection-Info.patch (6.6 KB) - added by Dcastelo 9 years ago.
3G Support-Connection-Info.patch V1
0001-3G-Connection-Info.patch (6.6 KB) - added by tomeu 9 years ago.
0002-Patch-Connection-InfoV2.patch (6.3 KB) - added by Dcastelo 9 years ago.
3G Support-Connection-Info.patch V2
0001-3G-Connection-Info.2.patch (7.1 KB) - added by Dcastelo 9 years ago.
3G-Connection-Info.patch (mixed of previous patches)
1652.patch (6.4 KB) - added by erikos 9 years ago.
diff with fixed issues
0001-Connection-Info-Fix.patch (2.1 KB) - added by Dcastelo 9 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by 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.

comment:2 Changed 9 years ago by Dcastelo

  • Keywords r! removed

Changed 9 years ago by Dcastelo

3G Support-Connection-Info.patch V1

comment:3 Changed 9 years ago by Dcastelo

  • Keywords r? added

Changed 9 years ago by tomeu

comment:4 Changed 9 years ago by tomeu

  • Keywords r! added; r? removed
+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?

Changed 9 years ago by Dcastelo

3G Support-Connection-Info.patch V2

comment:5 Changed 9 years ago by Dcastelo

  • Keywords r? added; r! removed

Changed 9 years ago by Dcastelo

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

Changed 9 years ago by erikos

diff with fixed issues

comment:6 Changed 9 years ago by erikos

  • Keywords r+ added; r? removed
  • Resolution set to fixed
  • Status changed from new to closed

comment:7 Changed 9 years ago by erikos

  • Bug Status changed from Unconfirmed to Assigned
  • Milestone changed from Unspecified by Release Team to 0.88
  • Priority changed from Unspecified by Maintainer to Urgent
  • Resolution fixed deleted
  • Severity changed from Unspecified to Major
  • Status changed from closed to reopened
  • Version changed from Unspecified to Git as of bugdate

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

Changed 9 years ago by Dcastelo

comment:8 follow-up: Changed 9 years ago by Dcastelo

  • Keywords r? added; r+ removed

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.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 9 years ago by 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.

comment:10 in reply to: ↑ 9 Changed 9 years ago by tomeu

  • Keywords r+ added; r? removed
  • Resolution set to fixed
  • Status changed from reopened to closed

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!

comment:11 Changed 6 years ago by dnarvaez

  • Milestone 0.88 deleted

Milestone 0.88 deleted

Note: See TracTickets for help on using tickets.