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

Proposed by Diego Sarmentero
Status: Merged
Approved by: Diego Sarmentero
Approved revision: 1333
Merged at revision: 1325
Proposed branch: lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-timer
Merge into: lp:ubuntuone-client
Diff against target: 492 lines (+121/-84)
9 files modified
tests/platform/sync_menu/test_linux.py (+32/-17)
tests/status/test_aggregator.py (+15/-0)
tests/syncdaemon/test_main.py (+0/-14)
tests/syncdaemon/test_status_listener.py (+2/-2)
ubuntuone/platform/sync_menu/common.py (+0/-10)
ubuntuone/platform/sync_menu/linux.py (+38/-23)
ubuntuone/status/aggregator.py (+29/-3)
ubuntuone/syncdaemon/main.py (+3/-13)
ubuntuone/syncdaemon/status_listener.py (+2/-2)
To merge this branch: bzr merge lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-timer
Reviewer Review Type Date Requested Status
Alejandro J. Cura (community) Approve
Mike McCracken (community) Approve
Review via email: mp+125499@code.launchpad.net

Commit message

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

To post a comment you must log in.
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) wrote :

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

review: Needs Fixing
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) wrote :

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) wrote :

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) wrote :

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
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) wrote :

Looks good.

review: Approve
Alejandro J. Cura (alecu) wrote :

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
1333. By Diego Sarmentero on 2012-09-26

