API problems with differentiating between set and unset value of unsigned property.
@krnowak
Submitted by Krzesimir Nowak Link to original bug (#683581)
Description
There is a bunch of functions taking or returning gint or int for properties of type xsd:unsignedInt. While I understand that for some of those properties it is pretty harmless, because no one is going to set a e.g. res@bitrate to value > 2^31 and, in addition, it gives us a convenient possibility to unset the property by passing a value < 0, but it creates two problems:
-
Some properties have to be unsigned, like objectUpdateID, because value of SystemUpdateID is assigned to it and spec clearly describes a situation when SystemUpdateID reaches value of 2^32 - 1. With current gupnp-av it will never reach this point, because with SystemUpdateID reaching value of 2^31 the objectUpdateID would be simply unset if we stored it as a signed int in GUPnPDIDLLiteObject and marked -1 as unsetting value.
-
Inconsistent API to recognize whether the property is set or not. This stems from 1) as we cannot use magic -1 to mark it as unset.
-
There is a need to guard against writing negative values for properties of type xsd:unsignedInt.
API problems:
gupnp_didl_lite_container_[gs]et_child_count - should use guint, should use %u for g_strdup_printf, otherwise should guard against value being < 0
gupnp_didl_lite_resource_[gs]et_size(64)? - should use guint64, should use G_GUINT64_FORMAT, don't know about the "passing -1 to unset the property".
gupnp_didl_lite_resource_[gs]et_duration - can't get subsecond duration.
gupnp_didl_lite_resource_[gs]et_bitrate - should use guint, should use %u, don't know about the "passing -1 to unset the property". OTOH I don't know if it is sensible to expect that bitrate will be greater than 2^31.
gupnp_didl_lite_resource_[gs]et_sample_freq - see bitrate
gupnp_didl_lite_resource_[gs]et_bits_per_sample - see bitrate
gupnp_didl_lite_resource_[gs]et_audio_channels - see bitrate
gupnp_didl_lite_resource_[gs]et_(width|height) - could add just one set_resolution taking two guints and add unset_resolution.
gupnp_didl_lite_resource_[gs]et_color_depth - see bitrate
Probably there are more.
In general, for all properties that have None as default value an unset and is_set functions could be provided.
Instead of is_set functions, getters could take a pointer to gboolean saying whether the property was set. But that is rather unfriendly for vala bindings.
Also, when creating DIDL object from XML, it could be parsed once and all errors reported then instead of converting text content to numbers everytime we ask for some numeric property which text content could be invalid (LG FTW) - currently XML utils ignore possible conversion errors. Thus when we are getting some unsigned value we don't know if that 0 we got is because the property is really 0 or whether the property wasn't set or whether there was some parsing error.
Differentiating between 0/FALSE and unset values could be solved in several ways:
// 4 functions per property, yuck. uint get_foo(); bool is_foo_set(); void set_foo(uint val); void unset_foo();
OR
// unfriendly for vala property getter uint get_foo(bool& is_set); void set_foo(uint val); void unset_foo();
OR
// yet another type. UINTVAL { uint val; bool is_set }; UINTVAL get_foo(); void set_foo(UINTVAL val);
OR
// could work if we stored property values in Priv. null means unset uint* get_foo() void set_foo(uint*)
Every of above four ideas is ugly in its way I think. I wasn't even thinking about throwing exceptions when trying to get unset property or using GValues with G_TYPE_INVALID for unset property - those would be super awful.
Version: 0.10.x