Thanks for the review. I've made all changes except for the ones listed below:
On Mon, Oct 24, 2011 at 6:41 PM, Sascha Silbe <si...@activitycentral.com> wrote: > [extensions/cpsection/modemconfiguration/view.py] > [ModemConfiguration.__init__()] >> + self._timeout_sid = 0 > > How about _timeout_source_id? It took me some time to figure out what > "sid" is supposed to mean. (Yes, I'm aware you only moved it.) Yes. I'm not making changes to existing code where they aren't necessary at this stage. I will follow up with further cleanup work moving this to gobject introspection later. If you want to fix existing stylistic items in the meantime I have no objection but I won't be doing it myself. >> - def undo(self): >> - self._model.undo() > > What happened to the undo functionality? Since we still apply the > changes after a timeout rather than after closing the dialog with the > "apply" / "OK" button, we need this for the "cancel" button. I couldn't figure out how this is supposed to work. It looks broken - where is undo implemented? undo with an instant-apply configuration system doesn't make much sense to me. > [class WirelessDeviceView] >> def __deactivate_connection_cb(self, palette, data=None): >> - if self._mode == network.NM_802_11_MODE_INFRA: >> - connection = network.find_connection_by_ssid(self._name) >> - if connection: >> - connection.disable_autoconnect() >> - >> network.disconnect_access_points([self._active_ap_op]) > > Wouldn't this reintroduce SL#1802 [1]? I guess something like the > following should do the trick: > > settings = connection.get_settings() > settings['connection']['autoconnect'] = False > connection.update_settings(settings) > > The same applies to WirelessNetworkView. Disabling autoconnect is not what you want any more - this would mean that the network would not be connected to after rebooting, as this setting persists. This new code matches the way that nm-applet does it and seems to work OK. > [class GsmDeviceView] >> def _update_state(self, state, old_state, reason): >> gsm_state = None >> >> - if state is network.DEVICE_STATE_ACTIVATED: >> + if state is network.NM_DEVICE_STATE_ACTIVATED: > > Since you're already touching these lines, how about replacing the "is" > with "=="? While the former does work in the specific case the constants > are integers (and we might rely on that fact for the range comparisons > as noted above), it's bad practice IMO. As noted above I'm not yet going to make this kind of effort for existing code that does not need to be modified, but I would not object to others doing it. > [src/jarabe/desktop/networkviews.py] > [WirelessNetworkView.__init__()] >> - interface_props.Get(_NM_DEVICE_IFACE, 'State', >> - reply_handler=self.__get_device_state_reply_cb, >> - error_handler=self.__get_device_state_error_cb) >> - interface_props.Get(_NM_WIRELESS_IFACE, 'WirelessCapabilities', > [...] >> def __get_active_ap_reply_cb(self, ap_path): >> self.__update_active_ap(ap_path) >> + interface_props = dbus.Interface(self._device, >> dbus.PROPERTIES_IFACE) >> + interface_props.Get(network.NM_DEVICE_IFACE, 'State', >> + reply_handler=self.__get_device_state_reply_cb, >> + error_handler=self.__get_device_state_error_cb) > > Is there a reason for the change of sequence? Yes. Now that networks may be set up and connected to outside of Sugar, the code must not assume that networks start off as disconnected like it did before. So, in the current (and slightly weird) design of the code, the active AP must be known before deciding how to parse device state, since that information is used in the decisions that are made. Without this, if NM connects to a network during boot, it won't show as connected when Sugar later starts up. > [WirelessNetworkView._connect()] >> + # Otherwise, create new connection and activate it >> + logging.debug("Creating new connection for %s", self._name) >> + settings = Settings() >> + settings.connection.id = str(self._name) > > We should check that this doesn't introduce even more problems with > non-ASCII SSIDs. See SL#2023 [3], SL#2099 [4]. > > Also, is there a reason we change the name from 'Auto <SSID>' to just > '<SSID>'? Did NM / nm-applet do the same? (I don't mind the change, just > wondering about the reason.) Yes, the "Auto" thing is gone, and connecting to networks with non-ascii characters works fine in my testing. > [deactivate_active_channel()] >> + netmgr_props.Get(network.NM_IFACE, 'ActiveConnections', \ >> reply_handler=self.__get_active_connections_reply_cb, >> error_handler=self.__get_active_connections_error_cb) > > No need for the backslash here, implied line continuation (open > parenthesis) is enough. Again, the code was already like this. >> + @dbus.service.method(NM_SECRET_AGENT_IFACE, >> async_callbacks=('reply', 'error'), >> + in_signature='a{sa{sv}}osasb', >> + out_signature='a{sa{sv}}', >> + sender_keyword='sender', >> + byte_arrays=True) >> + def GetSecrets(self, settings, connection_path, setting_name, hints, >> + request_new, reply, error, sender=None): >> + if setting_name != '802-11-wireless-security': >> + raise ValueError("Unsupported setting type %s" % setting_name) >> + if not sender: >> + raise Exception("Internal error: couldn't get sender") >> + uid = self._bus.get_unix_user(sender) >> + if uid != 0: >> + raise Exception("UID %d not authorized" % uid) >> + >> + response = SecretsResponse(reply, error) >> + try: >> + self.secrets_request.send(self, settings=settings, >> response=response) >> + except Exception: >> + logging.exception('Error requesting the secrets via dialog') > > Shouldn't we return something? Don't think so - why do you think this is needed? > Also, what gets blocked while we're waiting for the dialog to get filled > in and submitted? Would it make sense to make this method asynchronous? This is already asynchronous, not sure what makes you think otherwise. >> + def is_sugar_internal_connection(self): >> + """Returns True if this connection is a 'special' Sugar connection, >> + i.e. one that has been created by Sugar internals and should not be >> + visible to the user or deleted by connection-clearing code.""" >> + connection_id = self.get_id() >> + return connection_id == GSM_CONNECTION_ID \ >> + or connection_id.startswith(ADHOC_CONNECTION_ID_PREFIX) \ >> + or connection_id.startswith(MESH_CONNECTION_ID_PREFIX) \ >> + or connection_id.startswith(XS_MESH_CONNECTION_ID_PREFIX) > > Using the 'in' operator makes it terser and IMO more readable: > > return connection_id in (GSM_CONNECTION_ID, ADHOC_CONNECTION_ID_PREFIX, > MESH_CONNECTION_ID_PREFIX, > XS_MESH_CONNECTION_ID_PREFIX) > > We could even factor out the tuple into a global constant. Your suggested replacement code does not match the behaviour of the original, so I have left the original in place. >> + connections_o = settings.ListConnections() >> + for connection_o in connections_o: >> + self._monitor_connection(connection_o) > > Why the intermediary variable? It does not seem to provide any more > information, nor would we otherwise need a line break. > connection_path would be a better name for connection_o, BTW (the same > applies to a few other places with *_o variables). I've left them named as connection_o for now as a lot of the existing code uses this name. But I have removed the temporary variable. >> + def clear(self): >> + """Remove all connections except Sugar-internal ones.""" >> + >> + # copy the list, to avoid problems with removing elements of a list >> + # while looping over it >> + connections = list(self._connections) >> + for connection in connections: >> + if connection.is_sugar_internal_connection(): >> + continue >> + connection.delete() > > Should we really attempt to delete _all_ connections, including those > created by other users and / or the administrator? What happens if we > are disallowed from removing an individual connection (e.g. because it > was set up by the administrator and we have no permission to remove it)? > Will we end up with only some of the connections removed? Connections which we do not have permission to access are not presented to Sugar at all, so this already behaves OK. > [_migrate_old_wifi_connections()] >> + os.unlink(config_path) > > We should rename the file rather than irrevocably destroying it. I disagree - once the data has been migrated there is no need to store a duplicate copy. The file will only be deleted if the migration succeeds. Daniel _______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel