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

 A few comments:

 {{{
 +    # Sets text from model to entry
 +    def set_text_from_model(self):
 +            self.set_text(self.get_method())
 +
 +    # Sets text from entry to model
 +    def set_text_to_model(self):
 +            self.set_method(self.get_text())
 }}}

 Redundant comments, if you really feel the need, use docstrings. Also,
 watch out the indentation, here and in the rest of the patch. Consider
 running pylint (see wiki).

 {{{
 +    def get_method(self):
 }}}

 'method' brings nothing new, what about calling it get_value?

 {{{
 +    def set_method(self, username):
 +        return self._model.set_username(username)
 }}}

 The return makes no sense.

 {{{
 +        self._username_entry = UsernameEntry(model)
 +        username_field = self._create_field(self._username_entry,
 _('Username:'))
 +        self.pack_start(username_field, False)
 }}}

 As mentioned in a previous review, it doesn't help dealing with the
 specifics of each entry class in two different places. Please make
 GsmEntry inherit from gtk.HBox and contain a label and an entry, you will
 be able to drop some public members, which reduces coupling and increases
 coherence. http://msdn.microsoft.com/en-us/magazine/cc947917.aspx

 {{{
 +        self._username_entry.set_text_from_model()
 +        self._password_entry.set_text_from_model()
 +        self._number_entry.set_text_from_model()
 +        self._apn_entry.set_text_from_model()
 }}}

 Why the entries don't initialize themselves in their constructors?

 {{{
 +GSM_USERNAME_PATH = '/sugar/nm/gsm/username'
 }}}

 Maybe use network instead of nm?

 {{{
 +class StorageGsm(object):
 }}}

 Still not convinced of this class being worth it.

 Btw, Daniel's patch at #230 will conflict with this one.

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