[Bugs] #1622 UNSP: 3G (GSM) Modem Support

[Sugar Labs Bugs]

#1622: 3G (GSM) Modem Support
------------------------------------------+---------------------------------
    Reporter:  tch                        |          Owner:  tch                
        Type:  enhancement                |         Status:  new                
    Priority:  Unspecified by Maintainer  |      Milestone:  0.88               
   Component:  sugar                      |        Version:  0.86.x             
    Severity:  Unspecified                |       Keywords:  3G GSM modem usb r!
Distribution:  Ubuntu                     |   Status_field:  Unconfirmed        
------------------------------------------+---------------------------------
Changes (by tomeu):

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


Comment:

 > +# Copyright (C) 2009 Martin Abente

 Is your employer ok with this? (Just trying to avoid possible problems in
 the future)

 > +storage = StorageGsm()

 Better instantiate it when it's actually needed, see here for how we use
 to implement singletons:

 http://git.sugarlabs.org/projects/sugar/repos/mainline/blobs/master/src/jarabe/model/shell.py#line602

 > +storage = StorageGsm()

 Do we really need such a thin abstraction layer? It adds an indirection
 but I don't see it winning us much.

 > +class MyEntry(gtk.Entry):

 What about having UsernameEntry, PasswordEntry, etc.? Subclasses in python
 are very cheap. They could be subclasses of HBox, including both the label
 and the entry. I think that this would remove both the duplicated code and
 the need for setattrs/getattrs.

 >+        self._username_change_handler = self._username_entry.connect( \
 >+                'changed', self.__changed_cb)

 Won't be registering one more handler every time that setup() is called?
 Or is it guaranteed that before every setup() is called undo()?

 > +        if self._username_entry._is_valid and
 self._password_entry._is_valid and self._number_entry._is_valid and
 self._apn_entry._is_valid:

 This line needs to be wrapped. You could also avoid the if if you think it
 makes it clearer.

 > +_GSM_STATES = {
 > +    _GSM_STATE_NOT_READY : ['gsm-idle', '...', _('Please wait...')],
 > +    _GSM_STATE_DISCONNECTED : ['gsm-connect', _('Connect'),
 _('Disconnected')],
 > +    _GSM_STATE_CONNECTING : ['gsm-disconnect', _('Cancel'),
 _('Connecting...')],
 > +    _GSM_STATE_CONNECTED : ['gsm-disconnect', _('Disconnect'),
 _('Connected')]
 > +}

 I like these constructions in general, but in this case I think we can get
 away easily without duplicating any code. I would set the instance
 variables to None in __init__ and then call from there
 self.set_state(_GSM_STATE_NOT_READY) which would set them to their correct
 values in an ifelse statement.

 > +        self._current_signal =
 _GSM_STATES[_GSM_STATE_NOT_READY][_GSM_SIGNAL]

 I would prefer if you had instead a self._current_state var and in
 _toggle_state_cb() you emitted one signal or the other depending on it.
 This is because the reader of the code would have to track where is
 self._current_signal assigned in order to know which signal is emitted in
 every case.

 > +        self._palette = GsmPalette()

 Not really big deal, but would be better to create the palette lazily on a
 create_palette() method, so we reduce memory usage. Grep the code for
 examples.

 > +            logging.error('Could not connect to gsm connection, %s',
 detail)

 Better to use logging.exception so we get a nice traceback.

 > +        obj = self._bus.get_object(_NM_SERVICE, _NM_PATH)
 > +        netmgr = dbus.Interface(obj, _NM_IFACE)
 > +        netmgr_props = dbus.Interface(netmgr,
 'org.freedesktop.DBus.Properties')
 > +        active_connections_o = netmgr_props.Get(_NM_IFACE,
 'ActiveConnections')

 I think we should avoid using abbreviations.

 > +    def __gsm_idle_cb(self, palette, data=None):
 > +        pass

 What's the reason for this?

 > +        logging.error('Error getting gsm state %', err)

 Better raising an exception so we get a backtrace.

 > +        return { 'gsm' : secrets }

 Watch out the extra spaces.

 > -            self.save()
 > +            if self._settings.connection.type ==
 NM_CONNECTION_TYPE_802_11_WIRELESS:
 > +                self.save()

 I think I would just add a save() method to GsmSettings that did nothing.

 > +        self.client = gconf.client_get_default()

 Why public?

 > +        logging.error("While loading gsm connection from settings")

 Better logging.exception and make sure that the original exception is not
 lost.

 > +        if password != '':

 "if password:" is enough

 > +            logging.error("While adding the gsm connection")

 Same as before, eating exceptions make support much harder.

 That's all, excellent first patch, congratulations! Hope we push it soon.

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