[Bugs] #1652 UNSP: Add Connection Information to 3G (GSM) Modem Support

[Sugar Labs Bugs]

#1652: Add Connection Information to 3G (GSM) Modem Support
------------------------------------------+---------------------------------
    Reporter:  Dcastelo                   |          Owner:  Dcastelo                   
        Type:  enhancement                |         Status:  new                        
    Priority:  Unspecified by Maintainer  |      Milestone:  Unspecified by Release Team
   Component:  sugar                      |        Version:  Unspecified                
    Severity:  Unspecified                |       Keywords:  3G GSM modem usb r!        
Distribution:  Fedora                     |   Status_field:  Unconfirmed                
------------------------------------------+---------------------------------
Changes (by tomeu):

  * keywords:  3G GSM modem usb r? => 3G GSM modem usb r!


Comment:

 {{{
 +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?

-- 
Ticket URL: <http://bugs.sugarlabs.org/ticket/1652#comment:4>
Sugar Labs <http://sugarlabs.org/>
Sugar Labs bug tracking system