Merge lp:~thisfred/ubuntuone-client/filenames-in-notifications into lp:ubuntuone-client

[Eric Casteleijn]

Added a filename in the downloading/downloaded/uploading/uploaded notifications to give end user more context.

Added a filename in the downloading/downloaded/uploading/uploaded notifications to give end user more context.

Looks like:

"'foo.bar' was downloaded to your computer."

or:

"'baz.qux' and 796 other files are uploading to your personal cloud."

[Manuel de la Peña (mandel)]

AFAIK most of the code from the notification is platform anostic, which means that we can reuse the logic when doing the port (and use something else instead of libnotify, is this right? if that is the case, we might have to remove the /n with os.sep. Since the /n were already in the code, I'll approve and will fix it in the windows branch whenever needed.

review: Approve

[Eric Casteleijn (thisfred)]

yeah, everything that is not under platform should be agnostic, so let's fix that.

[lp:~thisfred/ubuntuone-client/filenames-in-notifications]

914. By Eric Casteleijn on 2011-03-09

keep state in the right place

915. By Eric Casteleijn on 2011-03-09

move property settings outside of __init__ and escape warnings

916. By Eric Casteleijn on 2011-03-09

added assertions for the filenames being set

[Alejandro J. Cura (alecu)]

Great branch!

review: Approve

=== modified file 'tests/status/test_aggregator.py'
--- tests/status/test_aggregator.py	2011-03-03 23:29:37 +0000
+++ tests/status/test_aggregator.py	2011-03-09 20:40:33 +0000
@@ -31,6 +31,9 @@
 from ubuntuone.syncdaemon import status_listener
 from ubuntuone.syncdaemon.volume_manager import Share, UDF
 
+FILENAME = 'example.txt'
+FILENAME2 = 'another_example.mp3'
+
 
 class PatchedClock(Clock):
     """Patch the clock to fix twisted bug #4823."""
@@ -746,6 +749,9 @@
 class FakeCommand(object):
     """A fake command."""
 
+    def __init__(self, path=''):
+        self.path = path
+
 
 class FakeVolumeManager(object):
     """A fake vm."""
@@ -767,6 +773,8 @@
         self.queued_commands = set()
         self.notification_switch = aggregator.NotificationSwitch()
         self.connected = False
+        self.files_uploading = []
+        self.files_downloading = []
 
     def queue_done(self):
         """The queue completed all operations."""
@@ -784,18 +792,35 @@
         """Create a new toggleable notification object."""
         return self.notification_switch.build_notification()
 
-    download_started = misc_command_queued
-    download_finished = misc_command_unqueued
-    upload_started = misc_command_queued
-    upload_finished = misc_command_unqueued
+    def download_started(self, command):
+        """A download just started."""
+        self.files_downloading.append(command)
+        self.misc_command_queued(command)
+
+    def download_finished(self, command):
+        """A download just finished."""
+        if command in self.files_downloading:
+            self.files_downloading.remove(command)
+        self.misc_command_unqueued(command)
+
+    def upload_started(self, command):
+        """An upload just started."""
+        self.files_uploading.append(command)
+        self.misc_command_queued(command)
+
+    def upload_finished(self, command):
+        """An upload just finished."""
+        if command in self.files_uploading:
+            self.files_uploading.remove(command)
+        self.misc_command_unqueued(command)
 
     def connection_made(self):
-       """The client made the connection to the server."""
-       self.connected = True
+        """The client made the connection to the server."""
+        self.connected = True
 
     def connection_lost(self):
-       """The client lost the connection to the server."""
-       self.connected = False
+        """The client lost the connection to the server."""
+        self.connected = False
 
 
 class StatusFrontendTestCase(BaseTwistedTestCase):
@@ -1255,9 +1280,11 @@
 
     def test_file_download_started(self):
         """Test that a file has started download."""
-        fc = FakeCommand()
+        fc = FakeCommand(path='testfile.txt')
+        self.assertEqual('', self.aggregator.downloading_filename)
         self.status_frontend.download_started(fc)
-        self.assertEqual(self.aggregator.download_total, 1)
+        self.assertEqual(1, self.aggregator.download_total)
+        self.assertEqual('testfile.txt', self.aggregator.downloading_filename)
         self.assertMiscCommandQueued(fc)
         self.assertEqual(1, self.fake_bubble.count)
 
@@ -1271,9 +1298,11 @@
 
     def test_file_upload_started(self):
         """Test that a file has started upload."""
-        fc = FakeCommand()
+        fc = FakeCommand(path='testfile.txt')
+        self.assertEqual('', self.aggregator.uploading_filename)
         self.status_frontend.upload_started(fc)
-        self.assertEqual(self.aggregator.upload_total, 1)
+        self.assertEqual(1, self.aggregator.upload_total)
+        self.assertEqual('testfile.txt', self.aggregator.uploading_filename)
         self.assertMiscCommandQueued(fc)
         self.assertEqual(1, self.fake_bubble.count)
 
@@ -1289,10 +1318,18 @@
         """Test the message that's shown on the discovery bubble."""
         uploading = 10
         downloading = 8
-        self.aggregator.files_uploading.extend(range(uploading))
-        self.aggregator.files_downloading.extend(range(downloading))
-        expected = (aggregator.files_being_uploaded(uploading) +
-                    "\n" + aggregator.files_being_downloaded(downloading))
+        filename = 'upfile0.ext'
+        filename2 = 'downfile0.ext'
+        self.aggregator.files_uploading.extend([
+            FakeCommand(path='upfile%d.ext' % n) for n in range(uploading)])
+        self.aggregator.uploading_filename = filename
+        self.aggregator.files_downloading.extend([
+            FakeCommand(path='downfile%d.ext' % n) for n in
+            range(downloading)])
+        self.aggregator.downloading_filename = filename2
+        expected = (
+            aggregator.files_being_uploaded(filename, uploading) + "\n" +
+            aggregator.files_being_downloaded(filename2, downloading))
         result = self.aggregator.get_discovery_message()
         self.assertEqual(expected, result)
 
@@ -1300,44 +1337,44 @@
         """Test the message that's shown on the progress bubble."""
         self.aggregator.upload_done = 5
         self.aggregator.upload_total = 10
+        self.aggregator.uploading_filename = FILENAME
         self.aggregator.download_done = 3
         self.aggregator.download_total = 8
+        self.aggregator.downloading_filename = FILENAME2
         self.aggregator.done_counter = 9
         self.aggregator.total_counter = 20
         percentage = int(100.0 * self.aggregator.done_counter /
                          self.aggregator.total_counter)
-        format_args = {
-            "percentage_completed": percentage,
-        }
-
-        format_string = (
-            aggregator.files_being_uploaded(self.aggregator.upload_total) +
-            "\n" +
-            aggregator.files_being_downloaded(self.aggregator.download_total) +
-            "\n" + aggregator.PROGRESS_COMPLETED)
-
-        expected = format_string % format_args
+
+        expected = (
+            aggregator.files_being_uploaded(
+                FILENAME, self.aggregator.upload_total) + "\n" +
+            aggregator.files_being_downloaded(
+                FILENAME2, self.aggregator.download_total) + "\n" +
+            aggregator.PROGRESS_COMPLETED) % {
+            'percentage_completed': percentage}
+
         result = self.aggregator.get_progress_message()
         self.assertEqual(expected, result)
 
     def test_get_progress_message_no_uploads(self):
         """The progress message when no uploads are going on."""
+
         self.aggregator.upload_done = 0
         self.aggregator.upload_total = 0
+        self.aggregator.downloading_filename = FILENAME
         self.aggregator.download_done = 3
         self.aggregator.download_total = 8
         self.aggregator.done_counter = 9
         self.aggregator.total_counter = 20
         percentage = int(100.0 * self.aggregator.done_counter /
                          self.aggregator.total_counter)
-        format_args = {
-            "percentage_completed": percentage}
-
-        format_string = (
-            aggregator.files_being_downloaded(self.aggregator.download_total) +
-            "\n" + aggregator.PROGRESS_COMPLETED)
-
-        expected = format_string % format_args
+        expected = (
+            aggregator.files_being_downloaded(
+                FILENAME, self.aggregator.download_total) + "\n" +
+            aggregator.PROGRESS_COMPLETED) % {
+            'percentage_completed': percentage}
+
         result = self.aggregator.get_progress_message()
         self.assertEqual(expected, result)
 
@@ -1345,21 +1382,19 @@
         """The progress message when no downloads are going on."""
         self.aggregator.upload_done = 5
         self.aggregator.upload_total = 10
+        self.aggregator.uploading_filename = FILENAME
         self.aggregator.download_done = 0
         self.aggregator.download_total = 0
         self.aggregator.done_counter = 9
         self.aggregator.total_counter = 20
         percentage = int(100.0 * self.aggregator.done_counter /
                          self.aggregator.total_counter)
-        format_args = {
-            "percentage_completed": percentage,
-        }
-
-        format_string = (
-            aggregator.files_being_uploaded(self.aggregator.upload_total) +
-            "\n" + aggregator.PROGRESS_COMPLETED)
-
-        expected = format_string % format_args
+        expected = (
+            aggregator.files_being_uploaded(
+                FILENAME, self.aggregator.upload_total) +
+            "\n" + aggregator.PROGRESS_COMPLETED) % {
+            'percentage_completed': percentage}
+
         result = self.aggregator.get_progress_message()
         self.assertEqual(expected, result)
 
@@ -1371,40 +1406,47 @@
     def test_get_final_status_message(self):
         """The final status message."""
         done = (5, 10)
+        self.aggregator.uploading_filename = FILENAME
+        self.aggregator.downloading_filename = FILENAME2
         self.aggregator.upload_done, self.aggregator.download_done = done
 
-        format_string = (
+        expected = (
             aggregator.FINAL_COMPLETED + "\n" +
-            aggregator.files_were_uploaded(self.aggregator.upload_done) +
-            "\n" +
-            aggregator.files_were_downloaded(self.aggregator.download_done))
+            aggregator.files_were_uploaded(
+                FILENAME, self.aggregator.upload_done) + "\n" +
+            aggregator.files_were_downloaded(
+                FILENAME2, self.aggregator.download_done))
 
         result = self.aggregator.get_final_status_message()
-        self.assertEqual(format_string, result)
+        self.assertEqual(expected, result)
 
     def test_get_final_status_message_no_uploads(self):
         """The final status message when there were no uploads."""
         done = (0, 12)
         self.aggregator.upload_done, self.aggregator.download_done = done
+        self.aggregator.downloading_filename = FILENAME2
 
-        format_string = (aggregator.FINAL_COMPLETED + "\n" +
-                         aggregator.files_were_downloaded(
-                             self.aggregator.download_done))
+        expected = (
+            aggregator.FINAL_COMPLETED + "\n" +
+            aggregator.files_were_downloaded(
+                FILENAME2, self.aggregator.download_done))
 
         result = self.aggregator.get_final_status_message()
-        self.assertEqual(format_string, result)
+        self.assertEqual(expected, result)
 
     def test_get_final_status_message_no_downloads(self):
         """The final status message when there were no downloads."""
         done = (8, 0)
         self.aggregator.upload_done, self.aggregator.download_done = done
+        self.aggregator.uploading_filename = FILENAME
 
-        format_string = (aggregator.FINAL_COMPLETED + "\n" +
-                         aggregator.files_were_uploaded(
-                             self.aggregator.upload_done))
+        expected = (
+            aggregator.FINAL_COMPLETED + "\n" +
+            aggregator.files_were_uploaded(
+                FILENAME, self.aggregator.upload_done))
 
         result = self.aggregator.get_final_status_message()
-        self.assertEqual(format_string, result)
+        self.assertEqual(expected, result)
 
     def test_started_progress_bubble(self):
         """The progress bubble is started."""
@@ -1490,7 +1532,7 @@
         self.patch(aggregator, "UbuntuOneLauncher", FakeLauncher)
         self.patch(aggregator.session, "Inhibitor", FakeInhibitor)
         clock = PatchedClock()
-        upload = FakeCommand()
+        upload = FakeCommand(path='upload.foo')
         sf = aggregator.StatusFrontend(clock=clock)
         sf.set_show_all_notifications(True)
 
@@ -1507,7 +1549,7 @@
         clock.advance(aggregator.FileDiscoveryGatheringState.initial_delay)
         # files found notification
         self.assertEqual(1, len(notifications_shown))
-        download = FakeCommand()
+        download = FakeCommand('download.bar')
         sf.download_started(download)
         self.assertEqual(1, len(notifications_shown))
         # the progress still is zero
 
=== modified file 'ubuntuone/status/aggregator.py'
--- ubuntuone/status/aggregator.py	2011-03-03 23:29:37 +0000
+++ ubuntuone/status/aggregator.py	2011-03-09 20:40:33 +0000
@@ -53,50 +53,72 @@
 FILE_SYNC_IN_PROGRESS = Q_("File synchronization in progress")
 
 
-def files_being_uploaded(files_uploading):
+def files_being_uploaded(filename, files_uploading):
     """Get the i18n string for files being uploaded."""
-    format_args = {"total_uploading_files": files_uploading}
+    other_files = files_uploading - 1
+    if other_files < 1:
+        return Q_(
+            "'%(filename)s' is being uploaded to your personal cloud.") % {
+            'filename': filename}
+    format_args = {
+        "filename": filename, "other_files": other_files}
     return gettext.dngettext(
         GETTEXT_PACKAGE,
-        "%(total_uploading_files)d file is being uploaded to your personal "
-        "cloud.",
-        "%(total_uploading_files)d files are being uploaded to your personal "
-        "cloud.",
-        files_uploading) % format_args
-
-
-def files_being_downloaded(files_downloading):
+        "'%(filename)s' and %(other_files)d other file is being "
+        "uploaded to your personal cloud.",
+        "'%(filename)s' and %(other_files)d other files are being "
+        "uploaded to your personal cloud.", other_files) % format_args
+
+
+def files_being_downloaded(filename, files_downloading):
     """Get the i18n string for files being downloaded."""
-    format_args = {"total_downloading_files": files_downloading}
+    other_files = files_downloading - 1
+    if other_files < 1:
+        return Q_(
+            "'%(filename)s' is being downloaded to your computer.") % {
+            'filename': filename}
+    format_args = {
+        "filename": filename, "other_files": other_files}
     return gettext.dngettext(
         GETTEXT_PACKAGE,
-        "%(total_downloading_files)d file is being downloaded to your "
-        "computer.",
-        "%(total_downloading_files)d files are being downloaded to "
-        "your computer.",
-        files_downloading) % format_args
-
-
-def files_were_uploaded(upload_done):
+        "'%(filename)s' and %(other_files)d other file is being "
+        "downloaded to your computer.",
+        "'%(filename)s' and %(other_files)d other files are being "
+        "downloaded to your computer.", other_files) % format_args
+
+
+def files_were_uploaded(filename, upload_done):
     """Get the i18n string for files that were uploaded."""
-    format_args = {'total_uploaded_files': upload_done}
+    other_files = upload_done - 1
+    if other_files < 1:
+        return Q_(
+            "'%(filename)s' was uploaded to your personal cloud.") % {
+            'filename': filename}
+    format_args = {
+        'filename': filename, 'other_files': other_files}
     return gettext.dngettext(
         GETTEXT_PACKAGE,
-        "%(total_uploaded_files)d file was uploaded to your personal "
-        "cloud.",
-        "%(total_uploaded_files)d files were uploaded to your "
-        "personal cloud.", upload_done) % format_args
-
-
-def files_were_downloaded(download_done):
+        "'%(filename)s' and %(other_files)d other file was uploaded to "
+        "your personal cloud.",
+        "'%(filename)s' and %(other_files)d other files were uploaded "
+        "to your personal cloud.", other_files) % format_args
+
+
+def files_were_downloaded(filename, download_done):
     """Get the i18n string for files that were downloaded."""
-    format_args = {'total_downloaded_files': download_done}
+    other_files = download_done - 1
+    if other_files < 1:
+        return Q_(
+            "'%(filename)s' was downloaded to your computer.") % {
+            'filename': filename}
+    format_args = {
+        'filename': filename, 'other_files': other_files}
     return gettext.dngettext(
         GETTEXT_PACKAGE,
-        "%(total_downloaded_files)d file was downloaded to your "
-        "computer.",
-        "%(total_downloaded_files)d files were downloaded to your "
-        "computer.", download_done) % format_args
+        "'%(filename)s' and %(other_files)d other file was "
+        "downloaded to your computer.",
+        "'%(filename)s' and %(other_files)d other files were "
+        "downloaded to your computer.", other_files) % format_args
 
 
 class ToggleableNotification(object):
@@ -592,6 +614,7 @@
         """Create a new toggleable notification object."""
         return self.notification_switch.build_notification()
 
+    # pylint: disable=W0201
     def reset(self):
         """Reset all counters and notifications."""
         self.total_counter = 0
@@ -601,7 +624,9 @@
         self.upload_total = 0
         self.upload_done = 0
         self.files_uploading = []
+        self.uploading_filename = ''
         self.files_downloading = []
+        self.downloading_filename = ''
 
         if self.file_discovery_bubble:
             self.file_discovery_bubble.cleanup()
@@ -615,17 +640,21 @@
         if self.final_status_bubble:
             self.final_status_bubble.cleanup()
         self.final_status_bubble = FinalStatusBubble(self)
+    # pylint: enable=W0201
 
     def get_discovery_message(self):
         """Get the text for the discovery bubble."""
         lines = []
         files_uploading = len(self.files_uploading)
         if files_uploading > 0:
-            lines.append(files_being_uploaded(files_uploading))
-
+            lines.append(files_being_uploaded(
+                self.uploading_filename, files_uploading))
         files_downloading = len(self.files_downloading)
         if files_downloading > 0:
-            lines.append(files_being_downloaded(files_downloading))
+            self.downloading_filename = self.files_downloading[0].path.split(
+                os.path.sep)[-1]
+            lines.append(files_being_downloaded(
+                self.downloading_filename, files_downloading))
         return "\n".join(lines)
 
     def get_progress_message(self):
@@ -634,10 +663,12 @@
         parts = []
         upload_total = self.upload_total
         if upload_total:
-            parts.append(files_being_uploaded(upload_total))
+            parts.append(files_being_uploaded(
+                self.uploading_filename, upload_total))
         download_total = self.download_total
         if download_total:
-            parts.append(files_being_downloaded(download_total))
+            parts.append(files_being_downloaded(
+                self.downloading_filename, download_total))
         progress_percentage = 100.0 * self.done_counter / self.total_counter
         format_args = {"percentage_completed": int(progress_percentage)}
         parts.append(PROGRESS_COMPLETED % format_args)
