#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)
Change History (17)
comment:1 Changed 13 years ago by Dcastelo
comment:2 Changed 13 years ago by Dcastelo
- Keywords r! removed
comment:3 Changed 13 years ago by Dcastelo
- Keywords r? added
Changed 13 years ago by tomeu
comment:4 Changed 13 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?
comment:5 Changed 13 years ago by Dcastelo
- Keywords r? added; r! removed
comment:6 Changed 13 years ago by erikos
- Keywords r+ added; r? removed
- Resolution set to fixed
- Status changed from new to closed
Thanks Daniel for following the review process that thorough.
comment:7 Changed 13 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 13 years ago by Dcastelo
comment:8 follow-up: ↓ 9 Changed 13 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: ↓ 10 Changed 13 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 13 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!
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: