Add support for metadata API

[mgeier]

See #40.

[mgeier]

@SpotlightKid Thanks for the review!

I don't have any motivation to actively write code for Python 2 support anymore. This PR assumes Python 3.

Do you need Python 2 support?

If yes, could you please make a new PR based on my PR that takes care of Python 2?

[SpotlightKid]

No, I myself decided to drop Python 2 support in my own projects successivley as well. It's just that the readme still mentions even Python 2.6 support :-).

[mgeier]

@SpotlightKid That's a good point! To fix this, I've just created #65, which I'm planning to merge before this PR.

Assuming only Python 3 compatibility, are there still open issues?

Any suggestions for API improvements?

[SpotlightKid]

Any suggestions for API improvements?

1. Should the default value for type really be '' not None?

2. You could add convenience methods to the Client and Port classes:

   • Client.get_property(key)
   • Client.get_properties()
   • Port.get_property(key)
   • Port.get_properties()
   • Port.set_property(key, type=None)
   • Port.remove_property(key)

   The problem is, that the following would conflict with the methods to set/remove properties on other subjects:

   • Client.set_property(key, value, type=None)
   • Client.remove_property(key)

   You could make all parameters keyword parameters and make subject optional, with a default of None, which means, replace it with self.uuid. But it could be confusing and it would mean all other parameters would have to be passed as keyword args.

3. You could allow the subject argument value for all metadata functions and methods to be also a Client or Port instance. Just put the following at the start of the _uuid_parse function with:

    if isinstance(uuid, (Client, Port)):
        uuid = uuid.uuid

4. I think returning the property values as byte strings is a bit inconvenient. Here's how I would handle it:

diff --git a/src/jack.py b/src/jack.py
index 6313e05..b41db6f 100644
--- a/src/jack.py
+++ b/src/jack.py
@@ -2560,21 +2560,29 @@ def _uuid_parse(uuid):
     raise TypeError('Invalid UUID: {!r}'.format(uuid))

-def _description_to_dict(desc):
+def _decode_property(prop, encoding='utf-8'):
+    type_ = _decode(prop.type) if prop.type else None
+    value = _ffi.string(prop.data)
+
+    if not type_ or type_.startswith('text/') and encoding:
+        value = value.decode(encoding, errors='ignore')
+
+    return value, type_
+
+
+def _description_to_dict(desc, encoding):
     assert desc != _ffi.NULL
     prop_dict = {}
     for i in range(desc.property_cnt):
         prop = desc.properties[i]
         key = _decode(prop.key)
-        prop_dict[key] = (
-            _ffi.string(prop.data),
-            _decode(prop.type) if prop.type else '')
+        prop_dict[key] = _decode_property(prop, encoding)
     free_description_itself = 0
     _lib.jack_free_description(desc, free_description_itself)
     return prop_dict

-def get_property(subject, key):
+def get_property(subject, key, encoding='utf-8'):
     """Get a metadata property on *subject*.

     Parameters
@@ -2610,11 +2618,17 @@ def get_property(subject, key):
     if _lib.jack_get_property(subject, key.encode(), data, type) != 0:
         return None
     data = _ffi.gc(data[0], _lib.jack_free)
+    data = _ffi.string(data)
     type = _ffi.gc(type[0], _lib.jack_free)
-    return _ffi.string(data), _decode(type) if type else ''
+    type = _decode(type) if type else None
+
+    if not type or type.startswith('text/') and encoding:
+        data = data.decode(encoding, errors='ignore')
+
+    return data, type

-def get_properties(subject):
+def get_properties(subject, encoding='utf8'):
     """Get all metadata properties of *subject*.

     Parameters
@@ -2647,10 +2661,10 @@ def get_properties(subject):
         raise RuntimeError(
             'Unable to get properties for subject {!r}'.format(subject))
     assert number == desc.property_cnt
-    return _description_to_dict(desc)
+    return _description_to_dict(desc, encoding)

