Merge lp:~brendan-donegan/checkbox/bug1314516_checkbox-gui into lp:checkbox

[Brendan Donegan]

This branch updates the GUI to use the summary field of a job in all cases in the UI, falling back to the partial_id of the job if the summary is not available. For local jobs it will display the description where the summary is not available.

[Zygmunt Krynicki (zyga)]

Please split that to two commits, one per project

review: Needs Fixing

[Zygmunt Krynicki (zyga)]

I also wonder if plainbox tests pass with those changes, perhaps we just have spotty coverage

[lp:~brendan-donegan/checkbox/bug1314516_checkbox-gui]

2978. By Brendan Donegan on 2014-05-07

plainbox: If summary does not exist, use partial_id

Signed-off-by: Brendan Donegan <email address hidden>

2979. By Brendan Donegan on 2014-05-07

checkbox-gui: Display summary where possible, otherwise partial_id

To make the UI of plainbox based tools consistent, let's use summary when
displaying jobs. However these is currently no guarantee summary is there
for any particular job, so we need to fall back to using partial_id.

To keep behaviour consistent, local jobs should fall back to using
description instead of partial_id.

Signed-off-by: Brendan Donegan <email address hidden>

[Brendan Donegan (brendan-donegan)]

Rebased. Overall coverage is 52%, but in e.g. job.py it's 91%, so there might be room for improvement, but it's not that bad.

review: Needs Resubmitting

[Zygmunt Krynicki (zyga)]

26 + variant = *iface->properties.find("summary");
27 + if (variant.isValid() && variant.canConvert(QMetaType::QString) ) {
28 + testname = variant.toString();
29 + }
30 +

When would that be false? We always return something for summary

review: Needs Information

[Brendan Donegan (brendan-donegan)]

That's just the pattern used in the current code - it's probably better to be defensive, don't you think?

review: Needs Resubmitting

[Zygmunt Krynicki (zyga)]

Ok, I see, I was just wondering if there was something else to it.

[Zygmunt Krynicki (zyga)]

+1

review: Approve

=== modified file 'checkbox-gui/checkbox-gui/testitemmodel.cpp'
--- checkbox-gui/checkbox-gui/testitemmodel.cpp	2014-04-07 09:07:29 +0000
+++ checkbox-gui/checkbox-gui/testitemmodel.cpp	2014-05-07 13:01:19 +0000
@@ -106,6 +106,7 @@
         double duration;
         QString checksum;
         QString depends;
+        QString partial_id;
         QString testname;
         QString requires;
         QString description;
@@ -143,9 +144,6 @@
         // Should we show this to the user at all?
         bool human = true;
 
-        // Local jobs use description as the visible name
-        bool description_as_name = false;
-
         // The path for this job is:
         path = jnode->m_node->object_path.path();
 
@@ -178,9 +176,14 @@
                         description = variant.toString();
                     }
 
+                    variant = *iface->properties.find("summary");
+                    if (variant.isValid() && variant.canConvert(QMetaType::QString) ) {
+                        testname = variant.toString();
+                    }
+
                     variant = *iface->properties.find("partial_id");
                     if (variant.isValid() && variant.canConvert(QMetaType::QString) ) {
-                        testname = variant.toString();
+                        partial_id = variant.toString();
                     }
 
                     variant = *iface->properties.find("requires");
@@ -205,8 +208,12 @@
                             type = tr("Manual");
                         }
 
+                        // local jobs should display description if there's no summary
+                        // not partial_id
                         if (variant.toString().compare("local") == 0) {
-                            description_as_name = true;
+                            if (testname == partial_id) {
+                                testname = description;
+                            }
                         }
 
                         if (variant.toString().compare("resource") == 0) {
@@ -235,14 +242,6 @@
             depth++;
         }
 
-        // For local jobs, we may substitute the description for the human name
-        if (description_as_name) {
-            // dont do this if the description is empty however!
-            if (!description.isEmpty()) {
-                testname = description;
-            }
-        }
-
         // Does this node have children?
         bool branch = false;
         if (!jnode->m_children.isEmpty()) {

=== modified file 'plainbox/plainbox/impl/job.py'
--- plainbox/plainbox/impl/job.py	2014-03-28 21:48:09 +0000
+++ plainbox/plainbox/impl/job.py	2014-05-07 13:01:19 +0000
@@ -270,14 +270,14 @@
 
     @property
     def summary(self):
-        return self.get_record_value('summary', self.id)
+        return self.get_record_value('summary', self.partial_id)
 
     def tr_summary(self):
         """
         Get the translated version of :meth:`summary`
         """
         return self.get_normalized_translated_data(
-            self.get_raw_record_value('summary')) or self.id
+            self.get_raw_record_value('summary')) or self.partial_id
 
     @property
     def name(self):