Merge lp:~charlesk/indicator-transfer/rtm-usability-fix into lp:indicator-transfer/14.10

[Charles Kerr]

Change the indicator's label to "Files" and only show the indicator if there are unfinished transfers.

== Description of Change

Update label and visibility to match changes listed in <https://docs.google.com/a/canonical.com/document/d/1jHIGzAzf7kFELgOiU9J0Hd9mOa3NzTsnEglLmx_qEfk/edit#>:

1. Change the title from "Transfers" to "Files"

2. Change the visibility behavior s.t. the indicator is only visible when there are incomplete transfers.

== Checklist

> Are there any related MPs required for this MP to build/function as expected? Please list.

No

> Is your branch in sync with latest trunk? (e.g. bzr pull lp:trunk -> no changes)

Yes

> Did the code build without warnings?

Yes

> Did the tests run successfully?

Yes

> Did you perform an exploratory manual test run of your code change and any related functionality?

Yes

> Has your component test plan been executed successfully on emulator or a physical device?

Yes, mako + r68

> Please list which manual tests are germane for the reviewer in this MP.

indicator-transfer/visibility

> Did you link to the checklist URL at https://wiki.ubuntu.com/Process/Merges/Checklists/indicator-transfer to make life easier for reviewers/testers?

Yes

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Ted Gould (ted)]

Small stuff, I'll mark approved but not top approve.

review: Approve

[lp:~charlesk/indicator-transfer/rtm-usability-fix]

26. By Charles Kerr on 2014-10-03

fix questionable use of 'const auto'

27. By Charles Kerr on 2014-10-03

fix syntax error in manual tests

28. By Charles Kerr on 2014-10-03

in test-view-gmenu's PhoneHeader test, improve local variablenames

29. By Charles Kerr on 2014-10-03

in test-view-gmenu.cpp, extract copy-and-pasted blocks into a function to reduce duplicated code

[Charles Kerr (charlesk)]

Fixed; comments inline

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

=== modified file 'src/view-gmenu.cpp'
--- src/view-gmenu.cpp	2014-08-25 21:00:22 +0000
+++ src/view-gmenu.cpp	2014-10-03 21:09:42 +0000
@@ -238,7 +238,7 @@
                           g_variant_new_string("accessible-desc"));
     g_variant_builder_add(&b, "{sv}", "label", g_variant_new_string("label"));
     g_variant_builder_add(&b, "{sv}", "title", g_variant_new_string("title"));
-    g_variant_builder_add(&b, "{sv}", "visible", g_variant_new_boolean(true));
+    g_variant_builder_add(&b, "{sv}", "visible", g_variant_new_boolean(false));
     return g_variant_builder_end(&b);
   }
 
@@ -419,17 +419,29 @@
     return ret;
   }
 
+  /* Show the header if there are any transfers that have begun and are
+     currently incomplete because they're either ongoing or paused. */
+  bool header_should_be_visible() const
+  {
+    for (const auto& transfer : m_model->get_all())
+      if (transfer->state != Transfer::FINISHED)
+        return true;
+
+    return false;
+  }
+
   GVariant* create_header_state()
   {
     auto reffed_icon_v = get_header_icon();
-    auto title_v = g_variant_new_string(_("Transfers"));
+    auto title_v = g_variant_new_string(_("Files"));
+    const bool visible = header_should_be_visible();
 
     GVariantBuilder b;
     g_variant_builder_init(&b, G_VARIANT_TYPE_VARDICT);
     g_variant_builder_add(&b, "{sv}", "title", title_v);
     g_variant_builder_add(&b, "{sv}", "icon", reffed_icon_v);
     g_variant_builder_add(&b, "{sv}", "accessible-desc", title_v);
-    g_variant_builder_add(&b, "{sv}", "visible", g_variant_new_boolean(true));
+    g_variant_builder_add(&b, "{sv}", "visible", g_variant_new_boolean(visible));
     auto ret = g_variant_builder_end (&b);
 
     g_variant_unref(reffed_icon_v);

=== modified file 'tests/manual'
--- tests/manual	2014-08-25 21:37:15 +0000
+++ tests/manual	2014-10-03 21:09:42 +0000
@@ -1,11 +1,19 @@
 
+Test-case indicator-transfer/visibility
+<dl>
+    <dt>Start downloading an image from the Browser to the Gallery.
+        (In the Browser, tap-and-hold on an image, choose 'Save image'
+        in the popup, and select 'Gallery' at the 'Open with ' prompt.</dt>
+        <dd>If the indicator wasn't already visible in the panel, it should appear as the transfer begins.</dd>
+        <dd>After the last transfer finished, the indicator should disappear from the panel.</dd>
+</dt>
+
 Test-case indicator-transfer/download-an-image
 <dl>
-    <dt>Ensure indicator-transfer-service is running<dt>
-        <dd>The indicator should be visible in the panel</dd>
     <dt>Start downloading an image from the Browser to the Gallery.
         (In the Browser, tap-and-hold on an image, choose 'Save image'
-        in the popup, and select 'Gallery' at the 'Open with ' prompt.</dt>
+        in the popup, and select 'Gallery' at the 'Open with ' prompt.)</dt>
+        <dd>If the indicator wasn't already visible in the panel, it should appear as the transfer begins.</dd>
         <dd>In the 'Ongoing Transfers' section, the transfer should appear</dd>
         <dd>In the 'Ongoing Transfers' section, a 'Pause all' button should appear</dd>
         <dd>While transfer is active, the indicator's icon should indicate activity</dd>
@@ -13,6 +21,7 @@
         <dd>As the transfer finishes, its menuitem should move from the 'Ongoing' to 'Successful' section</dd>
         <dd>As the transfer finishes, the indicator's icon should change to indicate an idle state</dd>
         <dd>As the transfer finishes, the 'Pause all' button should disappear</dd>
+        <dd>After the transfer is finished, the indicator should disappear from the panel.</dd>
     <dt>Click on the completed transfer's menuitem</dt>
         <dd>The gallery app should launch</dd>
     <dt>Press the 'Clear all' button.</dt>

=== modified file 'tests/test-view-gmenu.cpp'
--- tests/test-view-gmenu.cpp	2014-06-25 23:02:06 +0000
+++ tests/test-view-gmenu.cpp	2014-10-03 21:09:42 +0000
@@ -24,6 +24,7 @@
 #include <transfer/dbus-shared.h>
 #include <transfer/view-gmenu.h>
 
+#include <glib/gi18n.h>
 
 using namespace unity::indicator::transfer;
 
@@ -268,6 +269,132 @@
 
 /***
 ****
+****   Header
+**** 
+***/
+
+namespace
+{
+  bool is_header_visible(GActionGroup* action_group, const std::string& header_action_name)
+  {
+    auto dict = g_action_group_get_action_state(action_group, header_action_name.c_str());
+    g_assert(dict != nullptr);
+    g_assert(g_variant_is_of_type(dict, G_VARIANT_TYPE_VARDICT));
+    auto v = g_variant_lookup_value(dict, "visible", G_VARIANT_TYPE_BOOLEAN);
+    g_assert(v != nullptr);
+    const bool is_visible = g_variant_get_boolean(v);
+    g_clear_pointer(&v, g_variant_unref);
+    g_clear_pointer(&dict, g_variant_unref);
+    return is_visible;
+  }
+}
+
+TEST_F(GMenuViewFixture, PhoneHeader)
+{
+  const std::string profile = "phone";
+
+  wait_msec();
+
+  auto connection = g_bus_get_sync(G_BUS_TYPE_SESSION, nullptr, nullptr);
+
+  // SETUP: get the action group and wait for it to be populated
+  auto dbus_action_group = g_dbus_action_group_get(connection, BUS_NAME, BUS_PATH);
+  auto action_group = G_ACTION_GROUP(dbus_action_group);
+  auto names_strv = g_action_group_list_actions(action_group);
+  if (g_strv_length(names_strv) == 0)
+    {
+      g_strfreev(names_strv);
+      wait_for_signal(dbus_action_group, "action-added");
+      names_strv = g_action_group_list_actions(action_group);
+    }
+  g_clear_pointer(&names_strv, g_strfreev);
+
+  // SETUP: get the menu model and wait for it to be activated
+  auto phone_dbus_menu_model = g_dbus_menu_model_get(connection, BUS_NAME, BUS_PATH"/phone");
+  auto phone_menu_model = G_MENU_MODEL(phone_dbus_menu_model);
+  int n = g_menu_model_get_n_items(phone_menu_model);
+  if (!n)
+    {
+      // give the model a moment to populate its info
+      wait_msec(100);
+      n = g_menu_model_get_n_items(phone_menu_model);
+    }
+  EXPECT_TRUE(phone_menu_model != nullptr);
+  EXPECT_NE(0, n);
+
+  // test to confirm that a header menuitem exists
+  gchar* str = nullptr;
+  g_menu_model_get_item_attribute(phone_menu_model, 0, "x-canonical-type", "s", &str);
+  EXPECT_STREQ("com.canonical.indicator.root", str);
+  g_clear_pointer(&str, g_free);
+  g_menu_model_get_item_attribute(phone_menu_model, 0, G_MENU_ATTRIBUTE_ACTION, "s", &str);
+  const auto action_name = profile + "-header";
+  EXPECT_EQ(std::string("indicator.")+action_name, str);
+  g_clear_pointer(&str, g_free);
+
+  // cursory first look at the header
+  auto dict = g_action_group_get_action_state(action_group, action_name.c_str());
+  EXPECT_TRUE(dict != nullptr);
+  EXPECT_TRUE(g_variant_is_of_type(dict, G_VARIANT_TYPE_VARDICT));
+  auto v = g_variant_lookup_value(dict, "accessible-desc", G_VARIANT_TYPE_STRING);
+  EXPECT_TRUE(v != nullptr);
+  g_variant_unref(v);
+  v = g_variant_lookup_value(dict, "title", G_VARIANT_TYPE_STRING);
+  EXPECT_TRUE(v != nullptr);
+  EXPECT_STREQ(_("Files"), g_variant_get_string(v, nullptr));
+  g_variant_unref(v);
+  v = g_variant_lookup_value(dict, "visible", G_VARIANT_TYPE_BOOLEAN);
+  EXPECT_TRUE(g_variant_get_boolean(v));
+  EXPECT_TRUE(v != nullptr);
+  g_clear_pointer(&v, g_variant_unref);
+  g_clear_pointer(&dict, g_variant_unref);
+
+  // Visibility test #1:
+  // Change the model to all transfers finished.
+  // Confirm that the header is not visible.
+  for (auto& transfer : m_model->get_all())
+    {
+      transfer->state = Transfer::FINISHED;
+      m_model->emit_changed(transfer->id);
+    }
+
+  wait_msec(200);
+  EXPECT_FALSE(is_header_visible(action_group, action_name));
+
+  // Visibility test #2:
+  // Change the model to all transfers finished except one running.
+  // Confirm that the header is visible.
+  auto transfer = m_model->get("a");
+  transfer->state = Transfer::RUNNING;
+  m_model->emit_changed(transfer->id);
+  wait_msec(200);
+  EXPECT_TRUE(is_header_visible(action_group, action_name));
+
+  // Visibility test #3:
+  // Change the model to all transfers finished except one paused.
+  // Confirm that the header is visible.
+  transfer = m_model->get("a");
+  transfer->state = Transfer::PAUSED;
+  m_model->emit_changed(transfer->id);
+  wait_msec(200);
+  EXPECT_TRUE(is_header_visible(action_group, action_name));
+
+  // Visibility test #4:
+  // Remove all the transfers from the menu.
+  // Confirm that the header is not visible.
+  for (const auto& id : m_model->get_ids())
+    m_model->remove(id);
+  wait_msec(200);
+  EXPECT_FALSE(is_header_visible(action_group, action_name));
+
+  // cleanup
+  g_clear_object(&action_group);
+  g_clear_object(&phone_dbus_menu_model);
+  g_clear_object(&connection);
+}
+
+/***
+****
 ****  GMenu
 ****
 ***/
@@ -296,4 +423,3 @@
   g_clear_object(&dbus_menu_model);
   g_clear_object(&connection);
 }
-