-def get_all_properties():
+def get_all_properties(encoding='utf8'):
     """Get all properties for all subjects with metadata.

     Returns
@@ -2679,7 +2693,7 @@ def get_all_properties():
     for idx in range(number):
         desc = descs + idx
         assert desc.subject not in prop_dict
-        prop_dict[desc.subject] = _description_to_dict(desc)
+        prop_dict[desc.subject] = _description_to_dict(desc, encoding)
     return prop_dict

You could still passencoding=None to get a bytes value.

[mgeier]

Thanks a lot for those suggestions!

Should the default value for type really be '' not None?

I was considering this, but even when I was passing NULL as "type" argument to jack_set_property(), I didn't get NULL out from jack_get_property().

Probably I did something wrong. Did you try this?

You could add convenience methods to the Client and Port classes

You already mentioned some problems with this. Since I need a Client object for creating/deleting any property, it would be confusing to have one method that acts on self and a similarly named method that acts on a given "subject".

I think it is more consistent if the "subject" always has to be explicit.

Yet another problem is that changing properties on Port objects would not work, because they don't have an associated client. This would only work with OwnPort objects.

You could allow the subject argument value for all metadata functions and methods to be also a Client or Port instance.

Good idea, I've added your suggestion in d79be4a118acad1abcd02dc0e2d6c3afc0838aba.

I think returning the property values as byte strings is a bit inconvenient.

That's true, but I think encoding=None would not be understood as disabling the decode() step, but rather as using the default encoding, which is UTF-8.

Also, we shouldn't expose an encoding parameter without also having an errors parameter.

I don't really like the type.startswith('text/') heuristic in such a low-level module. I think such a thing should only be used in user code or probably in a wrapper module with more specific use cases.

Finally, I'm skeptical whether it is a good idea to return different types from a function depending on external factors.

The JACK docs state that when type is empty, the data is expected to be UTF-8 encoded. I was thinking about returning one of two types:

• (b'some data', 'some type')
• 'a Unicode string'

But then users wouldn't be able to use tuple unpacking and I think the handling would be more complicated. I think this is easier to handle:

• (b'some data', 'some type')
• (b'some UTF-8 encoded data', '')

If I understood correctly, your suggestion would return one of those:

• (b'some data', 'some type')
• ('a Unicode string', 'text/something')
• ('a Unicode string', '')

Wouldn't this make user code more complicated because they always have to check if they got a str or bytes back from the function call?

[SpotlightKid]

but even when I was passing NULL as "type" argument to jack_set_property(), I didn't get NULL out from jack_get_property(). [...] Did you try this?

Yes, when using ctypes, I get a null pointer back, when the type is not set. I think that's just an implementation detail of how cffi works?

[SpotlightKid]

I concur with the rest of your points, though having some more user-friendly wrapper code would probably be nice.

[mgeier]

Yes, when using ctypes, I get a null pointer back, when the type is not set.

OK, I tried it again, and it turns out I was mistaken about getting NULL back from jack_get_property().

If I set NULL, I get NULL, that works fine. But if I set an empty string, I don't get an empty string back (instead I get NULL).

I think that was the reason why I chose to not support type=None.

Are you getting an empty string back?

having some more user-friendly wrapper code would probably be nice.

I agree, but I think adding this to the Client class would be more confusing than helpful.

I think I would prefer providing a very basic low-level API, and users can create their own wrappers based on their specific use cases.

[SpotlightKid]

I think that was the reason why I chose to not support type=None.

Huh? I don't understand the logic behind that. Anyway, setting type to an empty string would be against the spec, since it should be either a mime-type or a URI.

[mgeier]

I think that was the reason why I chose to not support type=None.

Huh? I don't understand the logic behind that.

If I set the type to '', I would expect to get a type of '' back, but instead I get NULL (or None, when translated to Python).

If I allow both type=None and type='', one of them will behave inconsistently.

Therefore, I think it's better to allow only one of them.

Since disallowing type='' would be really strange (wouldn't it?), I think it makes much more sense to disallow type=None.

Does that argument make sense?

Anyway, setting type to an empty string would be against the spec, since it should be either a mime-type or a URI.

According to http://jackaudio.org/api/structjack__property__t.html#a298b7c97464c5ac3a6070c467732bd9c:

"If type is NULL or empty, the data is assumed to be a UTF-8 encoded string (text/plain)." (emphasis mine)

Do you have a different spec?

[SpotlightKid]

Do you have a different spec?

No, I just didn't interpret "empty" as an empty string ;)

If I allow both type=None and type='', one of them will behave inconsistently.

OTOH, I would find it surprising that if no type argument is given to set_property it will result in type being set to an empty string.

If I set the type to '', I would expect to get a type of '' back, but instead I get NULL

Which strengthens my belief that the intention of the JACK developers was, that if type is set, it should be a string containing something (a mime-type or a URI).

Maybe this should be raised as an issue with JACK? Have you tested the behavior of Jack2 (current git)?

[mgeier]

OTOH, I would find it surprising that if no type argument is given to set_property it will result in type being set to an empty string.

The function signature of set_property() clearly shows type=''.

There is no reason for a user to believe that type should ever be anything other than a str.

Would you expect None? What would make you expect that?

If I set the type to '', I would expect to get a type of '' back, but instead I get NULL

Which strengthens my belief that the intention of the JACK developers was, that if type is set, it should be a string containing something (a mime-type or a URI).

I don't think so.

I think that's just them dealing with an idiosyncrasy of C, where a "string" can be either a NULL pointer or a pointer to some zero-terminated characters.

There is no reason to provide two different representations for "empty" (NULL pointer and pointer to '\0'), but C forces them to handle both cases.

No other language has to worry about those two cases and can simply use the empty string to signify "empty".

Maybe this should be raised as an issue with JACK?

I don't think that's an issue. But feel free to open an issue there if you think there is one!

Have you tested the behavior of Jack2 (current git)?

No, I didn't. But whatever its behavior is, I think my current solution would work (and return '' for empty types).

[SpotlightKid]

Sorry for not having answered yet.

I think we we both made our views clear now and it's up to you to decide what the API should look like in the end. I'm looking forward to using it.

[SpotlightKid]

Just a little ping... anything still preventing this from being merged?

[mgeier]

@SpotlightKid Sorry for the late response and thanks for the reminder!

I've just merged the current state, but if you have further suggestions for changes feel free to comment here or make a new PR!