#1673 closed defect (fixed)
discard network history does nothing
Reported by: | quozl | Owned by: | tomeu |
---|---|---|---|
Priority: | High | Milestone: | |
Component: | Sugar | Version: | Git as of bugdate |
Severity: | Minor | Keywords: | r! |
Cc: | Distribution/OS: | ||
Bug Status: | New |
Description
clear_networks() in extensions/cpsection/network/model.py looks like this:
def clear_networks(): """Clear saved passwords and network configurations. """ pass
ideally this should clear the contents of nm/connections.cfg which is maintained by src/jarabe/model/network.py
is a contributing cause of previous workaround failure on dev.laptop.org 9977 (AP association failure after removing encryption).
Attachments (5)
Change History (28)
comment:1 Changed 14 years ago by tomeu
comment:2 Changed 14 years ago by quozl
- Keywords r? removed
Changed 14 years ago by quozl
Patch to Sugar 0.84 restores the discard network history button to normal operation. Review requested.
comment:3 Changed 14 years ago by quozl
- Keywords r? added
comment:4 Changed 14 years ago by erikos
- Keywords olpc-0.84 added
comment:5 Changed 14 years ago by erikos
- Milestone changed from Unspecified by Release Team to 0.84
comment:6 Changed 14 years ago by sayamindu
- Keywords r! added; r? removed
Is there any need for the
config = ConfigParser.ConfigParser()
Can't we simply write a blank file instead ?
comment:7 Changed 14 years ago by quozl
This does effectively write a blank file, in that the file size is zero.
The code was copied from load_connections. If you think that an empty file should be created without referring to ConfigParser, then you should change how load_connections creates the config file when it does not exist:
config = ConfigParser.ConfigParser() if not os.path.exists(config_path): if not os.path.exists(os.path.dirname(config_path)): os.makedirs(os.path.dirname(config_path), 0755) f = open(config_path, 'w') config.write(f) f.close()
to
if not os.path.exists(config_path): if not os.path.exists(os.path.dirname(config_path)): os.makedirs(os.path.dirname(config_path), 0755) f = open(config_path, 'w') f.close() config = ConfigParser.ConfigParser()
A test program consisting of an import of ConfigParser followed by either of these two methods costs 104ms on XO-1.5, averaged over 20 runs. There seems to be no real advantage.
However, I've not tested that inside Sugar.
comment:8 follow-up: ↓ 9 Changed 14 years ago by sascha_silbe
- Bug Status changed from Unconfirmed to New
- Distribution/OS OLPC deleted
- Milestone changed from 0.84 to 0.88.x
- Priority changed from Unspecified by Maintainer to High
I've rebased quozls patch on top of current git master. It works, but there are two issues:
- Icon badges in the Neighborhood are not updated.
- The button has immediate effect, in contrast to the usual commit/reset style of control panel settings.
comment:9 in reply to: ↑ 8 Changed 14 years ago by quozl
Replying to sascha_silbe:
- Icon badges in the Neighborhood are not updated.
These are properly updated in my patch to 0.88. I shall attach it.
- The button has immediate effect, in contrast to the usual commit/reset style of control panel settings.
This has always been how this button has worked in existing deployments. The radio disable button works the same way. Can you make a case for changing both? (And a patch?)
Changed 14 years ago by quozl
comment:10 Changed 14 years ago by quozl
- Keywords r? added; r! olpc-0.84 removed
requesting review and merging of above patch.
comment:11 follow-ups: ↓ 12 ↓ 13 Changed 14 years ago by tomeu
- Keywords r+ added; r? removed
+def get_wifi_connections_path():
...
+ def create_wifi_connections(config_path):
These seem like should be private? (leading underscore)
+ _nm_settings.clear_wifi_connections()
Shouldn't we use get_settings() so we can be sure _nm_settings has been initialized without having to read too much code?
This patch is r+'ed based the above petty issues are addressed, but please split it up in several commits, so we have one commit message per discrete change.
Also, please create an user in gitorious so you can push your own patches.
comment:12 in reply to: ↑ 11 Changed 14 years ago by tomeu
Oh, and forgot to mention that the code in _disconnect_activate_cb makes several dbus sync calls, and any of those could block the UI for too long.
Please get your colleagues at OLPC to agree with this if they haven't yet, notify other interested downstreams with an email to sugar-devel and add an inline comment in the code about it.
Changed 14 years ago by quozl
comment:13 in reply to: ↑ 11 ; follow-ups: ↓ 15 ↓ 16 Changed 14 years ago by quozl
- Keywords r+ removed
Replying to tomeu:
These seem like should be private? (leading underscore)
Fixed.
Shouldn't we use get_settings() so we can be sure _nm_settings has been initialized without having to read too much code?
Fixed.
This patch is r+'ed based the above petty issues are addressed, but please split it up in several commits, so we have one commit message per discrete change.
I was unable to determine a suitable split point.
Replying to tomeu:
... the code in _disconnect_activate_cb makes several dbus sync calls ...
So does the existing code, in many places. My patch does not and is not intended to address this existing design issue. I could not find this dbus sync issue described in any other ticket.
comment:14 Changed 14 years ago by bernie
- Keywords r? added
Why has this review been stuck for 3 months? You probably had to reset the r? flag.
/me hates bug tracker reviews
comment:15 in reply to: ↑ 13 Changed 14 years ago by bernie
Replying to quozl:
This patch is r+'ed based the above petty issues are addressed, but please split it up in several commits, so we have one commit message per discrete change.
I was unable to determine a suitable split point.
It seems that the clear_wifi_connections() part would be easy to split out.
comment:16 in reply to: ↑ 13 ; follow-up: ↓ 17 Changed 14 years ago by tomeu
- Keywords r! added; r? removed
Replying to quozl:
Replying to tomeu:
These seem like should be private? (leading underscore)
Fixed.
Shouldn't we use get_settings() so we can be sure _nm_settings has been initialized without having to read too much code?
Fixed.
This patch is r+'ed based the above petty issues are addressed, but please split it up in several commits, so we have one commit message per discrete change.
I was unable to determine a suitable split point.
Let me suggest some:
- use the constant dbus.PROPERTIES_IFACE instead of a literal
- implement clearing the networks history
- implement disconnecting from a network
- set the autoconnect flag to False when disconnecting from a network
Are you aware of the importance of not conflating several unrelated issues in the same commit?
Replying to tomeu:
... the code in _disconnect_activate_cb makes several dbus sync calls ...
So does the existing code, in many places. My patch does not and is not intended to address this existing design issue.
I doubt you will find in Sugar a loop with 3 sync calls inside like the one your patch introduces.
I could not find this dbus sync issue described in any other ticket.
I had a conversation with you some months ago about this and you seemed to agree.
comment:17 in reply to: ↑ 16 Changed 14 years ago by tomeu
Replying to tomeu:
- use the constant dbus.PROPERTIES_IFACE instead of a literal
- implement clearing the networks history
- implement disconnecting from a network
- set the autoconnect flag to False when disconnecting from a network
Btw, git add -i may be useful to you here.
comment:18 follow-up: ↓ 19 Changed 14 years ago by quozl
None of you seem willing to show your suggestions as code, so I'm presuming you are looking for consensus. I don't think consensus has been achieved.
I'm happy with the commit in 0.84.15 on 2010-04-01 (fcb1cec), and have no great desire to fix it for 0.88 yet, but I'm sure we can get there eventually, once there is consensus on how it should be applied. I shall wait for consensus.
Regarding Tomeu's doubt that I will find in Sugar a loop with 3 synchronous D-Bus calls like that one I'm introducing in my patch ... that's false, I'm copying this _disconnect_activate_cb code with slight modifications from __deactivate_connection_cb in extensions/deviceicon/network.py ... and if you think that __deactivate_connection_cb should be fixed then raise a separate ticket. I don't think it should be fixed, because it's not broken. Yes, mixing synchronous and asynchronous D-Bus calls is unwise, but it usually works, as can be seen in the existing code. I propose a separate ticket for review and fix of use of synchronous D-Bus calls.
Regarding a split point ... I agree that the split described by Tomeu is one option, but I don't see why it is the best option. The unsplit patch was fully tested, and the 0.84 unsplit variant of the patch has had extensive testing in the OLPC XO-1 and XO-1.5 builds. Also, I was especially asked by Tomeu in IRC to fix (and therefore test) the use of the dbus.PROPERTIES_IFACE constant. Perhaps it should be dropped altogether if it causes a need to split. I propose a separate ticket for that.
Are there any further suggestions? The new activity on this ticket is very encouraging. I guess it is because the 0.88 deployments are affected?
comment:19 in reply to: ↑ 18 ; follow-up: ↓ 20 Changed 14 years ago by bernie
Replying to quozl:
Regarding a split point ... I agree that the split described by Tomeu is one option, but I don't see why it is the best option. The unsplit patch was fully tested, and the 0.84 unsplit variant of the patch has had extensive testing in the OLPC XO-1 and XO-1.5 builds.
I don't feel terribly strong for splitting, if it's too much work at this point.
However, I have bad news for you: due to the update_badge() cleanup, your patch conflicts with #1610, which has already been landed on master. It seems that some rework will be needed anyway.
In dextrose, the order of these patches is reversed. Here's how I rebased #1610 on top of #1673:
@@ -259,15 +268,15 @@ class WirelessNetworkView(CanvasPulsingI self.props.base_color = self._color def _update_badge(self): - if network.find_connection_by_ssid(self._name) is not None: - self.props.badge_name = "emblem-favorite" - self._palette_icon.props.badge_name = "emblem-favorite" - elif self._flags == network.NM_802_11_AP_FLAGS_PRIVACY: - self.props.badge_name = "emblem-locked" - self._palette_icon.props.badge_name = "emblem-locked" - else: - self.props.badge_name = None - self._palette_icon.props.badge_name = None + self.props.badge_name = None + self._palette_icon.props.badge_name = None + if self._mode != network.NM_802_11_MODE_ADHOC: + if network.find_connection_by_ssid(self._name) is not None: + self.props.badge_name = "emblem-favorite" + self._palette_icon.props.badge_name = "emblem-favorite" + elif self._flags == network.NM_802_11_AP_FLAGS_PRIVACY: + self.props.badge_name = "emblem-locked" + self._palette_icon.props.badge_name = "emblem-locked" def _disconnect_activate_cb(self, item): connection = network.find_connection_by_ssid(self._name)
Sounds sane?
Are there any further suggestions? The new activity on this ticket is very encouraging. I guess it is because the 0.88 deployments are affected?
Yes, we'd like to have this in Uruguay.
comment:20 in reply to: ↑ 19 Changed 14 years ago by quozl
Replying to bernie:
However, I have bad news for you: due to the update_badge() cleanup, your patch conflicts with #1610, which has already been landed on master. It seems that some rework will be needed anyway.
Quite expected, and not a concern, but thanks anyway.
In destrose, [...]
Ah, good, you've solved by forking.
Yes, we'd like to have this in Uruguay.
I'll attach a patch that only fixes the discard network history function without trying to fix anything else.
Changed 14 years ago by quozl
comment:21 Changed 14 years ago by tonyforster
see also #2264, extra menu item - forget, or remove as favourite
comment:22 Changed 11 years ago by dnarvaez
- Resolution set to fixed
- Status changed from new to closed
Where is the patch?