Merge lp:~ngaller/ubuntuone-client/hide-applet-notifications into lp:ubuntuone-client

[Nicolas Galler]

[Nicolas Galler (ngaller)]

Added an option to hide the "Updating file" notification popups. This is associated with https://bugs.launchpad.net/ubuntuone-client/+bug/485156. First time here, so please let me know if you need anything else from me!

Thanks

[Natalia Bidart (nataliabidart)]

Thank you for this contribution! I tested it and seems to work really well.

review: Approve

[Facundo Batista (facundo)]

It seems to work ok!

review: Approve

[dobey (dobey)]

This seems like the wrong solution to the problem, to me. It's solving the symptoms rather than the problem, and there are some "needs fixing" items if we do want to have an option to disable notifications. The option in the config and any variables or method names should be the whole word and not a truncation of it. Why was "notif" used instead of "notify" everywhere here? There's a one letter difference, but it makes a huge difference when you're reading the code, and don't already know what it is.

The symptom is that you're seeing several extraneous notifications when lots of tiny files get modified often. We shouldn't fix it by hiding the notifications, but with a proper fix to wait for a certain number of bytes, or a period of inactivity, so that we don't notify too often.

review: Disapprove

[Nicolas Galler (ngaller)]

I commented on why I thought it is still an important feature, regardless of the rate of updating, on the bug report.

However I should point out that I am very unaware of the naming conventions in use - I just tried to copy what I saw. The "hide_notif" stands for "hide_notifications" which seemed a bit long to type while "hide_notify" didn't seem grammatically sound to me. Not my baby though - feel free to rename it :)

[John O'Brien (jdobrien)]

This change seems reasonable to me since it is really changing the behavior of the applet and how it responds to notifications.

I personally would not have chosen to shorten to 'notif' but I don't think it's really that confusing.

Thanks for the contribution.

review: Approve

[dobey (dobey)]

I'm going to mark this as rejected as it seems like the wrong fix to me, and we are going to be getting rid of the applet soon. Thanks for contributing and I hope we can see more contributions from you. If you want to ping me or other developers in IRC or on the mailing list, to help lead in the right direction, we will be glad to help you.

Thanks again. :)

[Nicolas Galler (ngaller)]

Will do - thanks! :)

Unmerged revisions

279. By Nicolas Galler <nico@anoat> on 2009-11-19

Add option for hiding the Updating notifications

=== modified file 'bin/ubuntuone-client-applet'
--- bin/ubuntuone-client-applet	2009-10-22 18:56:58 +0000
+++ bin/ubuntuone-client-applet	2009-11-19 04:25:18 +0000
@@ -114,6 +114,9 @@
             if not self.config.has_option("ubuntuone", "show_applet"):
                   self.config.set("ubuntuone", "show_applet", "1")
 
+            if not self.config.has_option("ubuntuone", "hide_notif"):
+                  self.config.set("ubuntuone", "hide_notif", "False")
+
             if not self.config.has_option("ubuntuone", "connect"):
                   self.config.set("ubuntuone", "connect", "0")
 
@@ -441,6 +444,8 @@
             # A ConfigParser object that we can poke at
             self.__config = config
             self.__show_when = self.__config.getint("ubuntuone", "show_applet")
+            self.__hide_notif = self.__config.getboolean("ubuntuone", 
+                                                         "hide_notif")
 
             self.__managed_dir = None
 
@@ -525,6 +530,11 @@
             """Update the connection config."""
             self.__config.set("ubuntuone", "connect", str(connect))
 
+      def set_hide_notif_config(self, hide_notif):
+            """Update setting to hide non-critical notifications."""
+            self.__hide_notif = hide_notif
+            self.__config.set("ubuntuone", "hide_notif", hide_notif)
+
       def __update_transfer_status(self, done=False):
             """Update the status display."""
             with self.__lock:
@@ -562,14 +572,15 @@
                   self.__total = total + self.__updating
             if first:
                   self.set_from_icon_name("ubuntuone-client-updating")
