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

Proposed by Diego Sarmentero on 2012-09-21
Status: Merged
Approved by: Diego Sarmentero on 2012-09-28
Approved revision: 1330
Merged at revision: 1326
Proposed branch: lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-menuorder
Merge into: lp:ubuntuone-client
Prerequisite: lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-timer
Diff against target: 92 lines (+45/-11)
2 files modified
tests/platform/sync_menu/test_linux.py (+34/-4)
ubuntuone/platform/sync_menu/linux.py (+11/-7)
To merge this branch: bzr merge lp:~diegosarmentero/ubuntuone-client/ubuntuone-client-menuorder
Reviewer Review Type Date Requested Status
Manuel de la Peña (community) Approve on 2012-09-28
dobey (community) 2012-09-21 Approve on 2012-09-24
Review via email: mp+125768@code.launchpad.net

Commit message

- Sort items based on the written value, to prioritize the files being transferred (LP: #1052956).

To post a comment you must log in.
dobey (dobey) wrote :

The implications of how this might behave, frighten me a little. A video showing this in action in an open transfers menu, which remains open for many (at least 30) seconds, would be quite helpful.

But based on the bug report, and that this code will presumably be updated many times while the menu is being examined by a user, it seems like the ordering may change. And having menu items constantly changing position would be quite disturbing for the user; even if the menu items are not clickable (as they do not seem to be).

review: Needs Information
1325. By Diego Sarmentero on 2012-09-21

merge

Diego Sarmentero (diegosarmentero) wrote :

> The implications of how this might behave, frighten me a little. A video
> showing this in action in an open transfers menu, which remains open for many
> (at least 30) seconds, would be quite helpful.
>
> But based on the bug report, and that this code will presumably be updated
> many times while the menu is being examined by a user, it seems like the
> ordering may change. And having menu items constantly changing position would
> be quite disturbing for the user; even if the menu items are not clickable (as
> they do not seem to be).

This is the video i showed you on irc: http://youtu.be/qOmaBCayQAo

Take into account that i'm not deleting all the items and recreating the menu with the ones that has the highest written value, but just deleting the ones that complete the transfer, and always prioritizing the ones with the highest written value to put in those places.

dobey (dobey) :
review: Approve
1326. By Diego Sarmentero on 2012-09-25

fixing tests

1327. By Diego Sarmentero on 2012-09-25

tests updated

1328. By Diego Sarmentero on 2012-09-25

merge

1329. By Diego Sarmentero on 2012-09-26

merge

1330. By Diego Sarmentero on 2012-09-26

merge

Manuel de la Peña (mandel) wrote :

I really like the use of itemgetter. +1

review: Approve

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-26 20:24:20 +0000
3+++ tests/platform/sync_menu/test_linux.py 2012-09-26 20:24:20 +0000
4@@ -343,13 +343,43 @@
5 linux.SyncMenu.PROGRESS_MENUITEM_PROP_PERCENT_DONE),
6 percentage)
7
8+ def test_transfers_order(self):
9+ """Check that the proper transfers are shown first."""
10+ data_current = [
11+ ('file0', 1200, 610),
12+ ('file1', 3000, 400),
13+ ('file2', 2000, 100),
14+ ('file3', 2500, 150),
15+ ('file4', 2500, 950),
16+ ('file5', 3500, 550),
17+ ('file6', 1000, 600),
18+ ('file7', 5000, 4600)]
19+ expected = [
20+ ('file7', 5000, 4600),
21+ ('file4', 2500, 950),
22+ ('file0', 1200, 610),
23+ ('file6', 1000, 600),
24+ ('file5', 3500, 550)]
25+ self.status_frontend.uploading_data = data_current
26+ self.sync_menu.transfers.update_progress()
27+ children = self.sync_menu.transfers.get_children()
28+ # The menu should only show 5 current transfers.
29+ self.assertEqual(len(children), 5)
30+ for i, item in enumerate(children):
31+ text = item.property_get(linux.Dbusmenu.MENUITEM_PROP_LABEL)
32+ percentage = item.property_get_int(
33+ linux.SyncMenu.PROGRESS_MENUITEM_PROP_PERCENT_DONE)
34+ name, size, written = expected[i]
35+ percentage_expected = written * 100 / size
36+ self.assertEqual(text, name)
37+ self.assertEqual(percentage, percentage_expected)
38+
39 def test_update_transfers_delay(self):
40 """Check that the timer is being handle properly."""
41 self.patch(linux.status.aggregator, "Timer", FakeTimer)
42- self.sync_menu.next_update = time.time() + 3
43+ self.sync_menu.next_update = time.time()
44 self.sync_menu.update_transfers()
45- self.assertLess(self.sync_menu.timer.delay, 3)
46 self.sync_menu.timer = None
47- self.sync_menu.next_update = time.time() / 2
48+ self.sync_menu.next_update = time.time() * 2
49 self.sync_menu.update_transfers()
50- self.assertEqual(self.sync_menu.timer.delay, 0)
51+ self.assertEqual(self.sync_menu.timer.delay, 3)
52
53=== modified file 'ubuntuone/platform/sync_menu/linux.py'
54--- ubuntuone/platform/sync_menu/linux.py 2012-09-26 20:24:20 +0000
55+++ ubuntuone/platform/sync_menu/linux.py 2012-09-26 20:24:20 +0000
56@@ -33,6 +33,8 @@
57 import time
58 import sys
59 import webbrowser
60+from collections import OrderedDict
61+from operator import itemgetter
62
63 glib = None
64 try:
65@@ -182,18 +184,20 @@
66
67 def update_progress(self):
68 """Update the list of recent transfers and current transfers."""
69- current_transfers = self.status_frontend.recent_transfers()
70- uploading_data = {}
71- for filename, size, written in \
72- self.status_frontend.files_uploading():
73+ recent_transfers = self.status_frontend.recent_transfers()
74+ current_transfers = self.status_frontend.files_uploading()
75+ current_transfers.sort(key=itemgetter(2))
76+ current_transfers.reverse()
77+ uploading_data = OrderedDict()
78+ for filename, size, written in current_transfers:
79 uploading_data[filename] = (size, written)
80
81 temp_transfers = {}
82- if current_transfers != self.previous_transfers:
83- logger.debug("Update recent transfers with: %r", current_transfers)
84+ if recent_transfers != self.previous_transfers:
85+ logger.debug("Update recent transfers with: %r", recent_transfers)
86 for item_transfer in self._transfers_items:
87 self.child_delete(self._transfers_items[item_transfer])
88- for item in current_transfers:
89+ for item in recent_transfers:
90 recent_file = Dbusmenu.Menuitem()
91 recent_file.property_set(Dbusmenu.MENUITEM_PROP_LABEL,
92 item)

Subscribers

People subscribed via source and target branches