Merge lp:~chipaca/ubuntuone-client/branch-of-glowing-plus-three-hateage into lp:ubuntuone-client

[John Lenton]

This fixes several long-standing bugs in ubuntuone-preferences.

This fixes several long-standing bugs in ubuntuone-preferences.

[Eric Casteleijn (thisfred)]

Tests pass, syncdaemon and u1prefs work.

review: Approve

[dobey (dobey)]

review: Approve

[Rick McBride (rmcbride)]

nice

review: Approve

[dobey (dobey)]

The attempt to merge lp:~chipaca/ubuntuone-client/branch-of-glowing-plus-three-hateage into lp:ubuntuone-client failed.Below is the output from the failed tests.

/usr/bin/gnome-autogen.sh
checking for autoconf >= 2.53...
(B testing autoconf2.50... not found.
  testing autoconf... found 2.65
checking for automake >= 1.10...
(B testing automake-1.11... found 1.11.1
checking for libtool >= 1.5...
(B testing libtoolize... found 2.2.6b
checking for intltool >= 0.30...
(B testing intltoolize... found 0.41.0
checking for pkg-config >= 0.14.0...
(B testing pkg-config... found 0.22
Checking for required M4 macros...
(BChecking for forbidden M4 macros...
(BProcessing ./configure.ac
(BRunning libtoolize...
(Blibtoolize: putting auxiliary files in `.'.
libtoolize: copying file `./ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
Running intltoolize...
(BRunning aclocal-1.11...
(BRunning autoconf...
(BRunning autoheader...
(BRunning automake-1.11...
(BRunning ./configure --prefix=/usr ...
(Bchecking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking for style of include used by make... GNU
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking dependency style of gcc... gcc3
checking for library containing strerror... none required
checking for gcc... (cached) gcc
checking whether we are using the GNU C compiler... (cached) yes
checking whether gcc accepts -g... (cached) yes
checking for gcc option to accept ISO C89... (cached) none needed
checking dependency style of gcc... (cached) gcc3
checking build system type... i686-pc-linux-gnu
checking host system type... i686-pc-linux-gnu
checking for a sed that does not truncate output... /bin/sed
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for fgrep... /bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 1572864
checking whether the shell understands some XSI constructs... yes
checking whether the shell understands "+="... yes
checking for /usr/bin/ld option to reload object files... -r
checki...

[lp:~chipaca/ubuntuone-client/branch-of-glowing-plus-three-hateage]

501. By John Lenton on 2010-04-14

twisted defer lint

=== modified file 'bin/ubuntuone-preferences'
--- bin/ubuntuone-preferences	2010-04-09 15:09:21 +0000
+++ bin/ubuntuone-preferences	2010-04-14 23:56:26 +0000
@@ -220,21 +220,10 @@
         """
         Push the bandwidth settings at syncdaemon.
         """
-        try:
-            client = self.bus.get_object(DBUS_IFACE_NAME, "/config",
-                                         follow_name_owner_changes=True)
-            iface = dbus.Interface(client, DBUS_IFACE_CONFIG_NAME)
-            iface.set_throttling_limits(self.dn_limit, self.up_limit,
-                                        reply_handler=dbus_async,
-                                        error_handler=self.error)
-            if self.bw_limited:
-                iface.enable_bandwidth_throttling(reply_handler=dbus_async,
-                                                  error_handler=self.error)
-            else:
-                iface.disable_bandwidth_throttling(reply_handler=dbus_async,
-                                                   error_handler=self.error)
-        except DBusException, e:
-            self.error(str(e))
+        if self.sdtool.is_files_sync_enabled():
+            self.sdtool.set_throttling_limits(self.dn_limit or -1,
+                                              self.up_limit or -1)
+            self.sdtool.enable_throttling(self.bw_limited)
 
     def handle_bw_controls_changed(self, *a):
         """
@@ -242,6 +231,8 @@
 
         Start a timer to sync with syncdaemon too.
         """
+        if not self.sdtool.is_files_sync_enabled():
+            return
         # Remove the timeout ...
         if self._update_id != 0:
             gobject.source_remove(self._update_id)
@@ -307,9 +298,15 @@
 
         This might be better as an error dialog.
         """
-        self.clear_devices_view()
-        self.status_label.set_markup("<b>Error:</b> %s" % msg)
         logger.error(msg)
+        dialog = gtk.MessageDialog(self.get_toplevel(),
+                                   gtk.DIALOG_DESTROY_WITH_PARENT | gtk.MODAL,
+                                   gtk.MESSAGE_ERROR, gtk.BUTTONS_CLOSE,
+                                   'Error')
+        dialog.format_secondary_text(str(msg))
+        dialog.run()
+        while gtk.events_pending():
+            gtk.main_iteration()
 
     def get_devices(self):
         """
@@ -377,19 +374,22 @@
 
         token = get_access_token(self.keyring)
 
+        fsync_enabled = self.sdtool.is_files_sync_enabled()
+
         if not self.devices:
-            # a stopgap device so you can at least try to connect
-            self.devices = [{'kind': 'Computer',
-                             'description': _("<LOCAL MACHINE>"),
-                             'token': token.key if token else '',
-                             'FAKE': 'YES'}]
+            if fsync_enabled:
+                # a stopgap device so you can at least try to connect
+                self.devices = [{'kind': 'Computer',
+                                 'description': _("<LOCAL MACHINE>"),
+                                 'token': token.key if token else '',
+                                 'FAKE': 'YES'}]
         else:
             self.resize(len(self.devices)+1, 3)
 
         self.status_label.set_label("")
 
         i = 0
-        for row in self.devices:
+        for row in self.devices or ():
             i += 1
             img = gtk.Image()
             img.set_from_icon_name(row['kind'].lower(), gtk.ICON_SIZE_DND)
@@ -403,7 +403,8 @@
                 butn.connect('clicked', self.remove,
                              row['kind'], row.get('token'))
                 self.attach(butn, 2, 3, i, i+1, xoptions=0, yoptions=0)
-            if token and row.get('token') == token.key or 'FAKE' in row:
+            if ((token and row.get('token') == token.key or 'FAKE' in row)
+                and fsync_enabled):
                 self.bw_chk = ck_btn = gtk.CheckButton(
                     _("_Limit Bandwidth Usage"))
                 ck_btn.set_active(self.bw_limited)
@@ -427,8 +428,9 @@
                 dn_lbl.set_mnemonic_widget(dn_btn)
                 ck_btn.connect('toggled', self.handle_bw_checkbox_toggled,
                                up_lbl, up_btn, dn_lbl, dn_btn)
-                self.handle_bw_checkbox_toggled(ck_btn,
-                                                up_lbl, up_btn, dn_lbl, dn_btn)
+                if fsync_enabled:
+                    self.handle_bw_checkbox_toggled(ck_btn, up_lbl, up_btn,
+                                                    dn_lbl, dn_btn)
 
                 self.conn_btn = gtk.Button(_("Connect"))
                 if self.connected is None:
@@ -532,6 +534,17 @@
 
         # SD Tool object
         self.sdtool = SyncDaemonTool(self.__bus)
+        if self.sdtool.is_files_sync_enabled():
+            self._hookup_dbus()
+        else:
+            self.status_label.set_text(_("Disconnected"))
+
+        logger.debug("starting")
+
+    def _hookup_dbus(self):
+        """
+        Hook up dbus
+        """
         self.sdtool.get_status().addCallbacks(self.__got_state,
                                               self.__sd_error)
 
@@ -554,7 +567,6 @@
         except DBusException, e:
             self.__dbus_error(e)
 
-        logger.debug("starting")
 
     def __dbus_error(self, error):
         """Error getting throttling config."""
@@ -595,6 +607,8 @@
     def __handle_response(self, dialog, response):
         """Handle the dialog's response."""
         self.hide()
+        while gtk.events_pending():
+            gtk.main_iteration()
         self.devices.handle_bw_controls_changed()
         gobject.timeout_add_seconds(5, gtk.main_quit)
 
@@ -767,6 +781,10 @@
         self.sdtool.enable_files_sync(enabled).addCallbacks(
             lambda _: dbus_async,
             self.__sd_error)
+        def sd_enabled(_):
+            self._hookup_dbus()
+            self.sdtool.connect()
+            self.devices.list_devices()
         def sd_error(error):
             self.files_check.set_active(False)
             self.files_check.set_sensitive(False)
@@ -774,18 +792,22 @@
             self.__sd_error(error)
 
         if enabled:
-            self.sdtool.start().addErrback(sd_error)
+            self.sdtool.start().addCallbacks(sd_enabled,
+                                             sd_error)
             if self.ums_id:
                 self.music_check.set_sensitive(True)
                 if self.music_check.get_active():
                     self.__music_check_toggled(self.music_check)
         else:
-            self.sdtool.quit()
+            self.sdtool.quit().addCallback(
+                lambda _: self.devices.list_devices())
             self.music_check.set_sensitive(False)
 
     def music_check_toggled(self, checkbutton):
         """Handle toggling the music download service."""
         if not self.files_check.get_active() or not self.ums_id:
+            checkbutton.set_sensitive(False)
+            checkbutton.set_active(False)
             return
         enabled = checkbutton.get_active()
         def got_error(error):
@@ -801,11 +823,6 @@
 
     def get_syncdaemon_sync_config(self):
         """Update the files/music sync config from syncdaemon."""
-        def got_fs_err(error):
-            self.files_check.set_sensitive(False)
-            self.music_check.set_sensitive(False)
-            self.__sd_error(error)
-
         def got_ms_err(error):
             self.music_check.set_sensitive(False)
             self.__sd_error(error)
@@ -822,11 +839,8 @@
                 else:
                     self.music_check.set_active(False)
 
-        def got_fs(enabled):
-            self.files_check.set_active(enabled)
-
         self.sdtool.get_folder_info(U1MSPATH).addCallbacks(got_info, got_ms_err)
-        self.sdtool.is_files_sync_enabled().addCallbacks(got_fs, got_fs_err)
+        self.files_check.set_active(self.sdtool.is_files_sync_enabled())
 
     def connect_file_sync_callbacks(self):
         """Connect the file sync checkbox callbacks."""
@@ -1018,7 +1032,7 @@
         fbox.show()
 
         self.files_check = gtk.CheckButton(_("_File Synchronization"))
-        self.files_check.set_active(True)
+        self.files_check.set_active(False)
         fbox.pack_start(self.files_check, False, False)
         self.files_check.show()
 
@@ -1029,6 +1043,7 @@
 
         self.music_check = gtk.CheckButton(_("_Music Download"))
         self.music_check.set_active(True)
+        self.music_check.set_sensitive(False)
         alignment.add(self.music_check)
         self.music_check.show()
 
@@ -1058,7 +1073,8 @@
         self.dialog.request_quota_info()
         self.dialog.request_account_info()
         self.dialog.devices.get_devices()
-        if self.dialog.visible:
+        # watch it! jaunty has no 'get_visible' method
+        if self.dialog.get_property('visible'):
             self.dialog.present_with_time(int(time.time()))
 
     def got_newcredentials(self, realm, consumer_key):

=== modified file 'tests/test_preferences.py'
--- tests/test_preferences.py	2010-03-30 20:17:54 +0000
+++ tests/test_preferences.py	2010-04-14 23:56:26 +0000
@@ -25,12 +25,34 @@
 from contrib.testing.testcase import DBusTwistedTestCase, FakeLogin
 from twisted.internet import defer
 from twisted.python.failure import Failure
-from ubuntuone.syncdaemon import dbus_interface
+from ubuntuone.syncdaemon import dbus_interface, tools
+
+class SyncDaemonTool(tools.SyncDaemonTool):
+    """A subclass of tools.SyncDaemonTool that keep track of the deferreds."""
+
+    def __init__(self, *args, **kwargs):
+        super(SyncDaemonTool, self).__init__(*args, **kwargs)
+        self._deferreds = set()
+
+    def __getattribute__(self, attr):
+        attribute = super(SyncDaemonTool, self).__getattribute__(attr)
+        import inspect
+        if inspect.ismethod(attribute) or inspect.isfunction(attribute):
+            def wrapper(*args, **kwargs):
+                value = attribute(*args, **kwargs)
+                if isinstance(value, defer.Deferred):
+                    self._deferreds.add(value)
+                return value
+            return wrapper
+        else:
+            return attribute
+
 
 class InvalidSignalError(Exception):
     """Exception for when we get the wrong signal called."""
     pass
 
+
 class PreferencesTests(MockerTestCase, DBusTwistedTestCase):
     """Basic tests for the ubuntuone-preferences app."""
 
@@ -72,7 +94,9 @@
         self.mocker.result(None)
 
         self.u1prefs.make_rest_request = self.make_rest_request
+        self.u1prefs.SyncDaemonTool = SyncDaemonTool
 
+    @defer.inlineCallbacks
     def tearDown(self):
         # collect all signal receivers registered during the test
         signal_receivers = set()
@@ -81,13 +105,10 @@
                 for matches in group.values():
                     for match in matches.values():
                         signal_receivers.update(match)
-        d = self.cleanup_signal_receivers(signal_receivers)
-        def shutdown(r):
-            self.oauth.shutdown()
-            dbus_interface.DBUS_PATH_AUTH = self._old_path
-        d.addBoth(shutdown)
-        d.addBoth(lambda _: DBusTwistedTestCase.tearDown(self))
-        return d
+        yield self.cleanup_signal_receivers(signal_receivers)
+        self.oauth.shutdown()
+        dbus_interface.DBUS_PATH_AUTH = self._old_path
+        yield DBusTwistedTestCase.tearDown(self)
 
     def make_rest_request(self, url=None, method='GET',
                           callback=None, keyring=None):
@@ -213,6 +234,7 @@
         dialog.update_quota_display(1024, 2048)
         self.assertEqual(dialog.usage_graph.get_fraction(), 0.5)
         dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_request_quota_info(self):
         """Test that we can request the quota info properly."""
@@ -224,6 +246,7 @@
         dialog.request_quota_info()
         self.assertEqual(dialog.usage_graph.get_fraction(), 0.5)
         dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_no_overquota_notice_on_low_usage(self):
         """Test that the quota notice is not visible if usage is low."""
@@ -237,6 +260,7 @@
         # and the icon should be blank, too
         self.assertEqual(dialog.overquota_img.get_icon_name()[0], None)
         dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_overquota_notice_and_upgrade_offer_on_free_high_usage(self):
         """Test that the quota notice is visible if usage is 95%.
@@ -257,6 +281,7 @@
         self.assertEqual(dialog.overquota_img.get_icon_name()[0],
                          'dialog-information')
         dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_overquota_notice_and_not_upgrade_offer_on_paid_high_usage(self):
         """Test that the quota notice is visible if usage is 95%.
@@ -278,6 +303,7 @@
         self.assertEqual(dialog.overquota_img.get_icon_name()[0],
                          'dialog-information')
         dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_overquota_warning_and_upgrade_offer_there_on_free_over_quota(self):
         """Test that the quota notice is visible if usage is 100%.
@@ -298,6 +324,7 @@
         self.assertEqual(dialog.overquota_img.get_icon_name()[0],
                          'dialog-warning')
         dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_overquota_warning_there_on_paid_over_quota(self):
         """Test that the quota notice is visible if usage is 100%."""
@@ -313,6 +340,7 @@
         self.assertEqual(dialog.overquota_img.get_icon_name()[0],
                          'dialog-warning')
         dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_request_account_info(self):
         """Test that we can request the account info properly."""
@@ -336,6 +364,7 @@
         self.assertEqual(dialog.plan_label.get_text(), 'Paid')
         self.assertFalse(dialog.upgrade_link.get_visible())
         dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_toggle_bookmarks(self):
         """Test toggling the bookmarks service on/off."""
@@ -353,6 +382,8 @@
         self.assertFalse(dialog.bookmarks_check.get_active())
         dialog.bookmarks_check.set_active(True)
         self.assertTrue(dialog.bookmarks_check.get_active())
+        dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_toggle_contacts(self):
         """Test toggling the contacts service on/off."""
@@ -370,6 +401,9 @@
         self.assertFalse(dialog.abook_check.get_active())
         dialog.abook_check.set_active(True)
         self.assertTrue(dialog.abook_check.get_active())
+        dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
+
 
     def test_toggle_files(self):
         """Test toggling the files service on/off."""
@@ -382,6 +416,8 @@
         self.assertFalse(dialog.files_check.get_active())
         dialog.files_check.set_active(True)
         self.assertTrue(dialog.files_check.get_active())
+        dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_toggle_files_and_music(self):
         """Test toggling the files and music services on/off."""
@@ -417,6 +453,8 @@
         dialog.files_check.set_active(False)
         self.assertFalse(dialog.files_check.get_active())
         self.assertFalse(dialog.music_check.props.sensitive)
+        dialog.destroy()
+        return defer.DeferredList(dialog.sdtool._deferreds)
 
     def test_login_check(self):
         """Test that our login check works correctly."""

=== modified file 'ubuntuone/syncdaemon/tools.py'
--- ubuntuone/syncdaemon/tools.py	2010-04-05 16:51:53 +0000
+++ ubuntuone/syncdaemon/tools.py	2010-04-14 23:56:26 +0000
@@ -31,6 +31,7 @@
     DBUS_IFACE_PUBLIC_FILES_NAME,
     DBUS_IFACE_CONFIG_NAME,
 )
+from ubuntuone.syncdaemon.config import get_user_config
 from dbus.lowlevel import SignalMessage, MethodCallMessage, ErrorMessage
 from dbus.exceptions import DBusException
 from twisted.internet import defer, reactor
@@ -704,25 +705,27 @@
     def is_files_sync_enabled(self):
         """Check if files sync is enabled."""
         self.log.debug('is_files_sync_enabled')
-        d = defer.Deferred()
-        config_client = DBusClient(self.bus, '/config',
-                                   DBUS_IFACE_CONFIG_NAME)
-        config_client.call_method('files_sync_enabled',
-                                  reply_handler=d.callback,
-                                  error_handler=d.errback)
-        return d
+        return get_user_config().get_files_sync_enabled()
 
     def enable_files_sync(self, enabled):
         """Enable/disable files sync."""
         self.log.debug('enable_files_sync %d', enabled)
-        d = defer.Deferred()
-        config_client = DBusClient(self.bus, '/config',
-                                   DBUS_IFACE_CONFIG_NAME)
-        config_client.call_method('set_files_sync_enabled',
-                                  enabled,
-                                  reply_handler=d.callback,
-                                  error_handler=d.errback)
-        return d
+        config = get_user_config()
+        if config.get_files_sync_enabled():
+            d = defer.Deferred()
+            config_client = DBusClient(self.bus, '/config',
+                                       DBUS_IFACE_CONFIG_NAME)
+            config_client.call_method('set_files_sync_enabled',
+                                      enabled,
+                                      reply_handler=d.callback,
+                                      error_handler=d.errback)
+            return d
+        else:
+            if enabled:
+                config.set_files_sync_enabled(True)
+                config.save()
+                self.start()
+            return defer.succeed(None)
 
 
 # callbacks used by u1sdtool script