fixing naming and docstring problems

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tests/platform/sync_menu/test_linux.py'
2--- tests/platform/sync_menu/test_linux.py 2012-09-24 13:09:52 +0000
3+++ tests/platform/sync_menu/test_linux.py 2012-09-26 20:23:20 +0000
4@@ -28,6 +28,7 @@
5 # files in the program, then also delete it here.
6 """Test the Sync Menu."""
7
8+import time
9 from collections import Callable
10
11 from twisted.internet import defer
12@@ -57,18 +58,20 @@
13 return self.uploading_data
14
15
16-class FakeStatusListener(object):
17- """Fake StatusListener."""
18-
19- def __init__(self):
20- self.status_frontend = FakeStatusFrontend()
21-
22-
23-class FakeMain(object):
24- """Fake Main."""
25-
26- def __init__(self):
27- self.status_listener = FakeStatusListener()
28+class FakeTimer(object):
29+ """Fake Timer."""
30+
31+ def __init__(self, delay):
32+ self.delay = delay
33+ self.callback = None
34+
35+ def addCallback(self, callback):
36+ """Add callback."""
37+ self.callback = callback
38+
39+
40+class FakeSyncdaemonService(object):
41+ """Fake SyncdaemonService."""
42
43
44 class FakeSyncMenuApp(object):
45@@ -101,10 +104,10 @@
46 dummy = linux.DummySyncMenu('random', 'args')
47 self.assertIsInstance(dummy, linux.DummySyncMenu)
48
49- def test_dummy_has_start_timer(self):
50+ def test_dummy_has_update_transfers(self):
51 """Check that the dummy has the proper methods required by the API."""
52 dummy = linux.DummySyncMenu('random', 'args')
53- self.assertIsInstance(dummy.start_timer, Callable)
54+ self.assertIsInstance(dummy.update_transfers, Callable)
55
56
57 class SyncMenuTestCase(TestCase):
58@@ -117,12 +120,13 @@
59 yield super(SyncMenuTestCase, self).setUp()
60 self.patch(linux.SyncMenu, "App", FakeSyncMenuApp)
61 FakeSyncMenuApp.clean()
62- self.main = FakeMain()
63- self.status_frontend = self.main.status_listener.status_frontend
64+ self.syncdaemon_service = FakeSyncdaemonService()
65+ self.status_frontend = FakeStatusFrontend()
66 self._paused = False
67 self.patch(sync_menu.UbuntuOneSyncMenu, "change_sync_status",
68 self._change_sync_status)
69- self.sync_menu = sync_menu.UbuntuOneSyncMenu(self.main)
70+ self.sync_menu = sync_menu.UbuntuOneSyncMenu(self.status_frontend,
71+ self.syncdaemon_service)
72
73 def _change_sync_status(self, *args):
74 """Fake change_sync_status."""
75@@ -338,3 +342,14 @@
76 self.assertEqual(item.property_get_int(
77 linux.SyncMenu.PROGRESS_MENUITEM_PROP_PERCENT_DONE),
78 percentage)
79+
80+ def test_update_transfers_delay(self):
81+ """Check that the timer is being handle properly."""
82+ self.patch(linux.status.aggregator, "Timer", FakeTimer)
83+ self.sync_menu.next_update = time.time() + 3
84+ self.sync_menu.update_transfers()
85+ self.assertLess(self.sync_menu.timer.delay, 3)
86+ self.sync_menu.timer = None
87+ self.sync_menu.next_update = time.time() / 2
88+ self.sync_menu.update_transfers()
89+ self.assertEqual(self.sync_menu.timer.delay, 0)
90
91=== modified file 'tests/status/test_aggregator.py'
92--- tests/status/test_aggregator.py 2012-08-20 13:18:43 +0000
93+++ tests/status/test_aggregator.py 2012-09-26 20:23:20 +0000
94@@ -1314,6 +1314,21 @@
95 self.assertEqual({}, self.aggregator.to_do)
96 self.assertIdentical(None, self.aggregator.queue_done_timer)
97
98+ def test_register_progress_listener(self):
99+ """Check that register listener handles properly additions."""
100+
101+ def fake_callback():
102+ """Do nothing."""
103+
104+ self.aggregator.register_progress_listener(fake_callback)
105+ self.assertEqual(len(self.aggregator.progress_listeners), 1)
106+
107+ def test_register_progress_listener_fail(self):
108+ """Check that register listener fails with not Callable objects."""
109+ self.assertRaises(TypeError,
110+ self.aggregator.register_progress_listener, [])
111+ self.assertEqual(len(self.aggregator.progress_listeners), 0)
112+
113 def assertMiscCommandQueued(self, fc):
114 """Assert that some command was queued."""
115 self.assertEqual(len(self.aggregator.to_do), 1)
116
117=== modified file 'tests/syncdaemon/test_main.py'
118--- tests/syncdaemon/test_main.py 2012-09-19 17:23:02 +0000
119+++ tests/syncdaemon/test_main.py 2012-09-26 20:23:20 +0000
120@@ -70,19 +70,6 @@
121 return defer.succeed(None)
122
123
124-class FakedSyncMenu(object):
125- """Do nothing."""
126-
127- def __init__(self, *args, **kwargs):
128- self.arguments = (args, kwargs)
129-
130- def update_progress(self):
131- """Do nothing."""
132-
133- def start_timer(self):
134- """Do nothing."""
135-
136-
137 class MainTests(BaseTwistedTestCase):
138 """ Basic tests to check main.Main """
139
140@@ -100,7 +87,6 @@
141 self.patch(main_mod.event_logging, "get_listener", lambda *a: None)
142 # no status listener by default
143 self.patch(main_mod.status_listener, "get_listener", lambda *a: None)
144- self.patch(main_mod.sync_menu, "UbuntuOneSyncMenu", FakedSyncMenu)
145
146 self.handler = MementoHandler()
147 self.handler.setLevel(logging.DEBUG)
148
149=== modified file 'tests/syncdaemon/test_status_listener.py'
150--- tests/syncdaemon/test_status_listener.py 2012-04-09 20:07:05 +0000
151+++ tests/syncdaemon/test_status_listener.py 2012-09-26 20:23:20 +0000
152@@ -84,7 +84,7 @@
153 callback(args)
154
155 listener = Listener()
156- setattr(listener, 'handle_'+event, listener._handle_event)
157+ setattr(listener, 'handle_' + event, listener._handle_event)
158 event_q.subscribe(listener)
159 return listener
160
161@@ -92,7 +92,7 @@
162 class FakeStatusFrontend(object):
163 """A fake status frontend."""
164
165- def __init__(self):
166+ def __init__(self, *args, **kwargs):
167 """Initialize this instance."""
168 self.call_log = []
169
170
171=== modified file 'ubuntuone/platform/sync_menu/common.py'
172--- ubuntuone/platform/sync_menu/common.py 2012-09-18 16:29:22 +0000
173+++ ubuntuone/platform/sync_menu/common.py 2012-09-26 20:23:20 +0000
174@@ -31,13 +31,3 @@
175
176 class UbuntuOneSyncMenu(object):
177 """Integrate U1 with the Ubuntu Sync Menu."""
178-
179- def __init__(self, *args):
180- self.transfers = DummyTransfersMenu()
181-
182-
183-class DummyTransfersMenu(object):
184- """Dummy Transfers Menu."""
185-
186- def start_timer(self):
187- """Empty start timer, this is not needed on windows."""
188
189=== modified file 'ubuntuone/platform/sync_menu/linux.py'
190--- ubuntuone/platform/sync_menu/linux.py 2012-09-24 12:47:05 +0000
191+++ ubuntuone/platform/sync_menu/linux.py 2012-09-26 20:23:20 +0000
192@@ -29,6 +29,8 @@
193 """Use SyncMenu lib to integrate U1 with the Systray Sync Icon."""
194
195 import gettext
196+import logging
197+import time
198 import sys
199 import webbrowser
200
201@@ -50,10 +52,12 @@
202 use_syncmenu = True
203 except:
204 use_syncmenu = False
205-from twisted.internet import reactor
206
207 from ubuntuone.clientdefs import GETTEXT_PACKAGE
208-
209+from ubuntuone import status
210+
211+
212+logger = logging.getLogger("ubuntuone.platform.SyncMenu")
213
214 Q_ = lambda string: gettext.dgettext(GETTEXT_PACKAGE, string)
215
216@@ -63,6 +67,7 @@
217 MORE_STORAGE = Q_("Get More Space")
218 GET_HELP = Q_("Get Help on the Web")
219
220+DELAY_BETWEEN_UPDATES = 3
221 UBUNTUONE_LINK = u'https://one.ubuntu.com/'
222 DASHBOARD = UBUNTUONE_LINK + u'dashboard/'
223 HELP_LINK = UBUNTUONE_LINK + u'support/'
224@@ -72,11 +77,12 @@
225 class UbuntuOneSyncMenuLinux(object):
226 """Integrate U1 with the Ubuntu Sync Menu."""
227
228- def __init__(self, main):
229+ def __init__(self, status, syncdaemon_service):
230 """Initialize menu."""
231- self._main = main
232- self.status_listener = main.status_listener
233+ self._syncdaemon_service = syncdaemon_service
234 self._paused = False
235+ self.timer = None
236+ self.next_update = time.time()
237 self.root_menu = Dbusmenu.Menuitem()
238
239 self.open_u1 = Dbusmenu.Menuitem()
240@@ -86,7 +92,7 @@
241 self.go_to_web.property_set(Dbusmenu.MENUITEM_PROP_LABEL,
242 GO_TO_WEB)
243
244- self.transfers = TransfersMenu(self.status_listener)
245+ self.transfers = TransfersMenu(status)
246 self.transfers.property_set(Dbusmenu.MENUITEM_PROP_LABEL,
247 TRANSFERS)
248
249@@ -121,17 +127,13 @@
250 self.app.set_menu(self.server)
251 self.app.connect("notify::paused", self.change_sync_status)
252
253- def start_timer(self):
254- """Start the transfers timer."""
255- self.transfers.start_timer()
256-
257 def change_sync_status(self, *args):
258 """Triggered when the sync status is changed fromm the menu."""
259 if self._paused:
260- self._main.external.connect()
261+ self._syncdaemon_service.connect()
262 self._paused = False
263 else:
264- self._main.external.disconnect()
265+ self._syncdaemon_service.disconnect()
266 self._paused = True
267
268 def open_control_panel(self, *args):
269@@ -150,34 +152,45 @@
270 """Open the Ubunto One Help Page"""
271 webbrowser.open(GET_STORAGE_LINK)
272
273+ def _timeout(self, result):
274+ """The aggregating timer has expired, so update the UI."""
275+ self.next_update = int(time.time()) + DELAY_BETWEEN_UPDATES
276+ self.transfers.update_progress()
277+ self.timer = None
278+
279+ def update_transfers(self):
280+ """Set up a timer if there isn't one ticking and update the ui."""
281+ if not self.timer:
282+ logger.debug("Updating Transfers.")
283+ delay = int(max(0, min(DELAY_BETWEEN_UPDATES,
284+ self.next_update - time.time())))
285+ self.timer = status.aggregator.Timer(delay)
286+ self.timer.addCallback(self._timeout)
287+
288
289 class TransfersMenu(Dbusmenu.Menuitem):
290 """Menu that handles the recent and current transfers."""
291
292- def __init__(self, listener):
293+ def __init__(self, status_frontend):
294 super(TransfersMenu, self).__init__()
295- self.listener = listener
296+ self.status_frontend = status_frontend
297 self.uploading = {}
298 self.previous_transfers = []
299 self._transfers_items = {}
300 self._uploading_items = {}
301 self.separator = None
302
303- def start_timer(self):
304- """Trigger an update in one second."""
305- self.update_progress()
306- reactor.callLater(3, self.start_timer)
307-
308 def update_progress(self):
309 """Update the list of recent transfers and current transfers."""
310- current_transfers = self.listener.status_frontend.recent_transfers()
311+ current_transfers = self.status_frontend.recent_transfers()
312 uploading_data = {}
313 for filename, size, written in \
314- self.listener.status_frontend.files_uploading():
315+ self.status_frontend.files_uploading():
316 uploading_data[filename] = (size, written)
317
318 temp_transfers = {}
319 if current_transfers != self.previous_transfers:
320+ logger.debug("Update recent transfers with: %r", current_transfers)
321 for item_transfer in self._transfers_items:
322 self.child_delete(self._transfers_items[item_transfer])
323 for item in current_transfers:
324@@ -204,6 +217,8 @@
325 upload_item.property_set_int(
326 SyncMenu.PROGRESS_MENUITEM_PROP_PERCENT_DONE,
327 percentage)
328+ logger.debug("Current transfer %s progress update: %r",
329+ item, percentage)
330 items_added += 1
331 else:
332 self.child_delete(self._uploading_items[item])
333@@ -223,6 +238,7 @@
334 uploading_file.property_set_int(
335 SyncMenu.PROGRESS_MENUITEM_PROP_PERCENT_DONE,
336 percentage)
337+ logger.debug("Current transfer %s created", item)
338 self.child_append(uploading_file)
339 self._uploading_items[item] = uploading_file
340 items_added += 1
341@@ -234,8 +250,7 @@
342 def __init__(self, *args, **kwargs):
343 """Initialize menu."""
344
345- def start_timer(self):
346+ def update_transfers(self):
347 """Do nothing."""
348
349-
350 UbuntuOneSyncMenu = UbuntuOneSyncMenuLinux if use_syncmenu else DummySyncMenu
351
352=== modified file 'ubuntuone/status/aggregator.py'
353--- ubuntuone/status/aggregator.py 2012-08-16 12:00:12 +0000
354+++ ubuntuone/status/aggregator.py 2012-09-26 20:23:20 +0000
355@@ -33,7 +33,7 @@
356 import itertools
357 import operator
358 import os
359-from collections import deque
360+from collections import deque, Callable
361
362 import gettext
363
364@@ -41,7 +41,11 @@
365
366 from ubuntuone.clientdefs import GETTEXT_PACKAGE
367 from ubuntuone.status.logger import logger
368-from ubuntuone.platform import session, notification
369+from ubuntuone.platform import (
370+ notification,
371+ session,
372+ sync_menu
373+)
374 from ubuntuone.platform.messaging import Messaging
375 from ubuntuone.platform.launcher import UbuntuOneLauncher, DummyLauncher
376
377@@ -625,6 +629,7 @@
378 self.progress = {}
379 self.to_do = {}
380 self.recent_transfers = deque(maxlen=5)
381+ self.progress_listeners = []
382
383 def get_notification(self):
384 """Create a new toggleable notification object."""
385@@ -655,6 +660,13 @@
386 self.to_do = {}
387 # pylint: enable=W0201
388
389+ def register_progress_listener(self, listener):
390+ """Register a callable object to be notified."""
391+ if isinstance(listener, Callable):
392+ self.progress_listeners.append(listener)
393+ else:
394+ raise TypeError("Callable object expected.")
395+
396 def get_discovery_message(self):
397 """Get the text for the discovery bubble."""
398 lines = []
399@@ -716,6 +728,8 @@
400 progress = float(
401 sum(self.progress.values())) / sum(self.to_do.values())
402 self.progress_bar.set_progress(progress)
403+ for listener in self.progress_listeners:
404+ listener()
405
406 def download_started(self, command):
407 """A download just started."""
408@@ -801,13 +815,25 @@
409 class StatusFrontend(object):
410 """Frontend for the status aggregator, used by the StatusListener."""
411
412- def __init__(self, clock=reactor):
413+ def __init__(self, clock=reactor, service=None):
414 """Initialize this instance."""
415 self.aggregator = StatusAggregator(clock=clock)
416 self.notification = self.aggregator.get_notification()
417 self.messaging = Messaging()
418 self.quota_timer = None
419
420+ self.syncdaemon_service = service
421+ self.sync_menu = None
422+ self.start_sync_menu()
423+
424+ def start_sync_menu(self):
425+ """Create the sync menu and register the progress listener."""
426+ if self.syncdaemon_service is not None:
427+ self.sync_menu = sync_menu.UbuntuOneSyncMenu(self,
428+ self.syncdaemon_service)
429+ self.aggregator.register_progress_listener(
430+ self.sync_menu.update_transfers)
431+
432 def recent_transfers(self):
433 """Return a tuple with the recent transfers paths."""
434 return list(self.aggregator.recent_transfers)
435
436=== modified file 'ubuntuone/syncdaemon/main.py'
437--- ubuntuone/syncdaemon/main.py 2012-09-19 17:23:02 +0000
438+++ ubuntuone/syncdaemon/main.py 2012-09-26 20:23:20 +0000
439@@ -48,10 +48,7 @@
440 volume_manager,
441 )
442 from ubuntuone import syncdaemon, clientdefs
443-from ubuntuone.platform import (
444- event_logging,
445- sync_menu,
446-)
447+from ubuntuone.platform import event_logging
448 from ubuntuone.syncdaemon import status_listener
449 from ubuntuone.syncdaemon.interaction_interfaces import SyncdaemonService
450 from ubuntuone.syncdaemon.states import StateManager, QueueManager
451@@ -159,14 +156,6 @@
452 self.mark = task.LoopingCall(self.log_mark)
453 self.mark.start(mark_interval)
454
455- self.sync_menu = None
456- self.start_sync_menu()
457-
458- def start_sync_menu(self):
459- """Create the sync menu and run the loop."""
460- self.sync_menu = sync_menu.UbuntuOneSyncMenu(self)
461- self.sync_menu.start_timer()
462-
463 def start_event_logger(self):
464 """Start the event logger if it's available for this platform."""
465 self.eventlog_listener = event_logging.get_listener(self.fs, self.vm)
466@@ -176,7 +165,8 @@
467
468 def start_status_listener(self):
469 """Start the status listener if it is configured to start."""
470- self.status_listener = status_listener.get_listener(self.fs, self.vm)
471+ self.status_listener = status_listener.get_listener(self.fs, self.vm,
472+ self.external)
473 # subscribe to EQ, to be unsubscribed in shutdown
474 if self.status_listener:
475 self.event_q.subscribe(self.status_listener)
476
477=== modified file 'ubuntuone/syncdaemon/status_listener.py'
478--- ubuntuone/syncdaemon/status_listener.py 2012-08-10 12:49:46 +0000
479+++ ubuntuone/syncdaemon/status_listener.py 2012-09-26 20:23:20 +0000
480@@ -48,10 +48,10 @@
481 return True
482
483
484-def get_listener(fsm, vm):
485+def get_listener(fsm, vm, syncdaemon_service=None):
486 """Return an instance of the status listener, or None if turned off."""
487 if should_start_listener():
488- status_frontend = StatusFrontend()
489+ status_frontend = StatusFrontend(service=syncdaemon_service)
490 return StatusListener(fsm, vm, status_frontend)
491 else:
492 return None

Subscribers

People subscribed via source and target branches