[Bugs] #1610 NORM: Alternative to Create a new wireless network.

[Sugar Labs Bugs]

#1610: Alternative to Create a new wireless network.
----------------------------+-----------------------------------------------
    Reporter:  reuben       |          Owner:  erikos  
        Type:  enhancement  |         Status:  assigned
    Priority:  Normal       |      Milestone:  0.90    
   Component:  sugar        |        Version:  0.84.x  
    Severity:  Unspecified  |       Keywords:  r!      
Distribution:  Unspecified  |   Status_field:  Assigned
----------------------------+-----------------------------------------------
Changes (by tomeu):

  * keywords:  r? => r!


Comment:

 {{{
 +        if self._mode == network.NM_802_11_MODE_ADHOC and \
 +                self._name.startswith('Ad-hoc Network'):
 }}}

 We are doing this check in lots of places, and that string is a bit easy
 to misstype because of the hyphen, caps, etc. Could we have a constant
 instead or maybe better, a global function?

 {{{
 +    def __init__(self, manager, channel):
 }}}

 If manager is a singleton, why pass it around and have references to it?
 Just have a get_instance() function in the module and call it whenever
 it's needed.

 {{{
 +        self._palette = self._create_palette()
 +        self.set_palette(self._palette)
 }}}

 Would be great if we could create the palette only when it's needed.

 {{{
 +        _palette = palette.Palette(_("Ad-hoc Network %d") %
 self._channel,
 +                                   icon=self._palette_icon)
 }}}

 PEP-08 says to avoid name clashes by appending an underscore, not by
 prepending it.

 {{{
 +                try:
 +                    frequency = properties['Frequency']
 +                    channel = network.frequency_to_channel(frequency)
 +                    if self._channel == channel:
 +                        self._active = True
 +                    else:
 +                        self._active = False
 +                except KeyError:
 +                    logging.debug("Error getting the Frequency.")
 }}}

 This looks dangerous to me, in which cases do you anticipate getting a
 KeyError?

 {{{
 +        """If a new adhoc network is announcd that is named like an Ad-
 hoc
 +        network we take this as an indication that other learners are
 +        using the network an indicate this.
 +
 +        """
 }}}

 Docstrings should be answers to the question "what does this
 module/class/method does?". Same for other docstrings in the patch.

 {{{
 +    def indicate_population(self, state):
 }}}

 indicate_population is a bit disconcerting?

 {{{
 +        if state == True:
 }}}

 "if state:" is enough, but then seems like we need a better name than
 "state".

 {{{
 +    def _add_adhoc_networks(self, device):
 }}}

 Should be named as a callback?

 {{{
 +            self._check_mesh_source = gobject.timeout_add( \
 }}}

 Better use gobject.timeout_add_seconds to group timeouts and reduce
 wakeups.

 {{{
 +        if self._adhoc_manager != None and \
 }}}

 "is not None"

 {{{
 +            if old_hash is None: # new Ad-hoc network finished
 initializing
 }}}

 This looks like a bit too magic to me.

 {{{
 +                self._adhoc_manager.add_access_point(ap)
 }}}

 The view is updating the model? The expected is that the model emits
 updates and the view reacts to those updates.

 {{{
  class MeshBox(gtk.VBox):
 }}}

 This may be a good opportunity to remove one more bogus reference to
 "Mesh".

 {{{
 +    def __init__(self, icon, access_point):
 +        self.icon = icon
 +        self.access_point = access_point
 }}}

 The model shouldn't reference the view, only the other way around.

 {{{
 +    timeout = 30
 }}}

 Should be in caps because it's a constant and would be good if the name
 implied what kind of timeout it is (_AUTOCONNECT_TIMEOUT?).

 {{{
 +        self.channels = [1, 6, 11]
 }}}

 Another constant?

 {{{
 +        logging.debug('Failed to create Ad-hoc network: %s', err)
 }}}

 logging.error?

 {{{
 +        if ' 1' == access_point.name[-2:]:
 }}}

 Considered using str.endswith()?

 {{{
 +            try:
 +                self.channel =
 frequency_to_channel(properties['Frequency'])
 +            except KeyError:
 +                self.channel = 1
 }}}

 What do you expect could emit KeyError here?

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