[Bugs] #1610 NORM: Alternative to Create a new wireless network.
Sugar Labs Bugs
bugtracker-noreply at sugarlabs.org
Thu Aug 12 03:56:31 EDT 2010
#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
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
> + 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.
> + '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'
> + self._devices[device_o].connect('ap_added',
> + self._devices[device_o].connect('ap_removed',
> 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)
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:
> + 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
if self._adhoc_manager is not None:
> + 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.
_adhoc_manager_instance = None
if _adhoc_manager_instance is None:
_adhoc_manager_instance = AdHocManager()
get_adhoc_manager_instance is long but get_manager_instance would not be
descriptive enough imho.
> + try:
> + self._current_channel =
> + 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).
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
More information about the Bugs