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

Comment(by tomeu):

 {{{
         <long>>If TRUE, Sugar will show default Ad-hoc networks for
 channel 1,6 and 11</long>
 }}}

 One > too much, maybe we should mention we also create automatically an
 ad-hoc network, not just display them.

 {{{
 +            try:
 +                channel = network.frequency_to_channel(self._frequency)
 +            except KeyError:
 +                channel = 1
 }}}

 If you really want to use an exception here, then create a new one because
 KeyError can be raised because of several reasons. But probably
 frequency_to_channel() should just print a warning and return 1 if the
 frequency is not known.

 {{{
 +                if connection:
 }}}

 is not None

 {{{
 +                        connection.set_connected()
 }}}

 connection.connect() may be more in line with the existing code.

 {{{
 +        self._bus = dbus.SystemBus()
 +        self._manager = adhoc.get_manager()
 }}}

 No big deal, but if these aren't part of the sate of the class, then it
 would be better to just get references to them whenever we need it instead
 of caching the references.

 {{{
 +        'ap-added': (gobject.SIGNAL_RUN_FIRST, gobject.TYPE_NONE,
 }}}

 "ap" may not be obvious to all the future readers of this code.

 {{{
 +    _SHOW_ADHOC = '/desktop/sugar/network/adhoc'
 }}}
 s/_SHOW_ADHOC/_SHOW_ADHOC_GCONF_KEY?

 {{{
 +            self._devices[device_o].connect('ap_added',
 self.__ap_added_cb)
 +            self._devices[device_o].connect('ap_removed',
 self.__ap_removed_cb)
 }}}

 Better use - instead of underscores.

 {{{
 +            if properties['WirelessHardwareEnabled'] == True:
 }}}

 Why not else or at least elif? No need to compare to True/False (see
 PEP-08 for reasoning)

 {{{
 +            if self._adhoc_manager.remove_access_point(ap_o) == True:
 }}}

 Same here, but it's a bit weird to have the remove() func return a
 boolean. Why not having a separate is_ad_hoc() method or similar?

 {{{
 +            self._adhoc_manager.listen(device)
 }}}

 By the name of the method, I would expect it can be called several times.
 May be better to change it to start_listening() and to raise an exception
 if it's called more than once.

 {{{
 +_adhoc_manager = None
 +
 +
 +def get_manager():
 +    global _adhoc_manager
 +
 +    if not _adhoc_manager:
 +        _adhoc_manager = AdHocManager()
 +    return _adhoc_manager
 }}}

 May be good to keep it consistent with the other singletons. Don't assume
 that None evaluates to False.

 {{{
 +            try:
 +                self._current_channel =
 network.frequency_to_channel(frequency)
 +            except KeyError:
 +                logging.debug("Error getting the Frequency.")
 }}}

 Same as above.

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

 Why not endswith()? (Please reply to my comments if you don't agree so I
 don't have to repeat myself).

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