Code review comment for lp:~3v1n0/unity/debug-property-matching-fix

Revision history for this message
Marco Trevisan (TreviƱo) (3v1n0) wrote :

> 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.

« Back to merge proposal