GNOME Bugzilla – Bug 752141
sorting: Allow adding sorting clauses to an existing GomSorting
Last modified: 2015-07-10 16:45:37 UTC
.
Created attachment 307106 [details] [review] sorting: Create the queue in init This is where it should have been from the start, otherwise using g_object_new() won't have the queue created.
Created attachment 307107 [details] [review] sorting: Allow adding sorting clauses to an existing GomSorting This allows another way to use GomSorting objects. In addition, it makes it possible to use GomSorting from GI-based bindings. As it turns out, GObject Introspection does not support variadic functions, so gom_sorting_new just isn't usable from GI-based bindings. But with this commit, GI-based bindings can now simply create a new, empty GomSorting object, then add their sorting clauses to it.
Review of attachment 307106 [details] [review]: ++
Review of attachment 307107 [details] [review]: ::: gom/gom-sorting.c @@ +186,3 @@ + * Add a new ORDER BY clause to the sorting object. + * + * This allows chaining ORDER BY clases, adding them one at a time, rather classes. @@ +190,3 @@ + * + * Example: + * Missing some markup to make this show up as C. See the gdbus or gvariant docs to see how. @@ +199,3 @@ + * The above example maps to the following SQL statement: + * + * ORDER BY 'episodes'.'season-number' DESC, 'episodes'.'episode-number' Ditto. @@ +200,3 @@ + * + * ORDER BY 'episodes'.'season-number' DESC, 'episodes'.'episode-number' + */ You should mention in the _new() that it won't work for bindings, and link to this function's docs. @@ +214,3 @@ + + o->resource_type = resource_type; + o->property_name = strdup(property_name); Nope. g_strdup(). So you can free with g_free(). Please check other uses of strdup() in your original patch.
(In reply to Bastien Nocera from comment #4) > Review of attachment 307107 [details] [review] [review]: > > ::: gom/gom-sorting.c > @@ +186,3 @@ > + * Add a new ORDER BY clause to the sorting object. > + * > + * This allows chaining ORDER BY clases, adding them one at a time, rather > > classes. I actually meant "clauses" here. > @@ +190,3 @@ > + * > + * Example: > + * > > Missing some markup to make this show up as C. See the gdbus or gvariant > docs to see how. > > @@ +199,3 @@ > + * The above example maps to the following SQL statement: > + * > + * ORDER BY 'episodes'.'season-number' DESC, 'episodes'.'episode-number' > > Ditto. Both fixed in the next patch version. Note that the GomSorting docs actually weren't built at all, so I'm adding a patch to do that. > @@ +200,3 @@ > + * > + * ORDER BY 'episodes'.'season-number' DESC, 'episodes'.'episode-number' > + */ > > You should mention in the _new() that it won't work for bindings, and link > to this function's docs. Fixed in the next version of the patch. > @@ +214,3 @@ > + > + o->resource_type = resource_type; > + o->property_name = strdup(property_name); > > Nope. g_strdup(). So you can free with g_free(). Good thing you mentioned g_free, that made me realize I wasn't free-ing that string at all :x > Please check other uses of strdup() in your original patch. That's the only one, fixed in the next version of the patch.
Created attachment 307152 [details] [review] doc: Add GomSorting to the documentation
Created attachment 307153 [details] [review] sorting: Improve the gom_sorting_new documentation This adds language markup, and the proper # signs in front of types.
Created attachment 307154 [details] [review] sorting: Allow adding sorting clauses to an existing GomSorting This allows another way to use GomSorting objects. In addition, it makes it possible to use GomSorting from GI-based bindings. As it turns out, GObject Introspection does not support variadic functions, so gom_sorting_new just isn't usable from GI-based bindings. But with this commit, GI-based bindings can now simply create a new, empty GomSorting object, then add their sorting clauses to it.
Review of attachment 307152 [details] [review]: gom-sections.txt needs to be updated as well.
Review of attachment 307153 [details] [review]: Looks good otherwise. ::: gom/gom-sorting.c @@ +113,3 @@ * gom_sorting_new: (constructor) * @first_resource_type: A subclass of #GomResource. + * @first_property_name: A pointer to a const #gchar. "A string" is good enough. If you need to link to the char docs, I think the reader might need something more basic ;)
Review of attachment 307154 [details] [review]: Looks good otherwise. ::: gom/gom-sorting.c @@ +41,3 @@ +gom_order_by_term_free (GomOrderByTerm *term) +{ + g_free (term->property_name); We don't use spaces before parenthesis in gom. ::: gom/gom-sorting.h @@ +63,3 @@ GomSortingMode first_sorting_mode, ...); +void gom_sorting_add (GomSorting *sorting, Alignment of the "*sorting" variable name.
(In reply to Bastien Nocera from comment #9) > Review of attachment 307152 [details] [review] [review]: > > gom-sections.txt needs to be updated as well. That file is automatically generated. And I just checked, it does contain the GomSorting stuff.
Review of attachment 307152 [details] [review]: Looks good then.
Attachment 307152 [details] pushed as 9d2bca5 - doc: Add GomSorting to the documentation
Created attachment 307198 [details] [review] sorting: Improve the gom_sorting_new documentation This adds language markup, and the proper # signs in front of types.
Created attachment 307199 [details] [review] sorting: Properly free the GomOrderByTerm We were leaking the property_name string.
Created attachment 307200 [details] [review] sorting: Allow adding sorting clauses to an existing GomSorting This allows another way to use GomSorting objects. In addition, it makes it possible to use GomSorting from GI-based bindings. As it turns out, GObject Introspection does not support variadic functions, so gom_sorting_new just isn't usable from GI-based bindings. But with this commit, GI-based bindings can now simply create a new, empty GomSorting object, then add their sorting clauses to it.
Review of attachment 307198 [details] [review]: Yep.
Review of attachment 307199 [details] [review]: Looks good.
Review of attachment 307200 [details] [review]: Looks good.
Attachment 307198 [details] pushed as ab60def - sorting: Improve the gom_sorting_new documentation Attachment 307199 [details] pushed as 38ec658 - sorting: Properly free the GomOrderByTerm Attachment 307200 [details] pushed as e7a8801 - sorting: Allow adding sorting clauses to an existing GomSorting