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

 Thanks for the review!

 Replying to [comment:22 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.

 I added the 'automatically connect part' to the description.

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

 Yes. moved the error handling part into the function and log a descriptive
 warning. Returning 1 might hide the error too much, though. Maybe 0 or -1?

 > {{{
 > +                if connection:
 > }}}
 >
 > is not None

 Done.

 > {{{
 > +                        connection.set_connected()
 > }}}
 >
 > connection.connect() may be more in line with the existing code.

 set_connected is actually a method of NMSettingsConnection
 (model/network.py). Do you mean to change that one?

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

 Done.

 > {{{
 > +        'ap-added': (gobject.SIGNAL_RUN_FIRST, gobject.TYPE_NONE,
 > }}}
 >
 > "ap" may not be obvious to all the future readers of this code.

 Changed it to 'access-point-added'. Is quite long, that is why I did not
 do so in the first place, but yeah more descriptive.

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

 Done.

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

 Done.

 > {{{
 > +            if properties['WirelessHardwareEnabled'] == True:
 > }}}
 >
 > Why not else or at least elif? No need to compare to True/False (see
 PEP-08 for reasoning)

 Both done. In the spec (http://www.python.org/dev/peps/pep-0008/) they
 only say, that it is not the correct form but not the reasoning :) Under
 'Resolved Issues' one can find more:
 http://www.python.org/dev/peps/pep-0285/

 > {{{
 > +            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?

 Yes, I merged the checking and removing in one method. A made it two
 methods now.

 if self._adhoc_manager is not None:
     if self._adhoc_manager.is_sugar_adhoc_access_point(ap_o):
         self._adhoc_manager.remove_access_point(ap_o)
         return


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

 Done.

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

 _adhoc_manager_instance = None
 def get_adhoc_manager_instance():
     global _adhoc_manager_instance
     if _adhoc_manager_instance is None:
         _adhoc_manager_instance = AdHocManager()
     return _adhoc_manager_instance

 get_adhoc_manager_instance is long but get_manager_instance would not be
 descriptive enough imho.

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

 Ok.

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

 Sorry, forgot to reply. I did not want to use endswith i the first place,
 as I was checking for one character too '6' and I wanted to be consistent.
 I do check for ' 6' now, so it works with endswith.

 New patch will follow, will do more testing after these changes first.

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