Merge lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-timer into lp:ubuntuone-client

[Diego Sarmentero]

- Using a timer event that doesn't fire continuously if another update timer is in progress (LP: #1052922).

[lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-timer]

1322. By Diego Sarmentero on 2012-09-20

merge

1323. By Diego Sarmentero on 2012-09-20

adding logging

1324. By Diego Sarmentero on 2012-09-20

fixing test

1325. By Diego Sarmentero on 2012-09-20

adding test for register_listener

[Alejandro J. Cura (alecu)]

This branch is wrong: it keeps using cpu while syncdaemon is idle.

review: Needs Fixing

[lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-timer]

1326. By Diego Sarmentero on 2012-09-21

Improving timer

1327. By Diego Sarmentero on 2012-09-21

merge

1328. By Diego Sarmentero on 2012-09-21

Timer delay improved

1329. By Diego Sarmentero on 2012-09-25

raise error when the object is not callable

1330. By Diego Sarmentero on 2012-09-25

tests fixed

[Mike McCracken (mikemc)]

OK. looks good - tests pass for me on linux, and there are no new tests to run on darwin or windows.
(I have a branch that'll add tests for windows and darwin)

review: Approve

[Mike McCracken (mikemc)]

I should've added - I couldn't IRL test this because I don't have Q running yet, and IRL on darwin dies because of the bug that my other branch fixes...

[Mike McCracken (mikemc)]

I just noticed this - this branch removes the method "start_timer" from the UbuntuOneSyncMenuLinux class, but not from the DummySyncMenu class in sync_menu/linux.py.
The test in test_linux.py tests the dummy for start_timer, so it also needs to be updated.

Don't worry about fixing the same issue in common.py, my branch will do that.

review: Needs Fixing

[lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-timer]

1331. By Diego Sarmentero on 2012-09-26

removing start_timer from dummy

1332. By Diego Sarmentero on 2012-09-26

adding missing update_transfers for dummy and test

[Mike McCracken (mikemc)]

Looks good.

review: Approve

[Alejandro J. Cura (alecu)]

A few quick fixes needed:

The name of test_dummy_has_start_timer is wrong.

The docstring in test_register_listener_fail is copied from the one above.

"listeners_callbacks" and "register_listener" are misnamed. They should be "progress_listeners" and "register_progress_listener".

This docstring is wrong: """Create the sync menu and run the loop.""". It should be """Create the sync menu and register the progress listener."""

review: Approve

[lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-timer]

1333. By Diego Sarmentero on 2012-09-26

fixing naming and docstring problems

=== modified file 'tests/platform/sync_menu/test_linux.py'
--- tests/platform/sync_menu/test_linux.py	2012-09-24 13:09:52 +0000
+++ tests/platform/sync_menu/test_linux.py	2012-09-26 20:23:20 +0000
@@ -28,6 +28,7 @@
 # files in the program, then also delete it here.
 """Test the Sync Menu."""
 
+import time
 from collections import Callable
 
 from twisted.internet import defer
@@ -57,18 +58,20 @@
         return self.uploading_data
 
 
-class FakeStatusListener(object):
-    """Fake StatusListener."""
-
-    def __init__(self):
-        self.status_frontend = FakeStatusFrontend()
-
-
-class FakeMain(object):
-    """Fake Main."""
-
-    def __init__(self):
-        self.status_listener = FakeStatusListener()
+class FakeTimer(object):
+    """Fake Timer."""
+
+    def __init__(self, delay):
+        self.delay = delay
+        self.callback = None
+
+    def addCallback(self, callback):
+        """Add callback."""
+        self.callback = callback
+
+
+class FakeSyncdaemonService(object):
+    """Fake SyncdaemonService."""
 
 
 class FakeSyncMenuApp(object):
@@ -101,10 +104,10 @@
         dummy = linux.DummySyncMenu('random', 'args')
         self.assertIsInstance(dummy, linux.DummySyncMenu)
 
-    def test_dummy_has_start_timer(self):
+    def test_dummy_has_update_transfers(self):
         """Check that the dummy has the proper methods required by the API."""
         dummy = linux.DummySyncMenu('random', 'args')
-        self.assertIsInstance(dummy.start_timer, Callable)
+        self.assertIsInstance(dummy.update_transfers, Callable)
 
 
 class SyncMenuTestCase(TestCase):
@@ -117,12 +120,13 @@
         yield super(SyncMenuTestCase, self).setUp()
         self.patch(linux.SyncMenu, "App", FakeSyncMenuApp)
         FakeSyncMenuApp.clean()
-        self.main = FakeMain()
-        self.status_frontend = self.main.status_listener.status_frontend
+        self.syncdaemon_service = FakeSyncdaemonService()
+        self.status_frontend = FakeStatusFrontend()
         self._paused = False
         self.patch(sync_menu.UbuntuOneSyncMenu, "change_sync_status",
             self._change_sync_status)
-        self.sync_menu = sync_menu.UbuntuOneSyncMenu(self.main)
+        self.sync_menu = sync_menu.UbuntuOneSyncMenu(self.status_frontend,
+            self.syncdaemon_service)
 
     def _change_sync_status(self, *args):
         """Fake change_sync_status."""
@@ -338,3 +342,14 @@
             self.assertEqual(item.property_get_int(
                 linux.SyncMenu.PROGRESS_MENUITEM_PROP_PERCENT_DONE),
                 percentage)
+
+    def test_update_transfers_delay(self):
+        """Check that the timer is being handle properly."""
+        self.patch(linux.status.aggregator, "Timer", FakeTimer)
+        self.sync_menu.next_update = time.time() + 3
+        self.sync_menu.update_transfers()
+        self.assertLess(self.sync_menu.timer.delay, 3)
+        self.sync_menu.timer = None
+        self.sync_menu.next_update = time.time() / 2
+        self.sync_menu.update_transfers()
+        self.assertEqual(self.sync_menu.timer.delay, 0)
 
=== modified file 'tests/status/test_aggregator.py'
--- tests/status/test_aggregator.py	2012-08-20 13:18:43 +0000
+++ tests/status/test_aggregator.py	2012-09-26 20:23:20 +0000
@@ -1314,6 +1314,21 @@
         self.assertEqual({}, self.aggregator.to_do)
         self.assertIdentical(None, self.aggregator.queue_done_timer)
 
+    def test_register_progress_listener(self):
+        """Check that register listener handles properly additions."""
+
+        def fake_callback():
+            """Do nothing."""
+
+        self.aggregator.register_progress_listener(fake_callback)
+        self.assertEqual(len(self.aggregator.progress_listeners), 1)
+
+    def test_register_progress_listener_fail(self):
+        """Check that register listener fails with not Callable objects."""
+        self.assertRaises(TypeError,
+            self.aggregator.register_progress_listener, [])
+        self.assertEqual(len(self.aggregator.progress_listeners), 0)
+
     def assertMiscCommandQueued(self, fc):
         """Assert that some command was queued."""
         self.assertEqual(len(self.aggregator.to_do), 1)
 
=== modified file 'tests/syncdaemon/test_main.py'
--- tests/syncdaemon/test_main.py	2012-09-19 17:23:02 +0000
+++ tests/syncdaemon/test_main.py	2012-09-26 20:23:20 +0000
@@ -70,19 +70,6 @@
         return defer.succeed(None)
 
 
-class FakedSyncMenu(object):
-    """Do nothing."""
-
-    def __init__(self, *args, **kwargs):
-        self.arguments = (args, kwargs)
-
-    def update_progress(self):
-        """Do nothing."""
-
-    def start_timer(self):
-        """Do nothing."""
-
-
 class MainTests(BaseTwistedTestCase):
     """ Basic tests to check main.Main """
 
@@ -100,7 +87,6 @@
         self.patch(main_mod.event_logging, "get_listener", lambda *a: None)
         # no status listener by default
         self.patch(main_mod.status_listener, "get_listener", lambda *a: None)
-        self.patch(main_mod.sync_menu, "UbuntuOneSyncMenu", FakedSyncMenu)
 
         self.handler = MementoHandler()
         self.handler.setLevel(logging.DEBUG)
 
=== modified file 'tests/syncdaemon/test_status_listener.py'
--- tests/syncdaemon/test_status_listener.py	2012-04-09 20:07:05 +0000
+++ tests/syncdaemon/test_status_listener.py	2012-09-26 20:23:20 +0000
@@ -84,7 +84,7 @@
                     callback(args)
 
     listener = Listener()
-    setattr(listener, 'handle_'+event, listener._handle_event)
+    setattr(listener, 'handle_' + event, listener._handle_event)
     event_q.subscribe(listener)
     return listener
 
@@ -92,7 +92,7 @@
 class FakeStatusFrontend(object):
     """A fake status frontend."""
 
-    def __init__(self):
+    def __init__(self, *args, **kwargs):
         """Initialize this instance."""
         self.call_log = []
 
 
=== modified file 'ubuntuone/platform/sync_menu/common.py'
--- ubuntuone/platform/sync_menu/common.py	2012-09-18 16:29:22 +0000
+++ ubuntuone/platform/sync_menu/common.py	2012-09-26 20:23:20 +0000
@@ -31,13 +31,3 @@
 
 class UbuntuOneSyncMenu(object):
     """Integrate U1 with the Ubuntu Sync Menu."""
-
-    def __init__(self, *args):
-        self.transfers = DummyTransfersMenu()
-
-
-class DummyTransfersMenu(object):
-    """Dummy Transfers Menu."""
-
-    def start_timer(self):
-        """Empty start timer, this is not needed on windows."""
 
=== modified file 'ubuntuone/platform/sync_menu/linux.py'
--- ubuntuone/platform/sync_menu/linux.py	2012-09-24 12:47:05 +0000
+++ ubuntuone/platform/sync_menu/linux.py	2012-09-26 20:23:20 +0000
@@ -29,6 +29,8 @@
 """Use SyncMenu lib to integrate U1 with the Systray Sync Icon."""
 
 import gettext
+import logging
+import time
 import sys
 import webbrowser
 
@@ -50,10 +52,12 @@
     use_syncmenu = True
 except:
     use_syncmenu = False
-from twisted.internet import reactor
 
 from ubuntuone.clientdefs import GETTEXT_PACKAGE
-
+from ubuntuone import status
+
+
+logger = logging.getLogger("ubuntuone.platform.SyncMenu")
 
 Q_ = lambda string: gettext.dgettext(GETTEXT_PACKAGE, string)
 
@@ -63,6 +67,7 @@
 MORE_STORAGE = Q_("Get More Space")
 GET_HELP = Q_("Get Help on the Web")
 
+DELAY_BETWEEN_UPDATES = 3
 UBUNTUONE_LINK = u'https://one.ubuntu.com/'
 DASHBOARD = UBUNTUONE_LINK + u'dashboard/'
 HELP_LINK = UBUNTUONE_LINK + u'support/'
@@ -72,11 +77,12 @@
 class UbuntuOneSyncMenuLinux(object):
     """Integrate U1 with the Ubuntu Sync Menu."""
 
-    def __init__(self, main):
+    def __init__(self, status, syncdaemon_service):
         """Initialize menu."""
-        self._main = main
-        self.status_listener = main.status_listener
+        self._syncdaemon_service = syncdaemon_service
         self._paused = False
+        self.timer = None
+        self.next_update = time.time()
         self.root_menu = Dbusmenu.Menuitem()
 
         self.open_u1 = Dbusmenu.Menuitem()
@@ -86,7 +92,7 @@
         self.go_to_web.property_set(Dbusmenu.MENUITEM_PROP_LABEL,
             GO_TO_WEB)
 
-        self.transfers = TransfersMenu(self.status_listener)
+        self.transfers = TransfersMenu(status)
         self.transfers.property_set(Dbusmenu.MENUITEM_PROP_LABEL,
             TRANSFERS)
 
@@ -121,17 +127,13 @@
         self.app.set_menu(self.server)
         self.app.connect("notify::paused", self.change_sync_status)
 
-    def start_timer(self):
-        """Start the transfers timer."""
-        self.transfers.start_timer()
-
     def change_sync_status(self, *args):
         """Triggered when the sync status is changed fromm the menu."""
         if self._paused:
-            self._main.external.connect()
+            self._syncdaemon_service.connect()
             self._paused = False
         else:
-            self._main.external.disconnect()
+            self._syncdaemon_service.disconnect()
             self._paused = True
 
     def open_control_panel(self, *args):
@@ -150,34 +152,45 @@
         """Open the Ubunto One Help Page"""
         webbrowser.open(GET_STORAGE_LINK)
 
+    def _timeout(self, result):
+        """The aggregating timer has expired, so update the UI."""
+        self.next_update = int(time.time()) + DELAY_BETWEEN_UPDATES
+        self.transfers.update_progress()
+        self.timer = None
+
+    def update_transfers(self):
+        """Set up a timer if there isn't one ticking and update the ui."""
+        if not self.timer:
+            logger.debug("Updating Transfers.")
+            delay = int(max(0, min(DELAY_BETWEEN_UPDATES,
+                self.next_update - time.time())))
+            self.timer = status.aggregator.Timer(delay)
+            self.timer.addCallback(self._timeout)
+
 
 class TransfersMenu(Dbusmenu.Menuitem):
     """Menu that handles the recent and current transfers."""
 
-    def __init__(self, listener):
+    def __init__(self, status_frontend):
         super(TransfersMenu, self).__init__()
-        self.listener = listener
+        self.status_frontend = status_frontend
         self.uploading = {}
         self.previous_transfers = []
         self._transfers_items = {}
         self._uploading_items = {}
         self.separator = None
 
-    def start_timer(self):
-        """Trigger an update in one second."""
-        self.update_progress()
-        reactor.callLater(3, self.start_timer)
-
     def update_progress(self):
         """Update the list of recent transfers and current transfers."""
-        current_transfers = self.listener.status_frontend.recent_transfers()
+        current_transfers = self.status_frontend.recent_transfers()
         uploading_data = {}
         for filename, size, written in \
-            self.listener.status_frontend.files_uploading():
+            self.status_frontend.files_uploading():
             uploading_data[filename] = (size, written)
 
         temp_transfers = {}
         if current_transfers != self.previous_transfers:
+            logger.debug("Update recent transfers with: %r", current_transfers)
             for item_transfer in self._transfers_items:
                 self.child_delete(self._transfers_items[item_transfer])
             for item in current_transfers:
@@ -204,6 +217,8 @@
                 upload_item.property_set_int(
                     SyncMenu.PROGRESS_MENUITEM_PROP_PERCENT_DONE,
                     percentage)
+                logger.debug("Current transfer %s progress update: %r",
+                    item, percentage)
                 items_added += 1
             else:
                 self.child_delete(self._uploading_items[item])
@@ -223,6 +238,7 @@
                     uploading_file.property_set_int(
                         SyncMenu.PROGRESS_MENUITEM_PROP_PERCENT_DONE,
                         percentage)
+                    logger.debug("Current transfer %s created", item)
                     self.child_append(uploading_file)
                     self._uploading_items[item] = uploading_file
                     items_added += 1
@@ -234,8 +250,7 @@
     def __init__(self, *args, **kwargs):
         """Initialize menu."""
 
-    def start_timer(self):
+    def update_transfers(self):
         """Do nothing."""
 
-
 UbuntuOneSyncMenu = UbuntuOneSyncMenuLinux if use_syncmenu else DummySyncMenu
 
=== modified file 'ubuntuone/status/aggregator.py'
--- ubuntuone/status/aggregator.py	2012-08-16 12:00:12 +0000
+++ ubuntuone/status/aggregator.py	2012-09-26 20:23:20 +0000
@@ -33,7 +33,7 @@
 import itertools
 import operator
 import os
-from collections import deque
+from collections import deque, Callable
 
 import gettext
 
@@ -41,7 +41,11 @@
 
 from ubuntuone.clientdefs import GETTEXT_PACKAGE
 from ubuntuone.status.logger import logger
-from ubuntuone.platform import session, notification
+from ubuntuone.platform import (
+    notification,
+    session,
+    sync_menu
+)
 from ubuntuone.platform.messaging import Messaging
 from ubuntuone.platform.launcher import UbuntuOneLauncher, DummyLauncher
 
@@ -625,6 +629,7 @@
         self.progress = {}
         self.to_do = {}
         self.recent_transfers = deque(maxlen=5)
+        self.progress_listeners = []
 
     def get_notification(self):
         """Create a new toggleable notification object."""
@@ -655,6 +660,13 @@
         self.to_do = {}
     # pylint: enable=W0201
 
+    def register_progress_listener(self, listener):
+        """Register a callable object to be notified."""
+        if isinstance(listener, Callable):
+            self.progress_listeners.append(listener)
+        else:
+            raise TypeError("Callable object expected.")
+
     def get_discovery_message(self):
         """Get the text for the discovery bubble."""
         lines = []
@@ -716,6 +728,8 @@
             progress = float(
                 sum(self.progress.values())) / sum(self.to_do.values())
             self.progress_bar.set_progress(progress)
+        for listener in self.progress_listeners:
+            listener()
 
     def download_started(self, command):
         """A download just started."""
@@ -801,13 +815,25 @@
 class StatusFrontend(object):
     """Frontend for the status aggregator, used by the StatusListener."""
 
-    def __init__(self, clock=reactor):
+    def __init__(self, clock=reactor, service=None):
         """Initialize this instance."""
         self.aggregator = StatusAggregator(clock=clock)
         self.notification = self.aggregator.get_notification()
         self.messaging = Messaging()
         self.quota_timer = None
 
+        self.syncdaemon_service = service
+        self.sync_menu = None
+        self.start_sync_menu()
+
+    def start_sync_menu(self):
+        """Create the sync menu and register the progress listener."""
+        if self.syncdaemon_service is not None:
+            self.sync_menu = sync_menu.UbuntuOneSyncMenu(self,
+                self.syncdaemon_service)
+            self.aggregator.register_progress_listener(
+                self.sync_menu.update_transfers)
+
     def recent_transfers(self):
         """Return a tuple with the recent transfers paths."""
         return list(self.aggregator.recent_transfers)
 
=== modified file 'ubuntuone/syncdaemon/main.py'
--- ubuntuone/syncdaemon/main.py	2012-09-19 17:23:02 +0000
+++ ubuntuone/syncdaemon/main.py	2012-09-26 20:23:20 +0000
@@ -48,10 +48,7 @@
     volume_manager,
 )
 from ubuntuone import syncdaemon, clientdefs
