On 03/01/13 21:30, Philip Van Hoof wrote:
Hey Guys,
Hello Philip,
Again, is somebody planning to review this? If not I'm planning to go
ahead and push it to master.
I have just reviewed it briefly. My comments:
1. There are some white space issues here which need cleaning up.
Trailing white space, etc. see
src/libtracker-extract/tracker-extract-sparql.c.
2. Coding style too.
3. Shouldn't you use the tracker_sparql_* API properly in
sparql_builder_finish() ? WE're appending strings here but we have APIs
for inserting URNs and blank notes already which could make this a lot
easier. We should use these:
tracker_sparql_builder_object_blank_open() and _close()
(unless you need to refer to _:file later on of course)
and
tracker_sparql_builder_object_iri()
3. Extra spaces before end of function are unnecessary ;)
+ g_clear_error (&error);
+
4. I would have made sparql_builder_finish() take arguments in the order
we use in TrackerExtractInfo for consistency, but that's me being pedantic.
5. tracker_extract_get_sparql() takes a "temp_file", is teh file
temporary? Is it a URL or a path? we should be more specific with
variables like this especially if this is new API.
6. No need for the check here, just strdup it... also, I would keep the
variable name the same so it's clear what it is we're passing (i.e. a urn).
+ if (graph) {
+ data->graph_urn = g_strdup (graph);
+ }
7. Why not use -1 or a valid time_t for last_mod and last_access? Not
sure why we need the extra _set booleans here.
+ if (last_mod != 0) {
+ data->last_mod = last_mod;
+ data->last_mod_set = TRUE;
+ } else {
+ data->last_mod_set = FALSE;
+ }
8. This looks a bit wrong to me. First, we always set the file from the
temp_file variable given, but then we might set the url to be something
different based on dest_url. Not only is this confusing but I expect it
to cause problems inside tracker if the file used for the URL != file
used for the PATH. I can see why you want to do this, but we didn't do
it this way round with Nokia, we issued a separate update for the file
details when it actually was saved as something else. The problem I see
is that you could have a query which returns urn->path as /foo and
urn->uri as file:///bar and that's clearly wrong:
+ data->storage = tracker_storage_new ();
+ data->file = g_file_new_for_path(temp_file);
+ if (dest_url) {
+ data->url = g_strdup (dest_url);
+ } else {
+ data->url = g_file_get_uri (data->file);
+ }
You could ALWAYS set the url from the file and keep the dest_url as a
separate struct member to help avoid any confusion and I think it would
make a lot more sense here.
9. No need to use a variable here, you can just return the function
value with a cast I imagine:
+ res = g_simple_async_result_get_op_res_gpointer (simple);
+
+ return res;
10. Please put system includes before glib and libraries (coding style):
+#include <glib.h>
+#include <gio/gio.h>
+#include <time.h>
+
11. I wonder why you don't provide a sync API too?
12. This is wrong too:
+#endif /* __LIBTRACKER_EXTRACT_ENCODING_H__ */
13. Copyright should probably be 2013 ;)
14. Why do we change src/libtracker-common/tracker-marshal.list at all?
15. I've not checked the whole on_fileinfo_received() part because it's
really complex in the miner-fs already and we were fixing problems there
for years related to parent existence and creation prior to insertion of
a child path. This worries me about this branch because I get the
impression from quick review that it's not really addressed fully. You
also seem to allow parent URNs to not exist before adding a child to the
database. That's not how we've done things in the miner-fs and this
really breaks what people using the DB can expect. Correct me if I am wrong.
16. I presume the tracker-storage.[ch] addition to libtracker-extract is
a straight copy of the files from libtracker-miner? I am not sure if
this is such a good idea - but I've not been sure what to do here for a
while now. From a quick git grep on the code, it seems we only use it in
tracker-writeback, tracker-extract and tracker-miner-fs, and since
miner-fs already links with libtracker-extract, it looks like the
logical choice to move it there.
17. If you do plan to add tracker-storage.[ch] to libtracker-extract,
you need to update the documentation so people know it wasn't added in
0.8 but in 0.16. ;)
18. White space issues in src/libtracker-miner/Makefile.am
19. I didn't see any patches for the documentation and since you're
moving the whole tracker-storage.[ch] API to another library, you
definitely need to do that.
20. For tracker-sparql, I would prefer a cleaner approach. Right now,
it's really clear the utility is used for querying mainly. I think it
would be better to use:
"insert-file-path" instead of "metadata-file-path"
i.e. insert instead of metadata for all new options there.
It's then much clearer when looking at the options that they're about
specifying data to insert. It's also shorter :)
I also think that "metadata-file-path" is a bit long, can't we just use
"insert-path"? The argument clearly states it's a FILE too. Same for
"metadata-graph-urn", "insert-graph" is shorter and URN is the type it
expects. Same again for "metadata-dest-url", would be better as
"insert-url".
Finally, I think that "metadata-available" needs to have a proper value
type instead of NULL. It's not clear if "true" or "1" or "yes" are
really the right thing to put in here, we should give an example of the
boolean system being used here in the description text if it's still not
clear.
21. White space issues in tracker-sparql.c
22. The reason I asked for a sync API is so we could use it in
tracker-sparql ;)
--
NOTE: I have not compiled or tested this branch yet, this is just my
review of the code.
I can do some testing after some of the issues above are addressed - but
I am a bit concerned to see it just committed in it's current state.
Thanks for the work here all the same Philip!
--
Regards,
Martyn
Founder and CEO of Lanedo GmbH.
_______________________________________________
tracker-list mailing list
tracker-list@gnome.org
https://mail.gnome.org/mailman/listinfo/tracker-list