@@ -649,11 +680,13 @@
         parts.append(FINAL_COMPLETED)
         upload_done = self.upload_done
         if upload_done:
-            parts.append(files_were_uploaded(upload_done))
+            parts.append(files_were_uploaded(
+                self.uploading_filename, upload_done))
 
         download_done = self.download_done
         if download_done:
-            parts.append(files_were_downloaded(download_done))
+            parts.append(files_were_downloaded(
+                self.downloading_filename, download_done))
         return "\n".join(parts)
 
     def restart_progress_bubble(self):
@@ -689,6 +722,11 @@
         """A download just started."""
         self.files_downloading.append(command)
         self.download_total += 1
+        # pylint: disable=W0201
+        if not self.downloading_filename:
+            self.downloading_filename = self.files_downloading[0].path.split(
+                os.path.sep)[-1]
+        # pylint: enable=W0201
         self.misc_command_queued(command)
         self.file_discovery_bubble.new_file_found()
 
@@ -703,6 +741,11 @@
         """An upload just started."""
         self.files_uploading.append(command)
         self.upload_total += 1
+        # pylint: disable=W0201
+        if not self.uploading_filename:
+            self.uploading_filename = self.files_uploading[0].path.split(
+                os.path.sep)[-1]
+        # pylint: enable=W0201
         self.misc_command_queued(command)
         self.file_discovery_bubble.new_file_found()