-from ubuntuone.platform import (
-    event_logging,
-    sync_menu,
-)
+from ubuntuone.platform import event_logging
 from ubuntuone.syncdaemon import status_listener
 from ubuntuone.syncdaemon.interaction_interfaces import SyncdaemonService
 from ubuntuone.syncdaemon.states import StateManager, QueueManager
@@ -159,14 +156,6 @@
         self.mark = task.LoopingCall(self.log_mark)
         self.mark.start(mark_interval)
 
-        self.sync_menu = None
-        self.start_sync_menu()
-
-    def start_sync_menu(self):
-        """Create the sync menu and run the loop."""
-        self.sync_menu = sync_menu.UbuntuOneSyncMenu(self)
-        self.sync_menu.start_timer()
-
     def start_event_logger(self):
         """Start the event logger if it's available for this platform."""
         self.eventlog_listener = event_logging.get_listener(self.fs, self.vm)
@@ -176,7 +165,8 @@
 
     def start_status_listener(self):
         """Start the status listener if it is configured to start."""
-        self.status_listener = status_listener.get_listener(self.fs, self.vm)
+        self.status_listener = status_listener.get_listener(self.fs, self.vm,
+            self.external)
         # subscribe to EQ, to be unsubscribed in shutdown
         if self.status_listener:
             self.event_q.subscribe(self.status_listener)
 
=== modified file 'ubuntuone/syncdaemon/status_listener.py'
--- ubuntuone/syncdaemon/status_listener.py	2012-08-10 12:49:46 +0000
+++ ubuntuone/syncdaemon/status_listener.py	2012-09-26 20:23:20 +0000
@@ -48,10 +48,10 @@
     return True
 
 
-def get_listener(fsm, vm):
+def get_listener(fsm, vm, syncdaemon_service=None):
     """Return an instance of the status listener, or None if turned off."""
     if should_start_listener():
-        status_frontend = StatusFrontend()
+        status_frontend = StatusFrontend(service=syncdaemon_service)
         return StatusListener(fsm, vm, status_frontend)
     else:
         return None