[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        
------------------------------------------+---------------------------------

Comment(by tch):

 +# Copyright (C) 2009 Martin Abente
     Is your employer ok with this? (Just trying to avoid possible problems
 in the future)

 Done

     +storage = StorageGsm?()
     Better instantiate it when it's actually needed, see here for how we
 use to implement singletons:

 Done

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

 I believe yes, first reason is that it has an small control method that
 makes no sence to repeat everytime i want to access this information, and
 second, is not recommended to spread all over the harcoded gconf paths i
 used for the settings, because it might change.

     +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.

 Not exactly but mostly like it, Done

     + 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()?

 It is the same logic as all other control panel sections, i can dig more
 about this later and report incase is a common error.

     + 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.

 Done

     +_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.

 Done

     + 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.

 Done

     + 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.

 I dont think it is necessary since GsmDeviceView is only created when its
 needed, so GsmPalette().

     +logging.error('Could not connect to gsm connection, %s', detail)
     Better to use logging.exception so we get a nice traceback.

 Done

     + 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.

 I choose those identifiers because thats how its called in the whole code,
 thought it would be more readable because of that, i.e check line 463

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

 To keep the same logic when the menuitem emits gsm-idle signal as any
 other signal, without having to disconnect and reconnect events, im open
 for better suggestions that doesnt change the logic for only 1 special
 case, :)

     + logging.error('Error getting gsm state %', err)
     Better raising an exception so we get a backtrace.

 Done

     + return { 'gsm' : secrets }
     Watch out the extra spaces.

 Done

     - 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.

 save() is a NMSettingsConnection() method not a Settings() method, it
 needs decoupling but i didnt wanted to change that part if the original
 code because the patch would grow even more, so i only added those control
 if's to avoid problems

     + self.client = gconf.client_get_default()
     Why public?

 Should i use a gconf.client_get_default() for every set/get?

     + logging.error("While loading gsm connection from settings")
     Better logging.exception and make sure that the original exception is
 not lost.

 Done

     +if password != :
     "if password:" is enough

 Done

     +logging.error("While adding the gsm connection")
     Same as before, eating exceptions make support much harder.

 Done

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

 Thanks, please check the last patch to see the changes

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