Merge lp:~charlesk/indicator-transfer/lp-1378941-throttle-download-status-updates into lp:indicator-transfer/14.10

[Charles Kerr]

Throttle how frequently a transfer item updates its action state and menuitem.

Throttle how frequently a transfer item updates its action state and menuitem.

This is currently necessary because ubuntu-download-manager sends out a *lot* of progress update signals. In my tests for the 7digital.com download, it was around ten per second. indicator-transfer passes this flood along to unity8 -- in fact it doubles it by cascading this progress change into menu changes AND action state changes.

This fix throttles back the update broadcasts to a maximum of once per second so that the flood of changes can be sent to unity8 in a batch.

Fix tested on mako + r274 + the armhf .debs in Elleo's mikeasoft.com link in-ticket.

See also bug #1379026 for the u-d-m bug report.

[lp:~charlesk/indicator-transfer/lp-1378941-throttle-download-status-updates]

23. By Charles Kerr on 2014-10-08

fix minor memory leak spotted while tracking down this bug

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:22
http://jenkins.qa.ubuntu.com/job/indicator-transfer-ci/15/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-transfer-utopic-amd64-ci/15
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-transfer-utopic-armhf-ci/15
        deb: http://jenkins.qa.ubuntu.com/job/indicator-transfer-utopic-armhf-ci/15/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-transfer-ci/15/rebuild

review: Approve (continuous-integration)

[Ted Gould (ted)]

Looks good. Standard delay tactics.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:23
http://jenkins.qa.ubuntu.com/job/indicator-transfer-ci/16/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-transfer-utopic-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-transfer-utopic-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/indicator-transfer-utopic-armhf-ci/16/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-transfer-ci/16/rebuild

review: Approve (continuous-integration)

=== modified file 'src/world-dbus.cpp'
--- src/world-dbus.cpp	2014-08-26 20:51:10 +0000
+++ src/world-dbus.cpp	2014-10-08 20:41:48 +0000
@@ -72,6 +72,9 @@
 
   ~DBusTransfer()
   {
+    if (m_changed_tag)
+      g_source_remove(m_changed_tag);
+
     g_cancellable_cancel(m_cancellable);
     g_clear_object(&m_cancellable);
     g_clear_object(&m_bus);
@@ -239,7 +242,19 @@
     g_clear_pointer(&v, g_variant_unref);
   }
 
-  void emit_changed() { changed()(); }
+  void emit_changed_soon()
+  {
+    if (m_changed_tag == 0)
+        m_changed_tag = g_timeout_add_seconds(1, emit_changed_now, this);
+  }
+
+  static gboolean emit_changed_now(gpointer gself)
+  {
+    auto self = static_cast<DBusTransfer*>(gself);
+    self->m_changed_tag = 0;
+    self->m_changed();
+    return G_SOURCE_REMOVE;
+  }
 
   /* The 'started', 'paused', 'resumed', and 'canceled' signals
      from com.canonical.applications.Download all have a single
@@ -337,7 +352,7 @@
       }
 
     if (changed)
-      emit_changed();
+      emit_changed_soon();
   }
 
   void set_state(State state_in)
@@ -352,7 +367,7 @@
             m_history.clear();
           }
  
-        emit_changed();
+        emit_changed_soon();
       }
   }
 
@@ -363,7 +378,7 @@
     {
       g_debug("changing '%s' error to '%s'", m_ccad_path.c_str(), tmp.c_str());
       error_string = tmp;
-      emit_changed();
+      emit_changed_soon();
     }
   }
 
@@ -374,7 +389,7 @@
       {
         g_debug("changing '%s' path to '%s'", m_ccad_path.c_str(), tmp.c_str());
         local_path = tmp;
-        emit_changed();
+        emit_changed_soon();
       }
 
     // If we don't already have a title,
@@ -394,7 +409,7 @@
       {
         g_debug("changing '%s' title to '%s'", m_ccad_path.c_str(), tmp.c_str());
         title = tmp;
-        emit_changed();
+        emit_changed_soon();
       }
   }
 
@@ -460,7 +475,7 @@
       {
         g_debug("changing '%s' icon to '%s'", m_ccad_path.c_str(), tmp.c_str());
         app_icon = tmp;
-        emit_changed();
+        emit_changed_soon();
       }
   }
 
@@ -589,6 +604,7 @@
 
   core::Signal<> m_changed;
 
+  uint32_t m_changed_tag = 0;
   uint64_t m_received = 0;
   uint64_t m_total_size = 0;
   struct DownloadProgress {
@@ -763,7 +779,9 @@
                           const gchar* signal_name,
                           GVariant* parameters)
   {
-    g_debug("transfer signal: %s %s %s", cucdt_path, signal_name, g_variant_print(parameters, TRUE));
+    gchar* variant_str = g_variant_print(parameters, TRUE);
+    g_debug("transfer signal: %s %s %s", cucdt_path, signal_name, variant_str);
+    g_free(variant_str);
 
     if (!g_strcmp0(signal_name, "DownloadIdChanged"))
       {
@@ -792,7 +810,9 @@
                                  GVariant*          parameters,
                                  gpointer           gself)
   {
-    g_debug("download signal: %s %s %s", ccad_path, signal_name, g_variant_print(parameters, TRUE));
+    gchar* variant_str = g_variant_print(parameters, TRUE);
+    g_debug("download signal: %s %s %s", ccad_path, signal_name, variant_str);
+    g_free(variant_str);
 
     // Route this signal to the DBusTransfer for processing 
     auto self = static_cast<Impl*>(gself);