[Bug 725221] Add GstUri object for URI handling

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=725221
  GStreamer | gstreamer (core) | git

--- Comment #23 from David Waring <david.waring at rd.bbc.co.uk> 2014-09-22 15:46:32 UTC ---
(In reply to comment #19)
> Review of attachment 282251 [details]:
> 
> Looks very good in general, just some minor comments
> 
> ::: gst/gsturi.c
> @@ +909,3 @@
> + * using the algorithm described in RFC3986.
> + *
> + * Last reviewed on never ()
> 
> Remove that part :)
> 
Done.

> @@ +929,3 @@
> + * fragment NULL.
> + *
> + * Returns: A new GstUri object. [transfer full]
> 
> No need to add gtk-doc documentation for private functions
> 
Removed those although did leave in an explanation of one function as a simple
comment.

> @@ +1458,3 @@
> + * escaped except where indicated.
> + *
> + * Returns: (transfer full): A new #GstUri object.
> 
> Add Since: 1.6 at the end of the gtk-doc comments of every public function
> 
Done.

> @@ +1778,3 @@
> +{
> +  const gchar *r_scheme;
> +  GstUri *t;
> 
> Add guards to *all* public functions, e.g. here
> 
> g_return_val_if_fail (base_uri != NULL, NULL);
> g_return_val_if_fail (ref_uri != NULL, NULL);
> 
Done.

> @@ +2433,3 @@
> + * gst_uri_set_query_table:
> + * @uri: (transfer none)(nullable): The #GstUri to modify.
> + * @query_table: (transfer none)(nullable): The new query table to use.
> 
> Not sure if a GHashTable is ideal for this. Are the GI annotations to tell
> about the key and value types?
> 
Yes, those detail the key and value types.

> Maybe we need a GstUriQueryElement struct, and then here use a GList of
> GPtrArray of them?
> 
I've left this as a GHashTable for now as I didn't want to reinvent a key/value
pair type. I thought the hash table would be the best fit here as most of the
time you'd want to lookup a query value string by it's key name, or insert
key/value pairs. I couldn't see a situation where this structure wouldn't fit,
so it seemed best. It maybe worth revisiting later.

> @@ +2560,3 @@
> + * Get a list of the query keys from the URI.
> + *
> + * Returns: (transfer full)(element-type gchar*): A list of keys from
> 
> This is "transfer container" as the gchar* don't have to be freed
> 
Done.

> ::: gst/gsturi.h
> @@ +173,3 @@
> + */
> +typedef struct _GstUri GstUri;
> +struct _GstUri
> 
> Make this struct private and put it into the .c file. If any direct accesses
> are needed, add getters (and setters?) for them

Moved this to the .c file along with a few inline functions which no longer
have access directly to the internal parts of the structure.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.