-                  n = pynotify.Notification(
-                        _("Updating files..."),
-                        _("Ubuntu One is now updating your files."))
-                  pixbuf = self.__theme.load_icon(
-                        "ubuntuone-client-updating", NOTIFY_ICON_SIZE,
-                        gtk.ICON_LOOKUP_GENERIC_FALLBACK)
-                  n.set_icon_from_pixbuf(pixbuf)
-                  n.show()
+                  if not self.__hide_notif:
+                      n = pynotify.Notification(
+                            _("Updating files..."),
+                            _("Ubuntu One is now updating your files."))
+                      pixbuf = self.__theme.load_icon(
+                            "ubuntuone-client-updating", NOTIFY_ICON_SIZE,
+                            gtk.ICON_LOOKUP_GENERIC_FALLBACK)
+                      n.set_icon_from_pixbuf(pixbuf)
+                      n.show()
             if last:
                   if self.__last_id != 0:
                         gobject.source_remove(self.__last_id)
@@ -589,18 +600,21 @@
                   return False
 
             with self.__lock:
+                  total = self.__total
+                  self.__total = 0
+                  self.__updating = 0
+
+            if not self.__hide_notif:
                   n = pynotify.Notification(
                         _("Updating Finished"),
                         ngettext("Ubuntu One finished updating %(total)d file.",
                            "Ubuntu One finished updating %(total)d files.",
-                           self.__total) % { 'total' : self.__total })
-                  self.__total = 0
-                  self.__updating = 0
-            pixbuf = self.__theme.load_icon(
-                  "ubuntuone-client-updating", NOTIFY_ICON_SIZE,
-                  gtk.ICON_LOOKUP_GENERIC_FALLBACK)
-            n.set_icon_from_pixbuf(pixbuf)
-            n.show()
+                           total) % { 'total' : total})
+                  pixbuf = self.__theme.load_icon(
+                        "ubuntuone-client-updating", NOTIFY_ICON_SIZE,
+                        gtk.ICON_LOOKUP_GENERIC_FALLBACK)
+                  n.set_icon_from_pixbuf(pixbuf)
+                  n.show()
             self.__update_transfer_status(True)
             return False
 
@@ -1045,6 +1059,11 @@
       def set_connection_config(self, connect):
             self.icon.set_connection_config(connect)
 
+      @dbus.service.method(APPLET_CONFIG_NAME,
+                           in_signature='b', out_signature='')
+      def set_hide_notif_config(self, hide_notif):
+            self.icon.set_hide_notif_config(bool(hide_notif))
+
 
 if __name__ == "__main__":
       gettext.bindtextdomain(clientdefs.GETTEXT_PACKAGE, clientdefs.LOCALEDIR)

=== modified file 'bin/ubuntuone-client-preferences'
--- bin/ubuntuone-client-preferences	2009-11-06 04:44:12 +0000
+++ bin/ubuntuone-client-preferences	2009-11-19 04:25:18 +0000
@@ -118,6 +118,9 @@
             if not self.config.has_option("ubuntuone", "show_applet"):
                   self.config.set("ubuntuone", "show_applet", "1")
 
+            if not self.config.has_option("ubuntuone", "hide_notif"):
+                  self.config.set("ubuntuone", "hide_notif", "False")
+
             if not self.config.has_option("ubuntuone", "connect"):
                   self.config.set("ubuntuone", "connect", "0")
 
@@ -214,6 +217,19 @@
             except DBusException, e:
                   self.__dbus_error(e)
 
+      def __hide_notif_toggled(self, check, data=None):
+            """Update the config for hiding non-critical notifications."""
+            value = check.get_active()
+            self.config.set("ubuntuone", "hide_notif", str(value))
+            try:
+                  client = self.__bus.get_object(APPLET_IFACE_NAME, "/config",
+                                                 follow_name_owner_changes=True)
+                  iface = dbus.Interface(client, APPLET_IFACE_CONFIG_NAME)
+                  iface.set_hide_notif_config(value, reply_handler=dbus_async,
+                                              error_handler=self.__dbus_error)
+            except DBusException, e:
+                  self.__dbus_error(e)
+
       def __bw_limit_toggled(self, button, data=None):
             """Toggle the bw limit panel."""
             self.bw_enabled = self.limit_check.get_active()
@@ -277,6 +293,13 @@
             table.attach(self.show_menu, 1, 2, 0, 1)
             self.show_menu.show()
 
+            # Option to hide common notifications
+            check = gtk.CheckButton(_("_Hide Non-Critical Notifications"))
+            check.connect("toggled", self.__hide_notif_toggled)
+            check.set_active(self.config.getboolean("ubuntuone", "hide_notif"))
+            vbox.add(check)
+            check.show()
+
             label = gtk.Label(_("Connect on start:"))
             label.set_use_underline(True)
             label.set_alignment(0, 0.5)