Merge lp:~3v1n0/unity/debug-property-matching-fix into lp:unity

[Marco Trevisan (Treviño)]

DebugDBusInterface: match properties if they are in the AP array form [<type>, <value>]

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3782
http://jenkins.qa.ubuntu.com/job/unity-ci/871/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/407
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/403
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/405

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/871/rebuild

review: Approve (continuous-integration)

[Christopher Lee (veebers)]

Looks good to me. I ran the autopilot tests and all but 10 passed (an those 10 failures weren't obviously related to this branch).

It would be good to have someone else familiar with the code (i.e. on the unity team) to approve this code too.

I see there are no tests with this fix, I understand there isn't any testing for this area already setup and thus would be a bit of a commitment.
Perhaps if some of the tests were updated to use select_single/select_many + attribute selections that could add a small amount of coverage (as it's actively exercising this code).

[Christopher Lee (veebers)]

Actually I'm going to double back on my comment :-\

After conferring with Thomi he reminded me that all propertie value should always be an array (type, value).

The unpacking would be best to go into GetPropertyValue so it returns the value of the property (not the property array as it currently does).

review: Disapprove

[Marco Trevisan (Treviño) (3v1n0)]

Mh, wait... GetPropertyValue isn't also called to fetch the properties? And thus why should we hide the type from the returned value?

[Marco Trevisan (Treviño) (3v1n0)]

Branch updated to match veebers' review

[Christopher Lee (veebers)]

Nice, that's much better. Cheers :-).

On IRC I brought up concerns that:
  We know how the data will be packed (an array [<type>, <value>]) and
  thus we shouldn't need the recursion as we should be able to do the
  unpack on diff-line 17.
  This is fine for simple (int, bool, string) types but not the
  complex types (i.e. Point) which might be a Variant -> Variant ->
  Variant.
  This shouldn't be an issue as GetPropertyValue is only
  called in Match[int,bool,string]Property (complex type matching is
  done client side i.e. within autopilot).

I bring this up as the code currently is it adds complexity (in the
form of more lines of code) that isn't currently required. This might
also gloss over any break in the xpathselect 1.4 spec in regards to
data layout.

As it stands this code works and resolves the issue.

Can I get another Unity team member to sanity check the code too please?

[Christopher Lee (veebers)]

Ugh, sorry to flip-flop but after reading the xpathselect protocol docs[1] again it looks like GetPropertyValue is incorrect.

The data format is [type_id, val, ..(more possible vals)..]

So for instance if there is a query like //typename[globalRect=0] the actual property would be something like: [TYPE_RECT, 0, 0, 100, 100].

Currently UnpackPropertyValue would return [TYPE_RECT, 0, 0, 100, 100] (due to line 14) so that is wrong.
Assuming the removal of that check it would return 0 (the 2nd element in the array) which is also wrong.

I think what we actually want here is to remove the first (0th) element in that array and return the remainder. (so [TYPE_SIMPLE, 12] will return 12)
There shouldn't need a check for an array, as the protocol states that it should _always_ be an array and if it's not it is an error.

[1] http://developer.ubuntu.com/api/devel/ubuntu-14.04/autopilot/appendix/protocol.html#returning-state-data

review: Disapprove

[Marco Trevisan (Treviño) (3v1n0)]

> Ugh, sorry to flip-flop but after reading the xpathselect protocol docs[1]
> again it looks like GetPropertyValue is incorrect.
>
> The data format is [type_id, val, ..(more possible vals)..]
>
> So for instance if there is a query like //typename[globalRect=0] the actual
> property would be something like: [TYPE_RECT, 0, 0, 100, 100].
>
> Currently UnpackPropertyValue would return [TYPE_RECT, 0, 0, 100, 100] (due to
> line 14) so that is wrong.

Yes, but then the matching functions will refuse it, since it's not a simple type they know. So at the end nothing will match and this is correct.

> Assuming the removal of that check it would return 0 (the 2nd element in the
> array) which is also wrong.

That would be wrong, at the end.

> I think what we actually want here is to remove the first (0th) element in
> that array and return the remainder. (so [TYPE_SIMPLE, 12] will return 12)
> There shouldn't need a check for an array, as the protocol states that it
> should _always_ be an array and if it's not it is an error.

The protocol might support also other "raw" types (or more complex) as we do for launcher icon per-monitor states (and they are still in array under the type-0 case, so the checks are needed anyway). BTW, everything is in a such array already (done by IntrospectionData).

So, the only change (not actually relevant to make this to work) we might do is to ensure that the child_value[0] == 0, but this isn't either so relevant since the only other type that will actually might match is the "Date or Date/Time" format, which is the only one we have in the form [TYPE_ID, SIMPLE_VALUE], and I wouldn't consider this matching an error.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3783
http://jenkins.qa.ubuntu.com/job/unity-ci/895/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-amd64-ci/431
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-armhf-ci/427
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-trusty-i386-ci/429

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-ci/895/rebuild

review: Approve (continuous-integration)

[Christopher Lee (veebers)]

Awesome, thanks Marco. LGTM. The test in the linked bug now passes.

review: Approve

=== modified file 'unity-shared/DebugDBusInterface.cpp'
--- unity-shared/DebugDBusInterface.cpp	2014-01-15 16:04:41 +0000
+++ unity-shared/DebugDBusInterface.cpp	2014-04-23 22:09:26 +0000
@@ -84,7 +84,8 @@
       {
         if (!g_variant_is_of_type(prop_value, G_VARIANT_TYPE_STRING))
         {
-          LOG_WARNING(logger) << "Unable to match '"<< name << "', it's not a string property.";
+          LOG_WARNING(logger) << "Unable to match '"<< name << "', '" <<
+                                  prop_value << "' is not a string property.";
           return false;
         }
 
@@ -102,7 +103,8 @@
       {
         if (!g_variant_is_of_type(prop_value, G_VARIANT_TYPE_BOOLEAN))
         {
-          LOG_WARNING(logger) << "Unable to match '"<< name << "', it's not a boolean property.";
+          LOG_WARNING(logger) << "Unable to match '"<< name << "', '" <<
+                                  prop_value << "' is not a boolean property.";
           return false;
         }
 
@@ -138,7 +140,8 @@
           case G_VARIANT_CLASS_UINT64:
             return static_cast<uint64_t>(value) == prop_value.GetUInt64();
         default:
-          LOG_WARNING(logger) << "Unable to match '"<< name << "' against property of unknown integer type.";
+          LOG_WARNING(logger) << "Unable to match '"<< name << "', '" <<
+                                  prop_value << "' is not a known integer property.";
         };
       }
 
@@ -152,7 +155,24 @@
 
       IntrospectionData introspection;
       node_->AddProperties(introspection);
-      return g_variant_lookup_value(glib::Variant(introspection.Get()), name.c_str(), nullptr);
+
+      glib::Variant value(g_variant_lookup_value(glib::Variant(introspection.Get()), name.c_str(), nullptr), glib::StealRef());
+
+      if (!value)
+        return nullptr;
+
+      if (!g_variant_is_of_type(value, G_VARIANT_TYPE_ARRAY) || g_variant_n_children(value) != 2)
+      {
+        LOG_ERROR(logger) << "Property value for '"<< name << "' should be a 2-sized array, got instead '" << value << "'";
+        return nullptr;
+      }
+
+      glib::Variant child(g_variant_get_child_value(value, 1), glib::StealRef());
+
+      if (g_variant_is_of_type(child, G_VARIANT_TYPE_VARIANT))
+        return child.GetVariant();
+
+      return child;
     }
 
     std::vector<xpathselect::Node::Ptr> Children() const