Opened 9 years ago

Closed 6 years ago

Last modified 6 years ago

#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)

0001-restore-discard-network-history-function.patch (2.2 KB) - added by quozl 9 years ago.
Patch to Sugar 0.84 restores the discard network history button to normal operation. Review requested.
1673.patch (2.0 KB) - added by sascha_silbe 9 years ago.
restore discard network history function (quozl)
0001-fix-network-disconnect-and-discard-history.patch (23.2 KB) - added by quozl 9 years ago.
fix network disconnect and discard history, see also sugarlabs.org #1802, #1737, #1736, #1608, and laptop.org 9977 and 9788.
0001-fix-network-disconnect-and-discard-history.2.patch (22.6 KB) - added by quozl 9 years ago.
fix network disconnect and discard history, see also sugarlabs.org #1802, #1737, #1736, #1608, and laptop.org 9977 and 9788.
0001-fix-discard-history-1673-for-uruguay-consensus.patch (7.3 KB) - added by quozl 9 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 9 years ago by tomeu

Where is the patch?

comment:2 Changed 9 years ago by quozl

  • Keywords r? removed

Changed 9 years ago by quozl

Patch to Sugar 0.84 restores the discard network history button to normal operation. Review requested.

comment:3 Changed 9 years ago by quozl

  • Keywords r? added

comment:4 Changed 9 years ago by erikos

  • Keywords olpc-0.84 added

comment:5 Changed 9 years ago by erikos

  • Milestone changed from Unspecified by Release Team to 0.84

comment:6 Changed 9 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 9 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.

Changed 9 years ago by sascha_silbe

restore discard network history function (quozl)

comment:8 follow-up: Changed 9 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:

  1. Icon badges in the Neighborhood are not updated.
  2. The button has immediate effect, in contrast to the usual commit/reset style of control panel settings.

comment:9 in reply to: ↑ 8 Changed 9 years ago by quozl

Replying to sascha_silbe:

  1. Icon badges in the Neighborhood are not updated.

These are properly updated in my patch to 0.88. I shall attach it.

  1. 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 9 years ago by quozl

fix network disconnect and discard history, see also sugarlabs.org #1802, #1737, #1736, #1608, and laptop.org 9977 and 9788.

comment:10 Changed 9 years ago by quozl

  • Keywords r? added; r! olpc-0.84 removed

requesting review and merging of above patch.

comment:11 follow-ups: Changed 9 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 9 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 9 years ago by quozl

fix network disconnect and discard history, see also sugarlabs.org #1802, #1737, #1736, #1608, and laptop.org 9977 and 9788.

comment:13 in reply to: ↑ 11 ; follow-ups: Changed 9 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 9 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 9 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: Changed 9 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 9 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: Changed 9 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: Changed 9 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 9 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.

comment:21 Changed 9 years ago by tonyforster

see also #2264, extra menu item - forget, or remove as favourite

comment:22 Changed 6 years ago by dnarvaez

  • Resolution set to fixed
  • Status changed from new to closed

comment:23 Changed 6 years ago by dnarvaez

  • Milestone 0.88.x deleted

Milestone 0.88.x deleted

Note: See TracTickets for help on using tickets.